Eliminate unnecessary call to CheckBlock #7225

1. 
   sdaftuar commented at 6:52 pm on December 17, 2015: member

   ProcessNewBlock would return failure early if CheckBlock failed, before calling AcceptBlock. AcceptBlock also calls CheckBlock, and upon failure would update mapBlockIndex to indicate that a block was failed. By returning early in ProcessNewBlock, we were not marking blocks that fail a check in CheckBlock as permanently failed, and thus would continue to re-request and reprocess them.

   This should result in one fewer call to CheckBlock for valid blocks, at the expense of one extra call to AcceptBlockHeader for blocks that fail CheckBlock, and it avoids reprocessing those failed blocks over and over.

2. 
   fanquake commented at 0:27 am on December 26, 2015: member

   Concept ACK. If we’re going to remove this call can we also drop the CheckBlockHeader call inside CheckBlock ?

   As the comment in CheckBlock states, the call is mostly redundant with the call in AcceptBlockHeader. With this change, we only reach CheckBlock after calling AcceptBlock, which calls AcceptBlockHeader, which has called CheckBlockHeader.

3. 
   jtimon commented at 7:32 pm on January 3, 2016: contributor

   CheckBlock is also called in ConnectBlock() [which is the function that does ALL the consensus checks for a block].

   utACK https://github.com/sdaftuar/bitcoin/commit/00318f47e549d14b26e149cd1442c642187295a0

   I’m also fine with taking CheckBlockHeader() out of CheckBlock() as @fanquake suggests [as long as it’s properly moved out where needed, for example, in ConnectBlock()].

   As a side note this would very slightly simplify https://github.com/jtimon/bitcoin/commits/libconsensus-f2 so I consider this “libconsensus encapsulation friendly”(TM) [even if nobody ever asked for that kind of approval from me].

4. 
   sdaftuar commented at 4:43 pm on January 4, 2016: member

   @fanquake I think I’d prefer to leave refactoring CheckBlockHeader() out of CheckBlock() for someone to try in another pull, as it’s not obvious to me that change makes sense, and would at the least require more changes to other call sites as @jtimon points out.
5. laanwj added the label Validation on Jan 18, 2016
6. Eliminate unnecessary call to CheckBlock

   ProcessNewBlock would return failure early if CheckBlock failed, before
   calling AcceptBlock.  AcceptBlock also calls CheckBlock, and upon failure
   would update mapBlockIndex to indicate that a block was failed.  By returning
   early in ProcessNewBlock, we were not marking blocks that fail a check in
   CheckBlock as permanently failed, and thus would continue to re-request and
   reprocess them.

   dbb89dc793
7. sdaftuar force-pushed on Feb 1, 2016
8. 
   sdaftuar commented at 7:31 pm on February 1, 2016: member

   Needed rebase.
9. 
   jtimon commented at 4:04 pm on February 2, 2016: contributor

   Re-utACK dbb89dc
10. 
    laanwj commented at 11:47 am on February 3, 2016: member

    utACK dbb89dc
11. laanwj merged this on Feb 3, 2016
12. laanwj closed this on Feb 3, 2016

    ---

13. laanwj referenced this in commit eb331794a2 on Feb 3, 2016
14. MarcoFalke locked this on Sep 8, 2021

Contributors
sdaftuar fanquake jtimon laanwj

Labels
Validation