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

1. 
   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.
2. 
   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

3. Sjors force-pushed on Mar 6, 2019
4. fanquake added the label Docs on Mar 6, 2019
5. 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.
   

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

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

   It’s arguably the same issue as CVE-2017-12842
9. 
   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.

10. DrahtBot closed this on Mar 9, 2020

    ---

11. 
    DrahtBot commented at 8:34 pm on March 9, 2020: member
12. DrahtBot reopened this on Mar 9, 2020

    ---

13. DrahtBot added the label Needs rebase on Jun 2, 2021
14. [doc] explain why CheckBlock() is called before AcceptBlock()

    Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

    3d552b0d78
15. Sjors force-pushed on Jun 3, 2021
16. 
    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…
17. 
    sdaftuar commented at 5:19 pm on June 3, 2021: member

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

    cr ACK 3d552b0d788a7d3102396b32d0de08e57cbfd297
20. MarcoFalke merged this on Jun 4, 2021
21. MarcoFalke closed this on Jun 4, 2021

    ---

22. Sjors deleted the branch on Jun 4, 2021
23. sidhujag referenced this in commit 7149e02756 on Jun 4, 2021
24. gwillen referenced this in commit 35c1809ce9 on Jun 1, 2022
25. DrahtBot locked this on Aug 16, 2022

Contributors
Sjors sdaftuar luke-jr DrahtBot MarcoFalke

Labels
Docs