Be stricter in processing unrequested blocks

sdaftuar commented at 5:49 pm on March 10, 2015: member

The duplicate block handling code in AcceptBlock needed to be updated for autoprune (see #5863 ), since we no longer HAVE_DATA for every block previously processed. This change significantly tightens requirements on which blocks we process, so that the only blocks we process are either ones we’ve requested (from a network peer or from disk), or are new blocks that also have more work than our tip.

I believe this is a somewhat substantial behavior change to now link network behavior (ie, whether we’ve requested a block) to block processing. If we adopt this change, I think it would be easier for us to reduce our reliance on checkpoints in the future.

[I had been discussing this with @sipa on irc in the context of #5863, but after implementing it I realized that it’s actually not tied to autoprune at all and makes more sense to evaluate on its own.]

laanwj added the label P2P on Mar 11, 2015

jgarzik commented at 1:52 pm on March 11, 2015: contributor

What is a good way to test this in isolation, sans autoprune?

sdaftuar commented at 7:52 pm on March 11, 2015: member

I don’t know of a great answer to that question… I have been separately working on creating a python p2p framework for testing, and I have a test coded up in that framework that exercises this code for blocks received on the network (I manually tested the -reindex case of reading blocks from disk). If you’d like to take a look, you can see it in my repo at:

https://github.com/sdaftuar/bitcoin/commit/9a013ed41fd07217675b78bbb55e13575f0bb4e1

The test is called accept-block-test.py (run it with “–testbinary=path-to-bitcoind”, with and without the “–whitelist” argument, to cover both cases).

I think the test itself is a reasonable length but it builds on a lot of code that is still a work in progress, so I’m not sure how useful this is as a reference. But if you do take a look at this, I’d certainly welcome any feedback you have on the testing framework itself.

in src/main.cpp: in f0d6cef482 outdated

2708@@ -2706,9 +2709,9 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex,
2709     if (!AcceptBlockHeader(block, state, &pindex))
2710         return false;
2711 
2712-    if (pindex->nStatus & BLOCK_HAVE_DATA) {
2713-        // TODO: deal better with duplicate blocks.
2714-        // return state.DoS(20, error("AcceptBlock(): already have block %d %s", pindex->nHeight, pindex->GetBlockHash().ToString()), REJECT_DUPLICATE, "duplicate");
2715+    if (!fRequested && (pindex->nTx != 0 || pindex->nChainWork <= chainActive.Tip()->nChainWork)) {

sipa commented at 1:08 pm on March 12, 2015:

I think the BLOCK_HAVE_DATA test can remain here: there’s no point storing a block for which we already have the data.

sipa commented at 1:14 pm on March 12, 2015: member

Concept ACK. I think it should be safe to even never process unrequested blocks, but things like @TheBlueMatt’s fast relayer would probably stop working without it.

sdaftuar force-pushed on Mar 12, 2015

sdaftuar commented at 4:02 pm on March 12, 2015: member

Fixed to re-include the BLOCK_HAVE_DATA check as well.

in src/main.cpp: in 53a39c5f9b outdated

2708@@ -2706,9 +2709,11 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex,
2709     if (!AcceptBlockHeader(block, state, &pindex))
2710         return false;
2711 
2712-    if (pindex->nStatus & BLOCK_HAVE_DATA) {
2713-        // TODO: deal better with duplicate blocks.
2714-        // return state.DoS(20, error("AcceptBlock(): already have block %d %s", pindex->nHeight, pindex->GetBlockHash().ToString()), REJECT_DUPLICATE, "duplicate");
2715+    if ((pindex->nStatus & BLOCK_HAVE_DATA) ||
2716+            (!fRequested && (pindex->nTx != 0 || pindex->nChainWork <= chainActive.Tip()->nChainWork))) {

gavinandresen commented at 6:27 pm on March 18, 2015:

Can this be rewritten to make the logic clearer? Something like:

0bool fAlreadyHave = (pindex->nStatus & BLOCK_HAVE_DATA);
1bool fHasMoreWork = (pindex->nChainWork > chainActive.Tip()->nChainWork);
2
3if (fAlreadyHave) return true;
4if (!fRequested) { // If we didn't ask for it:
5   if (pindex->nTx != 0) return true; <-- what is this condition?
6   if (!fHasMoreWork) return true; // don't process less-work chains.
7}

spinza commented at 10:39 am on March 25, 2015: none

@sipa If the relayer is whitelisted it should still work? I see this treats blocks from whitelisted peers as if they were requested.

TheBlueMatt commented at 10:48 am on March 26, 2015: member

Is there no better way to check if we would want to request this block before we just ignore it? It seems to me that it is not ideal that we would jsut ignore data we get from a peer because they’re using a /very/ loose interpretation of the protocol.

sdaftuar commented at 5:20 pm on March 26, 2015: member

@TheBlueMatt I’ve thought about that a bit; I was concerned about that as well. I believe the test we’d like to perform is whether the block we’re processing is on some headers chain that is further along than our tip – if so, we should process it (because this is a block that we’d like to download). The options I could think of to try calculating that, using our existing data structures, are:

Walk every entry in mapBlockIndex and calculate exactly whether this block is the ancestor of a header with more work than our tip.
Loop over all our peers to see if we would try to download this block from one of them (using what we know about each peer’s headers chain).
Look specifically at the one peer who sent us this block to see if we would try to download this from them

I didn’t think any of these solutions really made sense. 1 seems too slow, and both 2 and 3 are approximations for what we’re interested in to begin with, so iterating over all peers as in 2 seemed too costly to me for this approximate optimization. 3 seems to catch too narrow a case to be worth it. If we had a data structure that was already keeping track of headers that are further along than our tip, then that would be nice to use here… Not sure it would make sense to create and maintain such a structure solely for this though.

Regarding peers that are only loosely respecting the network protocol – that’s why I thought adding an allowance for whitelisted peers made sense, so that unusual peers that are known to you can still have their blocks processed. On the other hand, if we’re connecting to an unknown peer that is behaving oddly, how much effort should we expend to differentiate that peer’s behavior from an attacker’s?

TheBlueMatt commented at 10:39 pm on March 26, 2015: member

What about just connecting if it happens to be a logical next block on our current tip? It’s a simple heuristic and would work for one common “loose interpretation” that you see in mining clients/servers/the relay network. More generally, if the peer is making a “loose interpretation”, there are no guarantees it has a sane header chain, so maybe it’s not worth the complexity there.

On March 26, 2015 1:20:36 PM EDT, Suhas Daftuar notifications@github.com wrote:

@TheBlueMatt I’ve thought about that a bit; I was concerned about that as well. I believe the test we’d like to perform is whether the block we’re processing is on some headers chain that is further along than our tip – if so, we should process it (because this is a block that we’d like to download). The options I could think of to try calculating that, using our existing data structures, are:

Walk every entry in mapBlockIndex and calculate exactly whether this block is the ancestor of a header with more work than our tip.

Loop over all our peers to see if we would try to download this block from one of them (using what we know about each peer’s headers chain).

Look specifically at the one peer who sent us this block to see if we would try to download this from them

I didn’t think any of these solutions really made sense. 1 seems too slow, and both 2 and 3 are approximations for what we’re interested in to begin with, so iterating over all peers as in 2 seemed too costly to me for this approximate optimization. 3 seems to catch too narrow a case to be worth it. If we had a data structure that was already keeping track of headers that are further along than our tip, then that would be nice to use here… Not sure it would make sense to create and maintain such a structure solely for this though.

Regarding peers that are only loosely respecting the network protocol – that’s why I thought adding an allowance for whitelisted peers made sense, so that unusual peers that are known to you can still have their blocks processed. On the other hand, if we’re connecting to an unknown peer that is behaving oddly, how much effort should we expend to differentiate that peer’s behavior from an attacker’s?

Reply to this email directly or view it on GitHub: #5875 (comment)

sdaftuar commented at 10:55 pm on March 26, 2015: member

Unless I’m misunderstanding you, this code does do that – this does accept an unrequested, non-duplicate block if it has more work than our tip. So in particular a new block that builds on the tip would be processed, even if unrequested.

sdaftuar commented at 10:57 pm on March 26, 2015: member

Also I pushed up a commit which rewrites the AcceptBlock code as @gavinandresen suggested – I can squash if this looks good.

TheBlueMatt commented at 11:08 pm on March 26, 2015: member

Heh, oops, I saw that code and looked right through it :/

On March 26, 2015 6:55:22 PM EDT, Suhas Daftuar notifications@github.com wrote:

Unless I’m misunderstanding you, this code does do that – this does accept an unrequested, non-duplicate block if it has more work than our tip. So in particular a new block that builds on the tip would be processed, even if unrequested.

Reply to this email directly or view it on GitHub: #5875 (comment)

sdaftuar force-pushed on Mar 31, 2015

sdaftuar commented at 7:10 pm on March 31, 2015: member

Squashed the second commit

in src/main.cpp: in 3484ad4e38 outdated

2715-        return true;
2716+    // Try to process all requested blocks that we don't have, but only
2717+    // process an unrequested block if it's new and has enough work to
2718+    // advance our tip.
2719+    bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA;
2720+    bool fHasMoreWork = (chainActive.Tip() ? pindex->nChainWork > chainActive.Tip()->nChainWork : true);

sipa commented at 5:06 pm on April 8, 2015:

Perhaps use CBlockIndexWorkComparator?

sdaftuar commented at 7:04 pm on April 8, 2015:

Looks like we can’t use that, because the nSequenceId in the CBlockIndex entry isn’t set until the block is actually processed (in ReceivedBlockTransactions). Since it’s initialized to zero when we create the entry, if it has the same work as chainActive.Tip(), then CBlockIndexWorkComparator()(chainActive.Tip(), pindex) will return true due to the not-yet-set nSequenceId.

If pindex has the same work as chainActive.Tip(), I think it’s guaranteed that we don’t need to process it, because we’re either in the process of receiving it for the first time after we received chainActive.Tip(), or we received it in the past and therefore don’t need to process it again, so I think it should be okay to leave this line as is.

sipa commented at 7:28 pm on April 8, 2015:

I would either set an error condition (with or without ban), and returnsl false; or return true. Just logging and returning false may break some assumptions. You can keep things as is now, but leave the comment about better dealing with duplicates then.

You’re right about nSequenceId, but I would like to only have a single place in the code for comparing/ordering potential blocks. Anyway, leave it for now.

in src/main.cpp: in 3484ad4e38 outdated

2720+    bool fHasMoreWork = (chainActive.Tip() ? pindex->nChainWork > chainActive.Tip()->nChainWork : true);
2721+    
2722+    if (fAlreadyHave) return true;
2723+    if (!fRequested) {  // If we didn't ask for it:
2724+        if (pindex->nTx != 0) return true;  // This is a previously-processed block
2725+        if (!fHasMoreWork) return true;     // Don't process less-work chains

sipa commented at 5:08 pm on April 8, 2015:

I think we may want to return false and an error state, so it can be reported etc?

sdaftuar commented at 6:06 pm on April 8, 2015:

Ah, I wasn’t sure about that – since we were previously returning true on a duplicate block, I just kept the behavior the same.

I didn’t think we should add ban points to the peer serving such a block, because it could be that we had requested this block from that peer, and some other oddly-behaving peer on the network (perhaps the relay node or some other whitelisted peer) happened to shove the block to us first. We’d have to do a bit more bookkeeping to distinguish that case from that of a misbehaving peer.

But I think we could just return false and log this (seemingly unlikely) event to the debug log; is that all you’re suggesting?

sdaftuar commented at 6:24 pm on April 8, 2015:

Or maybe some kind of ban score is called for in the last case, where we get an unrequested new block with too little work – I’m not coming up with any scenarios where that could result from legitimate behavior. Perhaps this case is worthy of an immediate ban?

in src/main.cpp: in 3484ad4e38 outdated

2764@@ -2756,21 +2765,22 @@ static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned
2765 }
2766 
2767 
2768-bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp)
2769+bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, bool fRequested, CDiskBlockPos *dbp)

sipa commented at 5:18 pm on April 8, 2015:

“fRequested” is maybe a bit confusing; it’s hard to argue that blocks loaded from external storage are by definition ‘requested’. Perhaps call it ‘fForceProcessing’ or something?

sipa commented at 5:19 pm on April 8, 2015:

I missed the comments in the header definition about it. Feel free to ignore.

sdaftuar force-pushed on Apr 9, 2015

sdaftuar commented at 2:03 pm on April 9, 2015: member

I am not sure how to proceed regarding the return value in AcceptBlock; if we return state.Invalid or state.DoS (I think it does make sense to ban in the case of an unrequested block with too little work), then the code in ProcessMessages would generate a reject message, which could be inconsistent with BIP 61, if I am reading this correctly:

Note: blocks that are not part of the server’s idea of the current best chain, but are otherwise valid, should not trigger reject messages.

I think the protocol here should probably be more nuanced to distinguish between normal block relay of valid but non-main-chain blocks, versus relaying that has DoS properties… But not sure what to do about this in the meantime.

I could return state.Error() (which wouldn’t generate a reject message), but that doesn’t seem like how CValidationState::Error() is meant to be used, as I understand it. Alternatively, I could also add a log message and leave it returning true, if that would be better.

For now, I’ve re-added the TODO comment about dealing better with this situation.

sdaftuar force-pushed on Apr 9, 2015

sdaftuar commented at 6:29 pm on April 9, 2015: member

Rebased.

sdaftuar force-pushed on Apr 11, 2015

sdaftuar force-pushed on Apr 13, 2015

sdaftuar commented at 4:19 pm on April 13, 2015: member

Rebased.

sdaftuar force-pushed on Apr 24, 2015

sdaftuar commented at 2:04 pm on April 24, 2015: member

Rebased

sipa commented at 1:32 pm on April 27, 2015: member

Lightly-tested ACK

sdaftuar force-pushed on May 4, 2015

in src/main.cpp: in 0c35bf9c72 outdated

2743+
2744+    // TODO: deal better with return value and error conditions for duplicate
2745+    // and unrequested blocks.
2746+    if (fAlreadyHave) return true;
2747+    if (!fRequested) {  // If we didn't ask for it:
2748+        if (pindex->nTx != 0) return true;  // This is a previously-processed block that was pruned

sdaftuar commented at 3:20 pm on May 4, 2015:

On further thought, I think I should remove this line.

Initially, I had removed the BLOCK_HAVE_DATA check, so this line would have been needed to keep us from reprocessing blocks that are on disk. Now that the BLOCK_HAVE_DATA check is back, this line would only meaningfully trigger if we were to somehow prune a block that has more work than chainActive.Tip(). That shouldn’t ever really happen, but even if it somehow did (say, because of a call to InvalidateBlock), then I think we’d rather re-process the block and not discard it.

Thoughts?

sipa commented at 1:08 pm on May 5, 2015:

So this is a non-requested, pruned block, which somehow has more work than the tip? I would ignore it. A forced-pushed block should only occur because the peer does not know we already knew about the block, but it seems we do, so I think it’s our responsibility to request it if necessary.

sdaftuar commented at 1:31 pm on May 5, 2015:

Ok, will leave as is.

sdaftuar commented at 3:23 pm on May 4, 2015: member

Rebased off master and added a p2p regression test that exercises this logic.

gavinandresen commented at 2:21 pm on May 13, 2015: contributor

Tested ACK.

in src/main.cpp: in 0c35bf9c72 outdated

2797 
2798     {
2799         LOCK(cs_main);
2800-        MarkBlockAsReceived(pblock->GetHash());
2801+        // Treat all whitelisted peers' blocks as having been requested.
2802+        fRequested = MarkBlockAsReceived(pblock->GetHash()) || (pfrom ? pfrom->fWhitelisted : false) || fRequested;

laanwj commented at 10:07 am on May 21, 2015:

nit: Instead of this long line, I’d prefer to see this written out explicitly, especially so that the MarkBlockAsReceived action doesn’t get lost in the noise:

0fRequested |= MarkBlockAsReceived(pblock->GetHash());
1// Treat all whitelisted peers' blocks as having been requested.
2if (pfrom)
3    fRequested |= pfrom->fWhitelisted;

laanwj commented at 10:14 am on May 21, 2015:

Or maybe even better (as I don’t think the whitelisting policy belongs in AcceptBlock, but in networking code): remove the pfrom check here, move the whitelisted to the call site in ProcessMessage - which is the only one where it matters:

0- ProcessNewBlock(state, pfrom, &block, false, NULL);
1+ // Treat all whitelisted peers' blocks as having been requested.
2+ ProcessNewBlock(state, pfrom, &block, pfrom->fWhitelisted, NULL);

sdaftuar commented at 1:51 pm on May 21, 2015: member

@laanwj That sounds reasonable, thanks for reviewing; I’ve pushed a commit that addresses, I can squash if this looks good.

laanwj commented at 2:57 pm on May 21, 2015: member

Looks good to me. Tested ACK (code reviewed, synced up to block 320700 with #5875 and #5927)

sdaftuar force-pushed on May 21, 2015

sdaftuar commented at 3:09 pm on May 21, 2015: member

Squashed back down to two commits.

Be stricter in processing unrequested blocks

AcceptBlock will no longer process an unrequested block, unless it has not
been previously processed and has more work than chainActive.Tip()

9be0e6837b

P2P regression test for new AcceptBlock behavior aa8c827968

in src/main.h: in b003c8676d outdated

159@@ -160,10 +160,11 @@ void UnregisterNodeSignals(CNodeSignals& nodeSignals);
160  * @param[out]  state   This may be set to an Error state if any error occurred processing it, including during validation/connection/etc of otherwise unrelated blocks during reorganisation; or it may be set to an Invalid state if pblock is itself invalid (but this is not guaranteed even when the block is checked). If you want to *possibly* get feedback on whether pblock is valid, you must also install a CValidationInterface (see validationinterface.h) - this will have its BlockChecked method called whenever *any* block completes validation.
161  * @param[in]   pfrom   The node which we are receiving the block from; it is added to mapBlockSource and may be penalised if the block is invalid.
162  * @param[in]   pblock  The block we want to process.
163+ * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers.
164  * @param[out]  dbp     If pblock is stored to disk (or already there), this will be set to its location.
165  * @return True if state.IsValid()
166  */
167-bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp = NULL);
168+bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, bool fForceProcessing, CDiskBlockPos *dbp);

jtimon commented at 0:01 am on May 22, 2015:

Since you’re changing ProcessNewBlock and AcceptBlock, do you mind to also pass const CChainParams& chainparams to them? If you do it like in https://github.com/jtimon/bitcoin/commit/4868fb0a8912e6176ee886b974c74cf3ccf1ebf8#diff-e8db9b851adc2422aadfffca88f14c91L400 and https://github.com/jtimon/bitcoin/commit/6380f721c65d10c3797544875d396a9c36d71318#diff-e8db9b851adc2422aadfffca88f14c91L169 it will be easy for me to rebase my other changes in that direction later…

sdaftuar commented at 10:03 am on May 24, 2015:

Apologies if I’m misunderstanding, but I think this would be an unrelated change to this pull? I’d like to see this merged in time for 0.11 (I think this behavior change should occur at the same time as pruning), so I’d prefer to limit changes to those that are strictly necessary to support this fix.

jtimon commented at 4:29 pm on May 24, 2015:

A couple of trivial fixup! commits that don’t change behaviour in any way shouldn’t delay getting this merged and would make history cleaner by avoiding changing them before (in which case this would be forced to rebase to solve the conflict) or after, since they can be squashed just before merging. Don’t feel forced to do it, it would just make this PR nicer.

jtimon commented at 4:31 pm on May 24, 2015:

Remember that the total diff would be similar since you’re already touching the same lines that would need to be touched for this.

sdaftuar force-pushed on Jun 2, 2015

sdaftuar commented at 6:24 pm on June 2, 2015: member

This needed rebasing due to the restructuring of the rpc-tests directory.

laanwj merged this on Jun 3, 2015

laanwj closed this on Jun 3, 2015

laanwj referenced this in commit 9d60602444 on Jun 3, 2015

laanwj referenced this in commit 304892fc03 on Jun 3, 2015

laanwj referenced this in commit 2edec4fe68 on Jun 3, 2015

laanwj commented at 2:25 pm on June 3, 2015: member

Backported to 0.11 as 304892fc03b11bf04286619d2a6fe0ba99ee41fc 2edec4fe68c5fe0fefb4c0561e8353462bb439bc

MarcoFalke locked this on Sep 8, 2021

Be stricter in processing unrequested blocks #5875