Page MenuHomePhabricator

[Part 1] Core PR 11531: p2p-acceptblock improvements
ClosedPublic

Authored by jasonbcox on Thu, Nov 29, 19:12.

Details

Summary

This change includes test refactor and improvements from Core PR 11531:
3b4ac43 is nearly the entire diff, minus a few modifications to match
existing behavior. These parts of the test can be reverted to match
the backport once those behavior changes have been fully reviewed in
separate diffs.

Partial backport of Core PR 11531
Progress on T456

Co-authored-by: Jason B. Cox <contact@jasonbcox.com>

The only changes from the original Core commit are as follows:

diff --git a/test/functional/p2p-acceptblock.py b/test/functional/p2p-acceptblock.py
index c87e24532..9fd40b7ff 100755
--- a/test/functional/p2p-acceptblock.py
+++ b/test/functional/p2p-acceptblock.py
@@ -159,8 +159,13 @@ class AcceptBlockTest(BitcoinTestFramework):
         assert(tip_entry_found)

         # But this block should be accepted by node since it has equal work.
-        self.nodes[0].getblock(block_h2f.hash)
-        self.log.info("Second height 2 block accepted, but not reorg'ed to")
+        # FIXME: Replace the assert with the commented lines once Core backport
+        # 932f118 is completed. Current behavior does not accept equal work
+        # blocks ontop of unprocessed blocks.
+        #self.nodes[0].getblock(block_h2f.hash)
+        #self.log.info("Second height 2 block accepted, but not reorg'ed to")
+        assert_raises_rpc_error(-1, "Block not found on disk",
+                                self.nodes[0].getblock, block_h2f.hash)

         # 4b. Now send another block that builds on the forking chain.
         block_h3 = create_block(
@@ -262,6 +267,12 @@ class AcceptBlockTest(BitcoinTestFramework):
         # 7. Send the missing block for the third time (now it is requested)
         test_node.send_message(msg_block(block_h1f))

+        # 7.b. Send the next missing block now that it's requested.
+        # FIXME: Remove this line once Core backport 932f118 is completed.
+        # Current behavior is to not process equal work blocks building ontop
+        # of unprocessed blocks.
+        test_node.send_message(msg_block(block_h2f))
+
         test_node.sync_with_ping()
         assert_equal(self.nodes[0].getblockcount(), 290)
         self.nodes[0].getblock(all_blocks[286].hash)
@@ -347,7 +358,9 @@ class AcceptBlockTest(BitcoinTestFramework):
         headers_message = msg_headers()
         headers_message.headers.append(CBlockHeader(block_293))
         test_node.send_message(headers_message)
-        test_node.wait_for_disconnect()
+        # FIXME: Uncomment this line once Core backport 015a525 is completed.
+        # Current behavior does not ban peers that give us headers on invalid chains.
+        #test_node.wait_for_disconnect()

         # 9. Connect node1 to node0 and ensure it is able to sync
         connect_nodes(self.nodes[0], 1)
Test Plan

test_runner.py p2p-acceptblock

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Thu, Nov 29, 19:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Nov 29, 19:12
jasonbcox edited the summary of this revision. (Show Details)Thu, Nov 29, 19:16
schancel accepted this revision.Thu, Nov 29, 22:32
schancel added inline comments.
test/functional/p2p-acceptblock.py
129 ↗(On Diff #6174)

int(self.nodes[0].getblockhash(0), 16)
??

155 ↗(On Diff #6174)

This seems to be a recurring pattern in the tests, we should make a ticket to add is as a helper.

This revision is now accepted and ready to land.Thu, Nov 29, 22:32
schancel retitled this revision from Backport 1 of many for Core PR 11531: p2p-acceptblock improvements to [Part 1] Core PR 11531: p2p-acceptblock improvements.Thu, Nov 29, 22:33
This revision now requires review to proceed.Mon, Dec 3, 23:26
Fabien requested changes to this revision.Tue, Dec 4, 21:46
Fabien added inline comments.
test/functional/p2p-acceptblock.py
97 ↗(On Diff #6174)

Move comment on top

238 ↗(On Diff #6174)

dito

340 ↗(On Diff #6174)

dito

345 ↗(On Diff #6174)

dito

This revision now requires changes to proceed.Tue, Dec 4, 21:46
jasonbcox requested review of this revision.Mon, Dec 10, 23:25

In the interest of reducing merge conflicts, the nits regarding hash formatting and comment location will need to be handled after this series of backports.

deadalnix accepted this revision.Tue, Dec 11, 01:14

Accepting because doing all the nits + the backport would become very tedious. I trust you'll follow through.

Task to followup on the nits: T501

Fabien accepted this revision.Tue, Dec 11, 08:02
This revision is now accepted and ready to land.Tue, Dec 11, 08:02
Closed by commit rABCe55fb9e61eee: [Part 1] Core PR 11531: p2p-acceptblock improvements (authored by Matt Corallo <git@bluematt.me>, committed by jasonbcox). · Explain WhyWed, Dec 12, 00:00
This revision was automatically updated to reflect the committed changes.