Bugfix: Restore ProcessBlock's ability to return with CValidationState set properly #5090

pull luke-jr wants to merge 4 commits into bitcoin:master from luke-jr:bugfix_processblock changing 3 files +108 −33

luke-jr commented at 3:53 AM on October 16, 2014: member

In cases where ProcessBlock does not in fact complete validation, CValidationState returns with a "inconclusive" state.
CValidationState: Track state as inconclusive until successfully connected a5ae0c8c2f
Abstract context-dependent block checking from acceptance e5d66f3e3d
in src/main.cpp:None in badaa8ad1d outdated
```
2031 | @@ -2003,10 +2032,19 @@ bool ActivateBestChain(CValidationState &state, CBlock *pblock) {
2032 |  
2033 |              // Whether we have anything to do at all.
2034 |              if (pindexMostWork == NULL || pindexMostWork == chainActive.Tip())
2035 | -                return true;
2036 | +                goto out;
```
rebroad commented at 2:21 PM on October 16, 2014:

We use gotos?! I thought this was usually considered bad programming practice.

luke-jr commented at 4:30 PM on October 16, 2014:

Only misuse is bad programming practice. In this case, is makes the code more readable.

btcdrak commented at 4:34 PM on October 16, 2014:

Wouldn't a simple break; suffice here given you are in a while loop?

in src/main.cpp:None in badaa8ad1d outdated

2039 | -                return false;
2040 | +            CValidationState stateSpecific;
2041 | +            if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, phashSpecific, &stateSpecific))
2042 | +            {
2043 | +                ret = false;
2044 | +                goto out;

btcdrak commented at 4:34 PM on October 16, 2014:

Wouldn't a simple break; suffice here?

luke-jr force-pushed on Oct 19, 2014
luke-jr commented at 11:46 PM on October 19, 2014: member

Refactored on top of headers first, including removal of gotos and cleaner flow.
Bugfix: Track final validation of blocks through ActivateBestChain, restoring the expectation that ProcessBlock gives some kind of final CValidationState for blocks
Should fix submitblock returning false positives (issue #5083)
e10aa0403c
CreateNewBlock and miner_tests: Check generated template is valid by ProcessBlockChecks (and therefore also AcceptBlockChecks) 03aafd8cd5
luke-jr force-pushed on Oct 20, 2014
luke-jr commented at 1:45 AM on October 20, 2014: member

Seems like we're going a different route to fix #5083 ...
luke-jr closed this on Oct 20, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

luke-jr

rebroad

btcdrak