Fix handling of invalid compact blocks #9026

Merged
merged 3 commits into from Nov 8, 2016

Projects

None yet

6 participants

@sdaftuar
Contributor

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.

@kazcw
Contributor
kazcw commented Oct 26, 2016

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?

@laanwj laanwj added the P2P label Oct 27, 2016
@sdaftuar
Contributor

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

@sdaftuar
Contributor

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.

@sdaftuar
Contributor
sdaftuar commented Nov 1, 2016 edited

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.

@TheBlueMatt

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.

@@ -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);
@TheBlueMatt
TheBlueMatt Nov 1, 2016 Contributor

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

@kazcw
kazcw Nov 1, 2016 Contributor

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
sipa Nov 3, 2016 Member

Agree with @kazcw here.

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

@sdaftuar
Contributor
sdaftuar commented Nov 3, 2016

Draft BIP update is here: bitcoin/bips#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.

sdaftuar added some commits Oct 26, 2016
@sdaftuar sdaftuar [qa] Test that invalid compactblocks don't result in ban c93beac
@sdaftuar sdaftuar Fix compact block handling to not ban if block is invalid 88c3549
@sdaftuar sdaftuar 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.
d4833ff
@sdaftuar
Contributor
sdaftuar commented Nov 3, 2016

Rebased.

@gmaxwell
Member
gmaxwell commented Nov 8, 2016

ACK

@sipa
Member
sipa commented Nov 8, 2016

utACK d4833ff

@sipa sipa merged commit d4833ff into bitcoin:master Nov 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sipa sipa added a commit that referenced this pull request Nov 8, 2016
@sipa sipa Merge #9026: Fix handling of invalid compact blocks
d4833ff Bump the protocol version to distinguish new banning behavior. (Suhas Daftuar)
88c3549 Fix compact block handling to not ban if block is invalid (Suhas Daftuar)
c93beac [qa] Test that invalid compactblocks don't result in ban (Suhas Daftuar)
dc6b940
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment