Skip to content

Conversation

sdaftuar
Copy link
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.

@fanquake
Copy link
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.

@jtimon
Copy link
Contributor

jtimon commented Jan 3, 2016

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

utACK sdaftuar@00318f4

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].

@sdaftuar
Copy link
Member Author

sdaftuar commented Jan 4, 2016

@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.

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.
@sdaftuar sdaftuar force-pushed the eliminate-extra-checkblock branch from 00318f4 to dbb89dc Compare February 1, 2016 19:31
@sdaftuar
Copy link
Member Author

sdaftuar commented Feb 1, 2016

Needed rebase.

@jtimon
Copy link
Contributor

jtimon commented Feb 2, 2016

Re-utACK dbb89dc

@laanwj
Copy link
Member

laanwj commented Feb 3, 2016

utACK dbb89dc

@laanwj laanwj merged commit dbb89dc into bitcoin:master Feb 3, 2016
laanwj added a commit that referenced this pull request Feb 3, 2016
dbb89dc Eliminate unnecessary call to CheckBlock (Suhas Daftuar)
@laanwj laanwj mentioned this pull request Feb 16, 2016
3 tasks
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants