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

  8. sdaftuar force-pushed on Nov 1, 2016
  9. in src/main.h: 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.
  10. TheBlueMatt commented at 5:53 pm on November 1, 2016: member

    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.

  11. sdaftuar force-pushed on Nov 1, 2016
  12. TheBlueMatt commented at 12:34 pm on November 3, 2016: member

    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

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

  14. [qa] Test that invalid compactblocks don't result in ban c93beac43f
  15. Fix compact block handling to not ban if block is invalid 88c35491ab
  16. 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
  17. sdaftuar force-pushed on Nov 3, 2016
  18. sdaftuar commented at 6:15 pm on November 3, 2016: member
    Rebased.
  19. gmaxwell commented at 0:10 am on November 8, 2016: contributor
    ACK
  20. sipa commented at 1:52 am on November 8, 2016: member
    utACK d4833ff74701cc97aab733c54adb8f46e602fa5e
  21. sipa merged this on Nov 8, 2016
  22. sipa closed this on Nov 8, 2016
  23. sipa referenced this in commit dc6b9406bd on Nov 8, 2016
  24. gladcow referenced this in commit 52f4a60ac1 on Mar 2, 2018
  25. gladcow referenced this in commit f10ba58e88 on Mar 13, 2018
  26. gladcow referenced this in commit 2e2245be5e on Mar 14, 2018
  27. gladcow referenced this in commit df95bcbab1 on Mar 15, 2018
  28. gladcow referenced this in commit 6a9a542f19 on Mar 15, 2018
  29. gladcow referenced this in commit 89325c5b58 on Mar 15, 2018
  30. gladcow referenced this in commit b65e0f0bfc on Mar 15, 2018
  31. gladcow referenced this in commit f79f640bd5 on Mar 24, 2018
  32. gladcow referenced this in commit 0f3ea35bfd on Apr 4, 2018
  33. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
  34. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
  35. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
  36. DrahtBot locked this on Sep 8, 2021


sdaftuar kazcw TheBlueMatt sipa gmaxwell

Labels
P2P

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: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me