Mark blocks with too many sigops as failed #7217

Merged
merged 1 commit into from Jan 5, 2016

Conversation

Projects
None yet
9 participants
@sdaftuar
Member

sdaftuar commented Dec 15, 2015

This unsets the "corruption possible" field in CValidationState when calling CheckBlock on a block with too many sigops. Without this change, bitcoind would re-request such a block rather than mark it as permanently failed.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Dec 15, 2015

Contributor

Hmm, that one's fun.... Yea, thinking briefly about it I don't see how that error could indicate block malleability.

On December 15, 2015 12:46:54 PM PST, Suhas Daftuar notifications@github.com wrote:

This unsets the "corruption possible" field in CValidationState when
calling CheckBlock on a block with too many sigops. Without this
change, bitcoind would re-request such a block rather than mark it as
permanently failed.
You can view, comment on, or merge this pull request online at:

#7217

-- Commit Summary --

  • Mark blocks with too many sigops as failed

-- File Changes --

M src/main.cpp (2)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/7217.patch
https://github.com/bitcoin/bitcoin/pull/7217.diff


Reply to this email directly or view it on GitHub:
#7217

Contributor

TheBlueMatt commented Dec 15, 2015

Hmm, that one's fun.... Yea, thinking briefly about it I don't see how that error could indicate block malleability.

On December 15, 2015 12:46:54 PM PST, Suhas Daftuar notifications@github.com wrote:

This unsets the "corruption possible" field in CValidationState when
calling CheckBlock on a block with too many sigops. Without this
change, bitcoind would re-request such a block rather than mark it as
permanently failed.
You can view, comment on, or merge this pull request online at:

#7217

-- Commit Summary --

  • Mark blocks with too many sigops as failed

-- File Changes --

M src/main.cpp (2)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/7217.patch
https://github.com/bitcoin/bitcoin/pull/7217.diff


Reply to this email directly or view it on GitHub:
#7217

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Dec 15, 2015

Member

A relayer cannot cause this error to trigger without also causing one of the real corruption-possible checks to fail, so concept ACK.

Member

sipa commented Dec 15, 2015

A relayer cannot cause this error to trigger without also causing one of the real corruption-possible checks to fail, so concept ACK.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Dec 15, 2015

Contributor

@sipa and I went back and traced the blame for that line...it looks like when the corruption possible flag was set for this case, it was, indeed needed as it was checked before the merkle tree was checked. As the order has now changed we do, indeed, no longer need this check.

Contributor

TheBlueMatt commented Dec 15, 2015

@sipa and I went back and traced the blame for that line...it looks like when the corruption possible flag was set for this case, it was, indeed needed as it was checked before the merkle tree was checked. As the order has now changed we do, indeed, no longer need this check.

@laanwj laanwj added the Validation label Dec 17, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 5, 2016

Member

utACK

Member

laanwj commented Jan 5, 2016

utACK

@laanwj laanwj merged commit 5246180 into bitcoin:master Jan 5, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jan 5, 2016

Merge pull request #7217
5246180 Mark blocks with too many sigops as failed (Suhas Daftuar)

laanwj added a commit that referenced this pull request Jan 5, 2016

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Jan 5, 2016

Contributor

Will this cause a hard-fork or a soft-fork?

Contributor

rebroad commented Jan 5, 2016

Will this cause a hard-fork or a soft-fork?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 5, 2016

Member

hard-fork or a soft-fork

neither, we'd already never accept such blocks, iiuc?

Member

MarcoFalke commented Jan 5, 2016

hard-fork or a soft-fork

neither, we'd already never accept such blocks, iiuc?

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jan 5, 2016

Contributor

@rebroad neither, this just has to do with avoiding downloading the block in a loop

Contributor

pstratem commented Jan 5, 2016

@rebroad neither, this just has to do with avoiding downloading the block in a loop

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 10, 2016

Member

Is this appropriate to backport to 0.11 and 0.10?

Member

luke-jr commented Jan 10, 2016

Is this appropriate to backport to 0.11 and 0.10?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jan 10, 2016

Member
Member

sipa commented Jan 10, 2016

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 11, 2016

Member

Yes, it should be also really simple to backport.

Member

jtimon commented Jan 11, 2016

Yes, it should be also really simple to backport.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 11, 2016

Member

ACK for backport to 0.10 and 0.11

Member

sdaftuar commented Jan 11, 2016

ACK for backport to 0.10 and 0.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment