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

TheBlueMatt · 2017-10-19T21:15:38Z

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

sdaftuar · 2017-10-20T01:52:32Z

src/validation.cpp

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


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.

Sjors · 2017-10-20T09:03:14Z

src/validation.cpp

+                    setDirtyBlockIndex.insert(invalid_walk);
+                    invalid_walk = invalid_walk->pprev;
+                }
+                return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");


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.

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.

achow101 · 2017-10-25T04:11:23Z

I think this needs a 0.15.0.2 tag

theuni · 2017-10-27T07:33:41Z

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

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

TheBlueMatt · 2017-10-27T18:00:54Z

Rebased on master with no changes.

ryanofsky

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.

ryanofsky · 2017-10-27T17:39:36Z

src/validation.cpp

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


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.

Nah, thats just a bug, fixing.

ryanofsky · 2017-10-27T17:45:10Z

src/validation.cpp

@@ -2554,6 +2560,19 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
        }
    }

+    while (invalid_walk_tip != pindex) {


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

ryanofsky · 2017-10-27T17:53:45Z

src/validation.cpp

-    setDirtyBlockIndex.insert(pindex);
-    setBlockIndexCandidates.erase(pindex);
+    // We first disconnect backwards and then mark the blocks as invalid.
+    // This prevents a case where pruned nodes may fail to invalidateblock


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.

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

ryanofsky · 2017-10-27T17:59:13Z

src/validation.cpp

+                    CBlockIndex* invalid_walk = pindexPrev;
+                    while (invalid_walk != failedit) {
+                        invalid_walk->nStatus |= BLOCK_FAILED_CHILD;
+                        setDirtyBlockIndex.insert(invalid_walk);


In commit "Reject headers building on invalid chains by tracking invalidity"

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

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

ryanofsky · 2017-10-27T18:04:10Z

src/validation.cpp

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


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.

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.

ryanofsky · 2017-10-27T20:18:49Z

src/validation.cpp

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


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.

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.

ryanofsky · 2017-10-27T20:36:11Z

test/functional/p2p-acceptblock.py

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


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?

ryanofsky · 2017-10-27T20:43:20Z

test/functional/p2p-acceptblock.py

+        # without assuming whether we will be disconnected or not
+        try:
+            test_node.sync_with_ping(timeout=1)
+        except AssertionError:


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?

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

ryanofsky · 2017-10-27T20:47:31Z

test/functional/p2p-acceptblock.py

+        try:
+            test_node.sync_with_ping(timeout=1)
+        except AssertionError:
+            test_node.wait_for_disconnect()


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?

ryanofsky · 2017-10-27T20:53:16Z

test/functional/p2p-acceptblock.py

+        test_node.send_message(msg_block(block_291))
+
+        # At this point we've sent an obviously-bogus block, wait for full processing
+        # without assuming whether we will be disconnected or not


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?

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

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.

TheBlueMatt · 2017-10-27T22:47:38Z

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

meshcollider

utACK 9429cef
Only briefly looked at tests, couple of very minor nits inlline

meshcollider · 2017-10-29T02:14:09Z

src/net_processing.cpp

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


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

meshcollider · 2017-10-29T02:14:50Z

src/validation.cpp

@@ -156,6 +156,15 @@ namespace {
    /** chainwork for the last block that preciousblock has been applied to. */
    arith_uint256 nLastPreciousChainwork = 0;

+    /** In order to effeciently track invalidity of headers, we keep the set of


Nit: typo effeciently -> efficiently

TheBlueMatt · 2017-10-29T17:45:46Z

Addressed @meshcollider's code comment issues.

achow101 · 2017-10-29T21:50:06Z

utACK 35e7b4e2bb7f749a4c8426dc3ea71b77f3457f30

meshcollider · 2017-10-29T21:52:22Z

re-utACK 35e7b4e

laanwj · 2017-10-30T16:54:58Z

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

TheBlueMatt · 2017-10-30T17:11:22Z

Squashed.

ryanofsky

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

ryanofsky · 2017-10-30T17:52:19Z

test/functional/p2p-acceptblock.py

+            if x['hash'] == block_292.hash:
+                assert_equal(x['status'], "headers-only")
+                tip_entry_found = True
+        assert(tip_entry_found)


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:

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

sipa · 2017-10-30T18:02:35Z

src/validation.cpp


    DisconnectedBlockTransactions disconnectpool;
    while (chainActive.Contains(pindex)) {
-        CBlockIndex *pindexWalk = chainActive.Tip();
-        pindexWalk->nStatus |= BLOCK_FAILED_CHILD;


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.

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 · 2017-10-31T01:06:22Z

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

$ test/functional/test_runner.py p2p-acceptblock
Temporary test directory at /var/folders/pw/k25spdv971g8xhccyx5t3yyr0000gn/T/bitcoin_test_runner_20171030_210539
..
p2p-acceptblock.py failed, Duration: 1 s

stdout:
2017-10-31 01:05:39.236000 TestFramework (INFO): Initializing test directory /var/folders/pw/k25spdv971g8xhccyx5t3yyr0000gn/T/bitcoin_test_runner_20171030_210539/p2p-acceptblock_333
2017-10-31 01:05:39.760000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:13664
2017-10-31 01:05:39.761000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:13665
2017-10-31 01:05:39.967000 TestFramework (INFO): First height 2 block accepted by node0; correctly rejected by node1
2017-10-31 01:05:40.080000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/sdaftuar/projects/ccl-bitcoin/test-11531/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/Users/sdaftuar/projects/ccl-bitcoin/test-11531/test/functional/p2p-acceptblock.py", line 147, in run_test
    assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, block_h2f.hash)
  File "/Users/sdaftuar/projects/ccl-bitcoin/test-11531/test/functional/test_framework/util.py", line 104, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
AssertionError: No exception raised
2017-10-31 01:05:40.085000 TestFramework (INFO): Stopping nodes
2017-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
2017-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


stderr:



TEST               | STATUS    | DURATION

p2p-acceptblock.py | ✖ Failed  | 1 s

ALL                | ✖ Failed  | 1 s (accumulated) 
Runtime: 1 s

TheBlueMatt · 2017-10-31T01:31:23Z

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.

sdaftuar

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 · 2017-10-30T19:17:00Z

src/validation.cpp

+      * then walk this set and check if a new header is a descendant of
+      * something in this set, preventing us from having to walk mapBlockIndex
+      * when we try to connect a bad block and fail.
+      */


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:

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

sdaftuar · 2017-10-31T01:48:39Z

test/functional/p2p-acceptblock.py

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


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.

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

sdaftuar · 2017-10-31T01:58:19Z

test/functional/p2p-acceptblock.py


-4b.Send 288 more blocks on the longer chain.
+4c.Send 288 more blocks on the longer chain (the number of blocks ahead


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

sdaftuar · 2017-10-31T12:20:36Z

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.

TheBlueMatt · 2017-10-31T17:52:34Z

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 · 2017-11-01T13:41:51Z

utACK: only changes between b291009 and f3d4adf:

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

@sdaftuar

…id block (more effeciently) f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo) 00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo) 015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo) 932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo) 3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo) 3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo) Pull request description: @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. Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1

sipa · 2017-11-01T19:14:38Z

posthumous utACK f3d4adf; didn't review the test 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). Github-Pull: bitcoin#11531 Rebased-From: 3b4ac43

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. Github-Pull: bitcoin#11531 Rebased-From: 3d9c70c

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. Github-Pull: bitcoin#11531 Rebased-From: 932f118

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. Github-Pull: bitcoin#11531 Rebased-From: 015a525

Github-Pull: bitcoin#11531 Rebased-From: 00dcda6

Github-Pull: bitcoin#11531 Rebased-From: f3d4adf

…id block (more effeciently) bitcoin/bitcoin#11531

@sdaftuar

…n invalid block (more effeciently) f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo) 00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo) 015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo) 932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo) 3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo) 3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo) Pull request description: @sdaftuar pointed out that the version in bitcoin#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 bitcoin#11458. Includes tests from bitcoin#11487. Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1

@sdaftuar

…n invalid block (more effeciently) f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo) 00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo) 015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo) 932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo) 3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo) 3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo) Pull request description: @sdaftuar pointed out that the version in bitcoin#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 bitcoin#11458. Includes tests from bitcoin#11487. Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1

@sdaftuar

…n invalid block (more effeciently) f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo) 00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo) 015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo) 932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo) 3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo) 3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo) Pull request description: @sdaftuar pointed out that the version in bitcoin#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 bitcoin#11458. Includes tests from bitcoin#11487. Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1

…id block (more effeciently) (#218) bitcoin/bitcoin#11531

This was referenced Oct 19, 2017

Check that new headers are not a descendant of an invalid block #11487

Closed

Don't process unrequested, low-work blocks #11458

Merged

TheBlueMatt force-pushed the 2017-10-cache-invalid-indexes branch from 552ad25 to 9bbd66a Compare October 19, 2017 23:13

fanquake added the P2P label Oct 19, 2017

sdaftuar reviewed Oct 20, 2017

View reviewed changes

Sjors reviewed Oct 20, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-10-cache-invalid-indexes branch from 9bbd66a to ba5dd28 Compare October 20, 2017 19:35

laanwj added this to the 0.15.0.2 milestone Oct 26, 2017

laanwj added the Needs backport label Oct 26, 2017

ryanofsky reviewed Oct 27, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-10-cache-invalid-indexes branch from ba5dd28 to 9df01d2 Compare October 27, 2017 18:00

ryanofsky reviewed Oct 27, 2017

View reviewed changes

meshcollider reviewed Oct 29, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-10-cache-invalid-indexes branch from 35e7b4e to eae0559 Compare October 30, 2017 17:11

TheBlueMatt force-pushed the 2017-10-cache-invalid-indexes branch from eae0559 to 9a5013c Compare October 30, 2017 17:31

ryanofsky reviewed Oct 30, 2017

View reviewed changes

sipa reviewed Oct 30, 2017

View reviewed changes

sdaftuar reviewed Oct 31, 2017

View reviewed changes

Make p2p-acceptablock not an extended test

f3d4adf

TheBlueMatt force-pushed the 2017-10-cache-invalid-indexes branch from b291009 to f3d4adf Compare October 31, 2017 17:51

laanwj merged commit f3d4adf into bitcoin:master Nov 1, 2017

maflcko removed the Needs backport label Nov 1, 2017

morcos mentioned this pull request Nov 2, 2017

0.15: Backports #11592

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017

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

5bec774

Github-Pull: bitcoin#11531 Rebased-From: 00dcda6

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017

Make p2p-acceptablock not an extended test

55b7abf

Github-Pull: bitcoin#11531 Rebased-From: f3d4adf

Sjors mentioned this pull request Aug 9, 2018

doc: Better explain GetAncestor check for m_failed_blocks in AcceptBlockHeader #13930

Merged

bambache pushed a commit to projectpai/paicoin that referenced this pull request Jan 31, 2019

Merge #11531: Check that new headers are not a descendant of an inval…

0ed5d9a

…id block (more effeciently) bitcoin/bitcoin#11531

bambache mentioned this pull request Jan 31, 2019

Feature/apply bitcoin merge11531 projectpai/paicoin#218

Merged

pgerzani pushed a commit to projectpai/paicoin that referenced this pull request Mar 4, 2020

Merge #11531: Check that new headers are not a descendant of an inval…

8cfac7e

…id block (more effeciently) (#218) bitcoin/bitcoin#11531

UdjinM6 mentioned this pull request Apr 7, 2020

Backports 0.16 pr16 dashpay/dash#3395

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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


		4b.Send 288 more blocks on the longer chain.
		4c.Send 288 more blocks on the longer chain (the number of blocks ahead

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

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

Uh oh!

Conversation

TheBlueMatt commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 25, 2017

Uh oh!

theuni commented Oct 27, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 27, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 27, 2017

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 29, 2017

Uh oh!

achow101 commented Oct 29, 2017

Uh oh!

meshcollider commented Oct 29, 2017

Uh oh!

laanwj commented Oct 30, 2017

TheBlueMatt commented Oct 19, 2017 •

edited

Loading

meshcollider left a comment •

edited

Loading