Skip to content

Conversation

defijesus
Copy link

Just a small code refractoring. But we should keep only one return in each function in order to improve readability and usability.

@defijesus
Copy link
Author

Travis CI: No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

@jonasschnelli
Copy link
Contributor

Travis fails with
test_bitcoin: key.cpp:198: void ECC_Start(): Assertion secp256k1_context == __null' failed.
unknown location(0): fatal error in "script_PushData": signal: SIGABRT (application abort requested)`
Problem location: https://github.com/bitcoin/bitcoin/blob/master/src/key.cpp#L198

Looks like a libsecp256k issue (@sipa)?
Same travis problem in #6509.

@@ -64,11 +64,12 @@ class CValidationState {
return mode == MODE_ERROR;
}
bool IsInvalid(int &nDoSOut) const {
bool isInvalid = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing code which spans over two lines to spread over six lines can only decrease readability.

@maflcko
Copy link
Member

maflcko commented Aug 4, 2015

This PR does the exact opposite of what the "block style example" says in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding

@laanwj
Copy link
Member

laanwj commented Aug 4, 2015

Too risky for what is gained, IMO. We don't usually merge these kinds of random code refactors because - even when they do improve readability - in the past some very nasty bugs have been introduced by them, accidentally.

That these changes are in consensus/validation code compounds the above-mentioned risks.

If your goal is to make the code more understandable, adding of comments is preferable.

@laanwj laanwj closed this Aug 4, 2015
@defijesus
Copy link
Author

Hi!
@laanwj I agree this is a small change in a very critical code. But if test pass and build pass. How does adding a new variable to store the return value should produce any bugs? (This kind of change is too simple to fail).
Thank you so much for reviewing

@laanwj
Copy link
Member

laanwj commented Aug 4, 2015

@pouta The tests hardly ever cover all possible cases. I agree that there is a class of changes that is too simple to fail, but just as well there are some seeming innocent changes that break bug-for-bug compatibility which is very important in the consensus code.

@jonasschnelli opened an issue for that, #6515. Looks like secp256k1 context is the victim, not the cause of the problem.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants