New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate unnecessary call to CheckBlock #7225

Merged
merged 1 commit into from Feb 3, 2016

Conversation

Projects
None yet
4 participants
@sdaftuar
Member

sdaftuar commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Dec 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.

Member

fanquake commented Dec 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 3, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 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.

Member

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.

@laanwj laanwj added the Validation label Jan 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Feb 1, 2016

Member

Needed rebase.

Member

sdaftuar commented Feb 1, 2016

Needed rebase.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Feb 2, 2016

Member

Re-utACK dbb89dc

Member

jtimon commented Feb 2, 2016

Re-utACK dbb89dc

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 3, 2016

Member

utACK dbb89dc

Member

laanwj commented Feb 3, 2016

utACK dbb89dc

@laanwj laanwj merged commit dbb89dc into bitcoin:master Feb 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 3, 2016

Merge #7225: Eliminate unnecessary call to CheckBlock
dbb89dc Eliminate unnecessary call to CheckBlock (Suhas Daftuar)

@laanwj laanwj referenced this pull request Feb 16, 2016

Open

Suboptimal Block Checks #5163

1 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment