[doc] explain why CheckBlock() is called before AcceptBlock #15545

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2019/03/clarify-checkblock changing 1 files +5 −2

Sjors commented at 12:03 pm on March 6, 2019: member

Based on https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html and its PDF attachment.
Sjors commented at 12:05 pm on March 6, 2019: member

@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: https://github.com/bitcoin/bitcoin/blob/6a178e52618dff0405d2a239c9f3cc0c4582fb50/src/consensus/merkle.cpp#L37-L40
Sjors force-pushed on Mar 6, 2019
fanquake added the label Docs on Mar 6, 2019

in src/validation.cpp:3525 in 00a5e89d6a outdated

3521@@ -3522,7 +3522,9 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
3522         LOCK(cs_main);
3523 
3524         // Ensure that CheckBlock() passes before calling AcceptBlock, as
3525-        // belt-and-suspenders.
3526+        // belt-and-suspenders. When CheckBlock() fails, e.g. due to an unknown

sdaftuar commented at 4:25 pm on March 6, 2019:

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:

0Skipping AcceptBlock() for CheckBlock() failures means that we will never mark a block as invalid if
1CheckBlock() fails.  This is protective against consensus failure if there are any unknown forms of block
2malleability that cause CheckBlock() to fail; see e.g. CVE-2012-2459 and 
3https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html.  Because CheckBlock() is
4not very expensive, the anti-DoS benefits of caching failure (of a definitely-invalid block) are not substantial.

sdaftuar commented at 4:39 pm on March 6, 2019: member

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

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.
Sjors commented at 6:50 pm on March 6, 2019: member

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.
luke-jr commented at 8:38 am on April 17, 2019: member

It’s arguably the same issue as CVE-2017-12842
DrahtBot commented at 7:32 pm on June 9, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.
DrahtBot closed this on Mar 9, 2020
DrahtBot commented at 8:34 pm on March 9, 2020: member
DrahtBot reopened this on Mar 9, 2020
DrahtBot added the label Needs rebase on Jun 2, 2021
[doc] explain why CheckBlock() is called before AcceptBlock()
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
3d552b0d78
Sjors force-pushed on Jun 3, 2021
Sjors commented at 5:12 pm on June 3, 2021: member

Rebased and switched to @sdaftuar’s suggested comment, which I found easier to read 2 years later…
sdaftuar commented at 5:19 pm on June 3, 2021: member

Thanks for updating this, looks good to me.
DrahtBot removed the label Needs rebase on Jun 3, 2021
MarcoFalke commented at 7:50 am on June 4, 2021: member

cr ACK 3d552b0d788a7d3102396b32d0de08e57cbfd297
MarcoFalke merged this on Jun 4, 2021
MarcoFalke closed this on Jun 4, 2021
Sjors deleted the branch on Jun 4, 2021
sidhujag referenced this in commit 7149e02756 on Jun 4, 2021
gwillen referenced this in commit 35c1809ce9 on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors
Sjors sdaftuar luke-jr DrahtBot MarcoFalke

Labels
Docs

[doc] explain why CheckBlock() is called before AcceptBlock #15545

Conflicts