Fix handling of invalid compact blocks #9026

pull sdaftuar wants to merge 3 commits into bitcoin:master from sdaftuar:fix-invalidcb-handling changing 9 files +81 −21
  1. sdaftuar commented at 6:30 PM on October 26, 2016: member

    BIP 152 allows compact blocks to be relayed prior to full validation, requiring that:

    A node MUST NOT send a cmpctblock message without having validated that the header properly commits to each transaction in the block, and properly builds on top of the existing chain with a valid proof-of-work. A node MAY send a cmpctblock before validating that each transaction in the block validly spends existing UTXO set entries.

    This patch fixes the handling of compact blocks so that after compact block reconstruction, we will not ban a peer if it turns out the block is invalid.

  2. kazcw commented at 8:08 PM on October 26, 2016: contributor

    Block source accounting serves multiple purposes: besides fixing DoS-score updates, doesn't this also eliminate all nLastBlockTime updates and some reject messages for compact blocks?

  3. laanwj added the label P2P on Oct 27, 2016
  4. sdaftuar commented at 1:45 PM on October 27, 2016: member

    Yes, good point -- I'll rework, thanks for the review.

  5. sdaftuar force-pushed on Oct 31, 2016
  6. sdaftuar commented at 2:45 PM on October 31, 2016: member

    Reworked to try to avoid the problems pointed out by @kazcw.

    I don't really like this approach; I was planning to wait for the refactors being done by @TheBlueMatt elsewhere to settle down to try to fix this more cleanly, but he pointed out that we'd want to backport this bugfix to 0.13 so I went ahead and redid this in a way that is close to how the backport would look.

    Due to the code moves that have already happened in master (I think #8865) this didn't cherry-pick cleanly on 0.13, so I'll separately PR the backport for review.

  7. sdaftuar cross-referenced this on Oct 31, 2016 from issue [0.13 backport] Fix handling of invalid compact blocks by sdaftuar
  8. sdaftuar commented at 3:21 PM on November 1, 2016: member

    I discussed this with @TheBlueMatt, who pointed out that FillBlock() was calling CheckBlock() and returning READ_STATUS_INVALID on CheckBlock() failures that weren't due to merkle-root mismatches. This would in turn cause peers to be banned. Since the intention is to only require our peers to check proof-of-work before relaying, I added a way to distinguish a CheckBlock() failure from other kinds of failures, and then in the caller I treat those the same as though CheckBlock() had succeeded -- this results in some extra computation in ProcessNewBlock() if an invalid block is received, but allows us to reuse the code that would mark the block as invalid, send reject messages, etc.

    Also @gmaxwell suggested on IRC (if I understood correctly!) that we bump the protocol version so that we could distinguish between peers that have the old (buggy) banning behavior and peers that have this new behavior, so I did that here as well.

    This all needs a BIP update, so I'll draft one, and I'll also mirror these changes in #9048.

  9. sdaftuar force-pushed on Nov 1, 2016
  10. in src/main.h:None in 19a6f1819c outdated 
     222 | @@ -223,7 +223,7 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
 223 |   * @param[out]  dbp     The already known disk position of pblock, or NULL if not yet stored.
 224 |   * @return True if state.IsValid()
 225 |   */
 226 | -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp);
 227 | +bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool fMayBanPeerIfInvalid);
    TheBlueMatt commented at 5:41 PM on November 1, 2016:

    Nit: would prefer a default argument here so that the diff is simpler.

    kazcw commented at 6:13 PM on November 1, 2016:

    It might be worth defining a simple "export version" wrapper func that just takes (state, params, block), to insulate the mining code that needs to submit blocks from main.cpp internals more generally.

    sipa commented at 7:14 AM on November 3, 2016:

    Agree with @kazcw here.

  11. TheBlueMatt commented at 5:53 PM on November 1, 2016: contributor

    Generally looks good, would like a test to check disconnect/ban on bad PoW, but not strictly required (didnt change that code here anyway).

    I assume the test failure is spurious, if thats the case, general utACK on this.

  12. sdaftuar force-pushed on Nov 1, 2016
  13. TheBlueMatt commented at 12:34 PM on November 3, 2016: contributor

    Note that the follow on to #8969 will remove both the pfrom and the new bool parameter from ProcessNewBlock, so I'd really prefer not to create some wrapper to hide net-related crap when it's moving to the callsite soon (or we can discuss then).

    On November 3, 2016 3:14:46 AM EDT, Pieter Wuille notifications@github.com wrote:

    sipa commented on this pull request.

    @@ -223,7 +223,7 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;

    • @param[out] dbp The already known disk position of pblock, or NULL if not yet stored.
      • @return True if state.IsValid() / -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp); +bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool fMayBanPeerIfInvalid);

    Agree with @kazcw here.

    You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #9026

  14. sdaftuar commented at 1:55 PM on November 3, 2016: member

    Draft BIP update is here: https://github.com/bitcoin/bips/pull/473.

    If there are no objections, my preference would be to merge this as-is without further changes to ProcessNewBlock's arguments and defer those discussions to the refactoring PRs that are in progress.

  15. [qa] Test that invalid compactblocks don't result in ban c93beac43f
  16. Fix compact block handling to not ban if block is invalid 88c35491ab
  17. Bump the protocol version to distinguish new banning behavior.
    This allows future software that would relay compact blocks before
