[doc] explain why CheckBlock() is called before AcceptBlock #15545
Conversation
|
@sdaftuar if there's a CVE for the v0.13 issue that might be better to link to. Also it would be nice to have a permanent URL for your PDF, which was quite insightful. I would also like to improve the related comment in Merkle to better explain what "treating that identically" means: bitcoin/src/consensus/merkle.cpp Lines 37 to 40 in 6a178e5 |
Sjors
force-pushed
the
2019/03/clarify-checkblock
branch
from
86b53a5 to
00a5e89
Compare
src/validation.cpp
Outdated
|
|
||
| // Ensure that CheckBlock() passes before calling AcceptBlock, as | ||
| // belt-and-suspenders. | ||
| // belt-and-suspenders. When CheckBlock() fails, e.g. due to an unknown |
There was a problem hiding this comment.
I might say something slightly different, to emphasize why we might want to keep this check here, even if we fix all the currently known malleability issues:
Skipping AcceptBlock() for CheckBlock() failures means that we will never mark a block as invalid if
CheckBlock() fails. This is protective against consensus failure if there are any unknown forms of block
malleability that cause CheckBlock() to fail; see e.g. CVE-2012-2459 and
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html. Because CheckBlock() is
not very expensive, the anti-DoS benefits of caching failure (of a definitely-invalid block) are not substantial.
I wasn't sure if a CVE for the 0.13 issue is worth it at this point, anyway linking to the mailing list archive seems sufficient for now? I was planning to also PR a test that will catch a regression with regards to the issue I raised in that email, so I'd be happy to update the code comments at that point as well. |
|
Such a test would be great. Feel free to include this change in your PR in that case. I'll just leave this open so we don't forget. |
|
It's arguably the same issue as CVE-2017-12842 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
| The last travis run for this pull request was 369 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
Co-authored-by: Suhas Daftuar <[email protected]>
Sjors
force-pushed
the
2019/03/clarify-checkblock
branch
from
00a5e89 to
3d552b0
Compare
|
Rebased and switched to @sdaftuar's suggested comment, which I found easier to read 2 years later... |
|
Thanks for updating this, looks good to me. |
|
cr ACK 3d552b0 |
…AcceptBlock 3d552b0 [doc] explain why CheckBlock() is called before AcceptBlock() (Sjors Provoost) Pull request description: Based on https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html and its PDF attachment. ACKs for top commit: MarcoFalke: cr ACK 3d552b0 Tree-SHA512: d1ef39855317853e0e7e051ec6015054d0d227fcdf20281c2c1921056537f1f79044aa1bdd35f46475edd17596fbcae79aeb338c4865b1269a01b158f6cb2ac4
6 participants
Based on https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html and its PDF attachment.