Check that new headers are not a descendant of an invalid block (more effeciently)

TheBlueMatt commented at 9:15 pm on October 19, 2017: member

@sdaftuar pointed out that the version in #11487 was somewhat DoS-able as someone could feed you a valid chain that forked off the the last checkpoint block and force you to do lots of work just walking backwards across blocks for each new block they gave you. We came up with a few proposals but settled on the one implemented here as likely the simplest without obvious DoS issues. It uses our existing on-load mapBlockIndex walk to make sure everything that descends from an invalid block is marked as such, and then simply caches blocks which we attempted to connect but which were found to be invalid. To avoid DoS issues during IBD, this will need to depend on #11458.

Includes tests from #11487.

TheBlueMatt force-pushed on Oct 19, 2017

fanquake added the label P2P on Oct 19, 2017

in src/validation.cpp:3090 in 98fa1df591 outdated

3085@@ -3066,6 +3086,19 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state
3086             return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");
3087         if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev, GetAdjustedTime()))
3088             return error("%s: Consensus::ContextualCheckBlockHeader: %s, %s", __func__, hash.ToString(), FormatStateMessage(state));
3089+
3090+        for (const CBlockIndex* failedit : g_failed_blocks) {

sdaftuar commented at 1:52 am on October 20, 2017:

What do you think about bypassing this logic in the event that pindexPrev is VALID_SCRIPTS? I’m slightly concerned about some kind of weird scenario where g_failed_blocks grows (hopefully slowly!) over time, slowing down block processing the longer a node has been up.

TheBlueMatt commented at 7:35 pm on October 20, 2017:

Done

in src/validation.cpp:3099 in 9bbd66a9a2 outdated

3094+                while (invalid_walk != failedit) {
3095+                    invalid_walk->nStatus |= BLOCK_FAILED_CHILD;
3096+                    setDirtyBlockIndex.insert(invalid_walk);
3097+                    invalid_walk = invalid_walk->pprev;
3098+                }
3099+                return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");

Sjors commented at 9:03 am on October 20, 2017:

It would be nice to specifically test for bad-prevblk being thrown, perhaps using the mechanism in #11220. I’m not sure how to do that in the python tests though.

Sjors commented at 9:36 am on October 20, 2017:

Another place to test this would be to add a test for ProcessNewBlockHeaders. I can take a stab at it, but I’m not sure which file that test should be added too, or which test would be a good example to base it on.

TheBlueMatt force-pushed on Oct 20, 2017

achow101 commented at 4:11 am on October 25, 2017: member

I think this needs a 0.15.0.2 tag

laanwj added this to the milestone 0.15.0.2 on Oct 26, 2017

laanwj added the label Needs backport on Oct 26, 2017

laanwj added this to the "Review priority 0.15.0.2" column in a project

theuni commented at 7:33 am on October 27, 2017: member

utACK up until ffedf48b218b50b8187cbabf2969e519315c98a6, which has nuances that I’m not comfortable ACKing before taking some rabbit-hole dives.

Side-note: This is a really well-done patch set that is (other than the last commit which is the meat of it) very straightforward and easy to review. Though the changes are mostly one-liners, breaking them up functionally is a big help. Nice work :)

ryanofsky commented at 4:15 pm on October 27, 2017: member

You may want to rebase this to remove the commits from #11458 now that it is merged.

in src/validation.cpp:2555 in ffedf48b21 outdated

2541@@ -2533,17 +2542,14 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
2542 {
2543     AssertLockHeld(cs_main);

ryanofsky commented at 5:39 pm on October 27, 2017:

In commit “Reject headers building on invalid chains by tracking invalidity”

Maybe assert chainActive.Contains(pindex) here. Otherwise it isn’t clear that the while (invalid_walk_tip != pindex) loop below will work.

TheBlueMatt commented at 10:30 pm on October 27, 2017:

Nah, thats just a bug, fixing.

in src/validation.cpp:2563 in ffedf48b21 outdated

2559@@ -2554,6 +2560,19 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
2560         }
2561     }
2562 
2563+    while (invalid_walk_tip != pindex) {

ryanofsky commented at 5:45 pm on October 27, 2017:

In commit “Reject headers building on invalid chains by tracking invalidity”

Maybe add // Mark descendants of the block invalid comment for consistency and easier skimming

in src/validation.cpp:2558 in ffedf48b21 outdated

2541@@ -2533,17 +2542,14 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
2542 {
2543     AssertLockHeld(cs_main);
2544 
2545-    // Mark the block itself as invalid.
2546-    pindex->nStatus |= BLOCK_FAILED_VALID;
2547-    setDirtyBlockIndex.insert(pindex);
2548-    setBlockIndexCandidates.erase(pindex);
2549+    // We first disconnect backwards and then mark the blocks as invalid.
2550+    // This prevents a case where pruned nodes may fail to invalidateblock

ryanofsky commented at 5:53 pm on October 27, 2017:

In commit “Reject headers building on invalid chains by tracking invalidity”

Maybe move this comment above the setBlockIndexCandidates.erase() call so it is clearer what this is referring to.

TheBlueMatt commented at 10:31 pm on October 27, 2017:

Its actually not related to setBlockIndexCandidates (which is not persistent) - its related to the nStatus flags, I’ll clarify comment.

in src/validation.cpp:3115 in ffedf48b21 outdated

3092+                if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) {
3093+                    assert(failedit->nStatus & BLOCK_FAILED_VALID);
3094+                    CBlockIndex* invalid_walk = pindexPrev;
3095+                    while (invalid_walk != failedit) {
3096+                        invalid_walk->nStatus |= BLOCK_FAILED_CHILD;
3097+                        setDirtyBlockIndex.insert(invalid_walk);

ryanofsky commented at 5:59 pm on October 27, 2017:

In commit “Reject headers building on invalid chains by tracking invalidity”

Why isn’t setBlockIndexCandidates.erase() needed here?

TheBlueMatt commented at 10:35 pm on October 27, 2017:

None of those objects can be in our setBlockIndexCandidates - if they were we sould have tried to connect towards them, found them to be invalid, and then marked them as such (at least enough to get them out of our setBlockIndexCandidates).

TheBlueMatt force-pushed on Oct 27, 2017

TheBlueMatt commented at 6:00 pm on October 27, 2017: member

Rebased on master with no changes.

in src/validation.cpp:3548 in ffedf48b21 outdated

3526@@ -3492,6 +3527,10 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams)
3527                 pindex->nChainTx = pindex->nTx;
3528             }
3529         }
3530+        if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) {

ryanofsky commented at 6:04 pm on October 27, 2017:

In commit “Reject headers building on invalid chains by tracking invalidity”

Should the first condition be pindex->nStatus & BLOCK_FAILED_CHILD instead of FAILED_MASK?

Otherwise if it is intentional to avoid setting FAILED_CHILD here when FAILED_VALID is set, it might be good to have a comment, because the |= FAILED_CHILD code in AcceptBlockHeader and InvalidateBlock isn’t checking this.

TheBlueMatt commented at 10:37 pm on October 27, 2017:

It shouldn’t matter - really this is an indication of corruption - you can’t be both FAILED_CHILD and FAILED_VALID, you also cannot be FAILED_VALID if you have a FAILED_MASK parent. At the risk of overcomplicating things for a (currently) harmless error, I’ll skip adding a check, unless you object.

in src/validation.cpp:176 in ffedf48b21 outdated

155@@ -156,6 +156,14 @@ namespace {
156     /** chainwork for the last block that preciousblock has been applied to. */
157     arith_uint256 nLastPreciousChainwork = 0;
158 
159+    /** In order to effeciently track invalidity of headers, we keep the set of
160+      * blocks which we tried to connect and found to be invalid here. We can
161+      * then walk this set and check if a new header is a descendant of
162+      * something in this set, preventing us from having to walk mapBlockIndex
163+      * when we try to connect a bad block and fail.
164+      */

ryanofsky commented at 6:13 pm on October 27, 2017:

In commit “Reject headers building on invalid chains by tracking invalidity”

Would be good if the comment said what the relationship to the BLOCK_FAILED_VALID flag is. For example if all the FAILED_VALID blocks will be in this set, or only some of them.

sdaftuar commented at 7:17 pm on October 30, 2017:

Yeah I think this comment is pretty cryptic for anyone not aware of the two approaches we’ve discussed about how to detect that a header builds on an invalid chain: a) when you detect an invalid block, mark all descendants as BLOCK_FAILED_CHILD b) (what we implement in this pr)

Perhaps:

0/** In order to efficiently track invalidity of headers, we keep the set of 
1  * blocks which we tried to connect and found to be invalid here (ie which 
2  * were set to BLOCK_FAILED_VALID since the last restart).  We can then
3  * determine if a new header is extending an invalid chain by checking if 
4  * its parent is invalid, or if it's a descendant of an entry in g_failed_blocks.
5
6  * On startup, g_failed_blocks is empty, and all headers that are invalid 
7  * (whether directly or because a parent is invalid) are marked as such 
8  * in LoadBlockIndexDB().
9  */

in test/functional/p2p-acceptblock.py:10 in 594b7d9c6d outdated

15-this test.  Node2 will have nMinimumChainWork set to 0x10, so it won't process
16-low-work unrequested blocks.
17-
18-We have one NodeConn connection to each, test_node, white_node, and min_work_node,
19-respectively.
20+We have one NodeConn connection to test_node, and one to min_work_node.

ryanofsky commented at 6:27 pm on October 27, 2017:

In commit “Rewrite p2p-acceptblock in preparation for slight behavior changes”

This sounds reversed and doesn’t actually describe the connections. Maye write “We have one NodeConn connection to node0 called test_node, and one to node1 called min_work_node.”

in test/functional/p2p-acceptblock.py:13 in 594b7d9c6d outdated

19-respectively.
20+We have one NodeConn connection to test_node, and one to min_work_node.
21 
22 The test:
23-1. Generate one block on each node, to leave IBD.
24+1. Generate one block, to leave IBD.

ryanofsky commented at 6:29 pm on October 27, 2017:

In commit “Rewrite p2p-acceptblock in preparation for slight behavior changes”

Clearer before with “on each node”

in test/functional/p2p-acceptblock.py:163 in 594b7d9c6d outdated

202 
203-        # But this block should be accepted by node0 since it has more work.
204-        self.nodes[0].getblock(blocks_h3[0].hash)
205+        # But this block should be accepted by node since it has more work.
206+        self.nodes[0].getblock(block_h3.hash)
207         self.log.info("Unrequested more-work block accepted from non-whitelisted peer")

ryanofsky commented at 7:01 pm on October 27, 2017:

In commit “Rewrite p2p-acceptblock in preparation for slight behavior changes”

Maybe drop “from non-whitelisted peer”

in test/functional/p2p-acceptblock.py:29 in 594b7d9c6d outdated

48+4. Send another two blocks that builds on the forking block.
49+   Node0 should process the second block but be stuck on the shorter chain,
50+   because it's missing an intermediate block.
51 
52-4b.Send 288 more blocks on the longer chain.
53+4c.Send 288 more blocks on the longer chain.

ryanofsky commented at 7:17 pm on October 27, 2017:

In commit “Rewrite p2p-acceptblock in preparation for slight behavior changes”

Unclear what 288 refers to. Maybe define MIN_BLOCKS_TO_KEEP constant in python and use that in the test.

in test/functional/p2p-acceptblock.py:129 in 594b7d9c6d outdated

145         for x in self.nodes[0].getchaintips():
146-            if x['hash'] == blocks_h2f[0].hash:
147+            if x['hash'] == block_h1f.hash:
148                 assert_equal(x['status'], "headers-only")
149+                tip_entry_found = True
150+        assert(tip_entry_found)

ryanofsky commented at 7:49 pm on October 27, 2017:

In commit “Rewrite p2p-acceptblock in preparation for slight behavior changes”

Maybe check for “Block not found on disk” here to verify the block wasn’t processed.

in src/net_processing.cpp:2483 in d90b329a50 outdated

2483@@ -2484,7 +2484,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2484         // unless we're still syncing with the network.
2485         // Such an unrequested block may still be processed, subject to the
2486         // conditions in AcceptBlock().
2487-        bool forceProcessing = pfrom->fWhitelisted && !IsInitialBlockDownload();
2488+        bool forceProcessing = false;

ryanofsky commented at 8:13 pm on October 27, 2017:

In commit “Stop always storing blocks from whitelisted peers”

Is this change related to the “new headers are not a descendant of an invalid block” check or is it only part of this PR because it simplifies the p2p-acceptblock test? Seems like good cleanup regardless, but would like to understand.

TheBlueMatt commented at 10:25 pm on October 27, 2017:

The second. Mostly because it started as a “cleanup p2p-acceptblock test” pr and then I didn’t want to rewrite the p2p-acceptblock test again to not care about this.

meshcollider commented at 2:14 am on October 29, 2017:

Seems like the comment above needs modification/removing because of this?

in src/validation.cpp:3171 in 051ff2432d outdated

3114@@ -3115,7 +3115,7 @@ static bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CValidation
3115     // process an unrequested block if it's new and has enough work to
3116     // advance our tip, and isn't too many blocks ahead.
3117     bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA;
3118-    bool fHasMoreWork = (chainActive.Tip() ? pindex->nChainWork > chainActive.Tip()->nChainWork : true);
3119+    bool fHasMoreOrSameWork = (chainActive.Tip() ? pindex->nChainWork >= chainActive.Tip()->nChainWork : true);

ryanofsky commented at 8:18 pm on October 27, 2017:

In commit “Accept unrequested blocks with work equal to our tip”

Same question again: Is this change related to the “new headers are not a descendant of an invalid block” check or is it only part of this PR because it simplifies the p2p-acceptblock test? Again it seems fine, just asking to understand.

TheBlueMatt commented at 10:25 pm on October 27, 2017:

The second. Mostly because it started as a “cleanup p2p-acceptblock test” pr and then I didn’t want to rewrite the p2p-acceptblock test again to not care about this.

in test/functional/p2p-acceptblock.py:297 in 9df01d2f2c outdated

286+        test_node.send_message(msg_block(block_291))
287+
288+        # At this point we've sent an obviously-bogus block, wait for full processing
289+        # without assuming whether we will be disconnected or not
290+        try:
291+            test_node.sync_with_ping(timeout=1)

ryanofsky commented at 8:36 pm on October 27, 2017:

In commit “[qa] test that invalid blocks on an invalid chain get a disconnect”

Could you add a comment to explain why the timeout is required? Is this assuming that if disconnect happens, it will happen within 1 second? Why not just do a normal sync_with_ping with the default timeout?

in test/functional/p2p-acceptblock.py:298 in 9df01d2f2c outdated

287+
288+        # At this point we've sent an obviously-bogus block, wait for full processing
289+        # without assuming whether we will be disconnected or not
290+        try:
291+            test_node.sync_with_ping(timeout=1)
292+        except AssertionError:

ryanofsky commented at 8:43 pm on October 27, 2017:

In commit “[qa] test that invalid blocks on an invalid chain get a disconnect”

Could you add a comment explaining AssertionError? Is this catching the disconnect or the timeout or both?

TheBlueMatt commented at 10:46 pm on October 27, 2017:

Honestly, I have no idea…someone who knows more about the test framework should come along and make the catch here much more specific.

in test/functional/p2p-acceptblock.py:299 in 9df01d2f2c outdated

288+        # At this point we've sent an obviously-bogus block, wait for full processing
289+        # without assuming whether we will be disconnected or not
290+        try:
291+            test_node.sync_with_ping(timeout=1)
292+        except AssertionError:
293+            test_node.wait_for_disconnect()

ryanofsky commented at 8:47 pm on October 27, 2017:

In commit “[qa] test that invalid blocks on an invalid chain get a disconnect”

This seems to waiting 1 second for a ping response, and if that fails, then wait 60 seconds for disconnection before reconnecting? Does sync_with_ping not raise an error on disconnect?

in test/functional/p2p-acceptblock.py:293 in 9df01d2f2c outdated

284+        self.nodes[0].getblock(block_290f.hash)
285+
286+        test_node.send_message(msg_block(block_291))
287+
288+        # At this point we've sent an obviously-bogus block, wait for full processing
289+        # without assuming whether we will be disconnected or not

ryanofsky commented at 8:53 pm on October 27, 2017:

In commit “[qa] test that invalid blocks on an invalid chain get a disconnect”

Can you expand comment to say why it’s not desirable to make an assumption. Is there some kind of nondeterminism or complexity in knowing what the node is going to do?

TheBlueMatt commented at 10:40 pm on October 27, 2017:

Standard testing practice - we’ve sent something for which a reasonable node may very well decide to DoS us, disconnect us or whathave you, dont test that the node not do a thing that its perfectly reasonable for it to do. I’d say thats implied here, but happy to clarify more (or put something in some dev-notes doc).

ryanofsky commented at 5:46 pm on October 30, 2017:

In commit “test that invalid blocks on an invalid chain get a disconnect”

Test seems like it will hang and fail in the case where a node takes more than one second to the respond to the ping, but does not disconnect because sync_with_ping and wait_for_disconnect will both timeout.

ryanofsky commented at 8:59 pm on October 27, 2017: member

utACK 9df01d2f2cb9eed5af395424e702fa5d7e441207. I left a lot of comments, but they are all minor, so feel free to ignore and just respond to whatever you think is useful.

TheBlueMatt commented at 10:47 pm on October 27, 2017: member

Addressed (or commented on) @ryanofsky’s feedback.

in src/validation.cpp:159 in 9429cefe31 outdated

155@@ -156,6 +156,15 @@ namespace {
156     /** chainwork for the last block that preciousblock has been applied to. */
157     arith_uint256 nLastPreciousChainwork = 0;
158 
159+    /** In order to effeciently track invalidity of headers, we keep the set of

meshcollider commented at 2:14 am on October 29, 2017:

Nit: typo effeciently -> efficiently

meshcollider commented at 2:27 am on October 29, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/pull/11531/commits/9429cefe3182d10cd8f99209feaae0a21465e5e0 Only briefly looked at tests, couple of very minor nits inlline

TheBlueMatt commented at 5:45 pm on October 29, 2017: member

Addressed @MeshCollider’s code comment issues.

achow101 commented at 9:50 pm on October 29, 2017: member

utACK 35e7b4e2bb7f749a4c8426dc3ea71b77f3457f30

meshcollider commented at 9:52 pm on October 29, 2017: contributor

re-utACK https://github.com/bitcoin/bitcoin/commit/35e7b4e2bb7f749a4c8426dc3ea71b77f3457f30

laanwj commented at 4:54 pm on October 30, 2017: member

utACK 35e7b4e (I guess the rationale behind f commits is that they should be squashed before merge?)

TheBlueMatt force-pushed on Oct 30, 2017

TheBlueMatt commented at 5:11 pm on October 30, 2017: member

Squashed.

TheBlueMatt force-pushed on Oct 30, 2017

in test/functional/p2p-acceptblock.py:280 in 9a5013c0ec outdated

277+        tip_entry_found = False
278+        for x in self.nodes[0].getchaintips():
279+            if x['hash'] == block_292.hash:
280+                assert_equal(x['status'], "headers-only")
281+                tip_entry_found = True
282+        assert(tip_entry_found)

ryanofsky commented at 5:52 pm on October 30, 2017:

In commit “[qa] test that invalid blocks on an invalid chain get a disconnect”

This chunk of code is repeated several times in the test. I think you could simplify it as:

0status = {block['hash']: block['status'] for block in self.nodes[0].getchaintips()}
1assert_equal(status[block_292.hash], "headers-only")

ryanofsky commented at 5:54 pm on October 30, 2017: member

utACK eae05599f4f577667441530109a338000a48bdd2. Has updated comments, new block not found checks in the test, and the pindex_was_in_chain fix.

in src/validation.cpp:2544 in 9a5013c0ec outdated

2557+    CBlockIndex *invalid_walk_tip = chainActive.Tip();
2558 
2559     DisconnectedBlockTransactions disconnectpool;
2560     while (chainActive.Contains(pindex)) {
2561-        CBlockIndex *pindexWalk = chainActive.Tip();
2562-        pindexWalk->nStatus |= BLOCK_FAILED_CHILD;

sipa commented at 6:02 pm on October 30, 2017:

Moving this has the disadvantage that if Bitcoin Core is shut down while invalidating, even after part of it is was flushed already, the invalidating will be reverted at startup.

TheBlueMatt commented at 6:34 pm on October 30, 2017:

i’d say thats better than, at least in the common-pruning-case, failing to start at all and requiring re-sync by assert(false)ing each time you try to start up?

sdaftuar commented at 1:06 am on October 31, 2017: member

p2p-acceptblock.py is broken, it’s apparently an extended test and not run by travis:

 0$ test/functional/test_runner.py p2p-acceptblock
 1Temporary test directory at /var/folders/pw/k25spdv971g8xhccyx5t3yyr0000gn/T/bitcoin_test_runner_20171030_210539
 2..
 3p2p-acceptblock.py failed, Duration: 1 s
 4
 5stdout:
 62017-10-31 01:05:39.236000 TestFramework (INFO): Initializing test directory /var/folders/pw/k25spdv971g8xhccyx5t3yyr0000gn/T/bitcoin_test_runner_20171030_210539/p2p-acceptblock_333
 72017-10-31 01:05:39.760000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:13664
 82017-10-31 01:05:39.761000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:13665
 92017-10-31 01:05:39.967000 TestFramework (INFO): First height 2 block accepted by node0; correctly rejected by node1
102017-10-31 01:05:40.080000 TestFramework (ERROR): Assertion failed
11Traceback (most recent call last):
12  File "/Users/sdaftuar/projects/ccl-bitcoin/test-11531/test/functional/test_framework/test_framework.py", line 120, in main
13    self.run_test()
14  File "/Users/sdaftuar/projects/ccl-bitcoin/test-11531/test/functional/p2p-acceptblock.py", line 147, in run_test
15    assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, block_h2f.hash)
16  File "/Users/sdaftuar/projects/ccl-bitcoin/test-11531/test/functional/test_framework/util.py", line 104, in assert_raises_rpc_error
17    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
18AssertionError: No exception raised
192017-10-31 01:05:40.085000 TestFramework (INFO): Stopping nodes
202017-10-31 01:05:40.295000 TestFramework (WARNING): Not cleaning up dir /var/folders/pw/k25spdv971g8xhccyx5t3yyr0000gn/T/bitcoin_test_runner_20171030_210539/p2p-acceptblock_333
212017-10-31 01:05:40.295000 TestFramework (ERROR): Test failed. Test logging available at /var/folders/pw/k25spdv971g8xhccyx5t3yyr0000gn/T/bitcoin_test_runner_20171030_210539/p2p-acceptblock_333/test_framework.log
22
23
24stderr:
25
26
27
28TEST               | STATUS    | DURATION
29
30p2p-acceptblock.py | ✖ Failed  | 1 s
31
32ALL                | ✖ Failed  | 1 s (accumulated) 
33Runtime: 1 s

TheBlueMatt commented at 1:31 am on October 31, 2017: member

Gah, Russ scared me into adding too many checks. I moved p2p-acceptblock into regular tests so that travis runs it and fixed the test.

in test/functional/p2p-acceptblock.py:147 in b29100958b outdated

143@@ -144,7 +144,7 @@ def run_test(self):
144                 assert_equal(x['status'], "headers-only")
145                 tip_entry_found = True
146         assert(tip_entry_found)
147-        assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, block_h2f.hash)
148+        self.nodes[0].getblock(block_h2f.hash)

sdaftuar commented at 1:48 am on October 31, 2017:

If you’re going to squash this in to Rewrite p2p-acceptblock in preparation for slight behavior changes, then you’ll need to have this line commented out (and re-uncomment it in Accept unrequested blocks with work equal to our tip), in order to make the test succeed after that first commit, I think.

TheBlueMatt commented at 5:51 pm on October 31, 2017:

Oh ffs, I’m just gonna remove that line.

in test/functional/p2p-acceptblock.py:30 in bb2c66d18c outdated

48+4. Send another two blocks that builds on the forking block.
49+   Node0 should process the second block but be stuck on the shorter chain,
50+   because it's missing an intermediate block.
51 
52-4b.Send 288 more blocks on the longer chain.
53+4c.Send 288 more blocks on the longer chain (the number of blocks ahead

sdaftuar commented at 1:58 am on October 31, 2017:

4c?

TheBlueMatt commented at 5:51 pm on October 31, 2017:

Yea, 4 was split into 4 a and b, I updated the comment to note that.

sdaftuar commented at 2:03 am on October 31, 2017: member

Few nits.

These changes seem conceptually fine, though I would have preferred that this PR had a narrower scope since we’re targeting this for 0.15.0.2, rather than master. The changes to the AcceptBlock unrequested block logic seem independent of the improvements to invalid block handling (and even the behavior improvement in InvalidateBlock seems not directly related, though maybe I’m missing something with that?).

But no objection to including this whole PR in 0.15 if others are fine with these changes; aside from my nits I think this is fine.

sdaftuar commented at 12:20 pm on October 31, 2017: member

I think it’s worth mentioning why I think his change, which adds a way to ban all peers giving us a block header that builds on an invalid previous block, is okay, when we have been discussing changing our ban-behavior to exclude inbound peers from these checks, and to generally favor disconnects over ban+disconnect even for outbound peers.

While I’d like to see the ban-behavior vastly improved, in this case we’re starting from a baseline of already banning a peer for relaying a block header that builds on an invalid block (regardless of inbound/outbound). This PR addresses the inconsistency in behavior between a situation where you get one header and block at a time (and discover that one such block is invalid), versus if you download a bunch of headers, and then process the first block in the chain and discover it is invalid.

So given that there’s already a situation where you’d ban all your peers that relay a block header building on an invalid block, I think it’s okay to extend that to the case where you receive descendant headers before you process the first block. I think it would also be okay to improve this logic so that we only apply it to (eg) outbound peers, but I don’t think that should be a merge blocker, and I think this is a valuable enough improvement that it’s worth shipping as-is for 0.15.0.2, as it addresses a shortcoming of our headers processing and disconnect logic that should be buttoned up.

ryanofsky commented at 5:28 pm on October 31, 2017: member

utACK b29100958b37fc7d499e9932e38ae88e3596fa1e. Only changes since last review are getblock test fixes and moving the test from extended -> base

Rewrite p2p-acceptblock in preparation for slight behavior changes

Removes checking whitelisted behavior (which will be removed, the
difference in behavior here makes little sense) and no longer
requires that blocks at the same work as our tip be dropped if not
requested (in part because we *do* request those blocks).

3b4ac43bc3

Stop always storing blocks from whitelisted peers

There is no reason to wish to store blocks on disk always just
because a peer is whitelisted. This appears to be a historical
quirk to avoid breaking things when the accept limits were added.

3d9c70ca0f

Accept unrequested blocks with work equal to our tip

This is a simple change that makes our accept requirements the
same as our request requirements, (ever so slightly) further
decoupling our consensus logic from our FindNextBlocksToDownload
logic in net_processing.

932f118e6a

Reject headers building on invalid chains by tracking invalidity

This tracks the set of all known invalid-themselves blocks (ie
blocks which we attempted to connect but which were found to be
invalid). This is used to cheaply check if new headers build on an
invalid chain.

While we're at it we also resolve an edge-case in invalidateblock
on pruned nodes which results in them needing a reindex if they
fail to reorg.

015a5258ad

[qa] test that invalid blocks on an invalid chain get a disconnect 00dcda60f6

Make p2p-acceptablock not an extended test f3d4adfa6f

TheBlueMatt force-pushed on Oct 31, 2017

TheBlueMatt commented at 5:52 pm on October 31, 2017: member

Addressed @sdaftuar’s suggestions and squashed. Indeed, I dont think the way things are with header disconnection here, and as mentioned already have a branch working towards redoing it rather significantly, but that certainly isn’t gonna make 0.15.0.2.

laanwj commented at 1:41 pm on November 1, 2017: member

utACK: only changes between b291009 and f3d4adf:

 0diff -dur bitcoin-b29100958b37fc7d499e9932e38ae88e3596fa1e/src/validation.cpp bitcoin-f3d4adfa6ff5db180ec09d93f78cdc8bfda26f96/src/validation.cpp
 1--- bitcoin-b29100958b37fc7d499e9932e38ae88e3596fa1e/src/validation.cpp	2017-10-31 02:30:32.000000000 +0100
 2+++ bitcoin-f3d4adfa6ff5db180ec09d93f78cdc8bfda26f96/src/validation.cpp	2017-10-31 18:51:34.000000000 +0100
 3@@ -162,6 +162,17 @@
 4       * walk this set and check if a new header is a descendant of something in
 5       * this set, preventing us from having to walk mapBlockIndex when we try
 6       * to connect a bad block and fail.
 7+      *
 8+      * While this is more complicated than marking everything which descends
 9+      * from an invalid block as invalid at the time we discover it to be
10+      * invalid, doing so would require walking all of mapBlockIndex to find all
11+      * descendants. Since this case should be very rare, keeping track of all
12+      * BLOCK_FAILED_VALID blocks in a set should be just fine and work just as
13+      * well.
14+      *
15+      * Because we alreardy walk mapBlockIndex in height-order at startup, we go
16+      * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time,
17+      * instead of putting things in this set.
18       */
19     std::set<CBlockIndex*> g_failed_blocks;
20 
21diff -dur bitcoin-b29100958b37fc7d499e9932e38ae88e3596fa1e/test/functional/p2p-acceptblock.py bitcoin-f3d4adfa6ff5db180ec09d93f78cdc8bfda26f96/test/functional/p2p-acceptblock.py
22--- bitcoin-b29100958b37fc7d499e9932e38ae88e3596fa1e/test/functional/p2p-acceptblock.py	2017-10-31 02:30:32.000000000 +0100
23+++ bitcoin-f3d4adfa6ff5db180ec09d93f78cdc8bfda26f96/test/functional/p2p-acceptblock.py	2017-10-31 18:51:34.000000000 +0100
24@@ -23,7 +23,7 @@
25    Node0 should not process this block (just accept the header), because it
26    is unrequested and doesn't have more or equal work to the tip.
27 
28-4. Send another two blocks that builds on the forking block.
29+4a,b. Send another two blocks that build on the forking block.
30    Node0 should process the second block but be stuck on the shorter chain,
31    because it's missing an intermediate block.
32 
33@@ -144,7 +144,6 @@
34                 assert_equal(x['status'], "headers-only")
35                 tip_entry_found = True
36         assert(tip_entry_found)
37-        self.nodes[0].getblock(block_h2f.hash)
38 
39         # But this block should be accepted by node since it has equal work.
40         self.nodes[0].getblock(block_h2f.hash)

laanwj merged this on Nov 1, 2017

laanwj closed this on Nov 1, 2017

laanwj referenced this in commit cffa5ee132 on Nov 1, 2017

fanquake removed this from the "Review priority 0.15.0.2" column in a project

sipa commented at 7:14 pm on November 1, 2017: member

posthumous utACK f3d4adfa6ff5db180ec09d93f78cdc8bfda26f96; didn’t review the test changes

MarcoFalke removed the label Needs backport on Nov 1, 2017

MarcoFalke referenced this in commit 05910682e0 on Nov 1, 2017

MarcoFalke referenced this in commit 26688d9a9a on Nov 1, 2017

MarcoFalke referenced this in commit 0269b32fab on Nov 1, 2017

MarcoFalke referenced this in commit 255d7c807b on Nov 1, 2017

MarcoFalke referenced this in commit 986c93b072 on Nov 1, 2017

MarcoFalke referenced this in commit 238b4a28c3 on Nov 1, 2017

MarcoFalke referenced this in commit e976c36ddf on Nov 2, 2017

MarcoFalke referenced this in commit c6e4d0ce82 on Nov 2, 2017

MarcoFalke referenced this in commit 51001d684b on Nov 2, 2017

MarcoFalke referenced this in commit 92d6105c4e on Nov 2, 2017

MarcoFalke referenced this in commit 5bec7744d1 on Nov 2, 2017

MarcoFalke referenced this in commit 55b7abfa8a on Nov 2, 2017

bambache referenced this in commit 0ed5d9acdb on Jan 31, 2019

codablock referenced this in commit b246b58720 on Sep 26, 2019

codablock referenced this in commit 7d21a78fa1 on Sep 29, 2019

barrystyle referenced this in commit 95b0dcef19 on Jan 22, 2020

pgerzani referenced this in commit 8cfac7e47c on Mar 4, 2020

DrahtBot locked this on Sep 8, 2021

Check that new headers are not a descendant of an invalid block (more effeciently) #11531