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

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:

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.

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

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.

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