full validation to announce only to peers that will not ban if the
block turns out to be invalid.
    d4833ff747
  18. sdaftuar force-pushed on Nov 3, 2016
  19. sdaftuar commented at 6:15 PM on November 3, 2016: member

    Rebased.

  20. TheBlueMatt cross-referenced this on Nov 3, 2016 from issue Decouple peer-processing-logic from block-connection-logic (#3) by TheBlueMatt
  21. gmaxwell commented at 12:10 AM on November 8, 2016: contributor

    ACK

  22. sipa commented at 1:52 AM on November 8, 2016: member

    utACK d4833ff74701cc97aab733c54adb8f46e602fa5e

  23. sipa merged this on Nov 8, 2016
  24. sipa closed this on Nov 8, 2016
  25. sipa referenced this in commit dc6b9406bd on Nov 8, 2016
  26. TheBlueMatt cross-referenced this on Dec 19, 2016 from issue Relay compact block messages prior to full block connection by TheBlueMatt
  27. OlegGirko cross-referenced this on Aug 1, 2017 from issue Backport Bitcoin PR#9075: Decouple peer-processing-logic from block-connection-logic (#3) by OlegGirko
  28. gladcow cross-referenced this on Mar 1, 2018 from issue Backport compact blocks functionality from bitcoin by gladcow
  29. gladcow referenced this in commit 52f4a60ac1 on Mar 2, 2018
  30. gladcow referenced this in commit f10ba58e88 on Mar 13, 2018
  31. gladcow referenced this in commit 2e2245be5e on Mar 14, 2018
  32. gladcow referenced this in commit df95bcbab1 on Mar 15, 2018
  33. gladcow referenced this in commit 6a9a542f19 on Mar 15, 2018
  34. gladcow referenced this in commit 89325c5b58 on Mar 15, 2018
  35. gladcow referenced this in commit b65e0f0bfc on Mar 15, 2018
  36. gladcow referenced this in commit f79f640bd5 on Mar 24, 2018
  37. gladcow referenced this in commit 0f3ea35bfd on Apr 4, 2018
  38. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
  39. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
  40. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
  41. bitcoin locked this on Sep 8, 2021
Contributors
sdaftuarkazcwTheBlueMattsipagmaxwell
Labels
P2P
Linked (view graph)
#8865 Decouple peer-processing-logic from block-connection-logic#8969 Decouple peer-processing-logic from block-connection-logic (#2)#9048 [0.13 backport] Fix handling of invalid compact blocks#9075 Decouple peer-processing-logic from block-connection-logic (#3)#9375 Relay compact block messages prior to full block connection
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC