Page MenuHomePhabricator

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

Authored by jasonbcox on Nov 29 2018, 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
Branch
pr11531-2
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixtest/functional/p2p-acceptblock.py:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 4174
Build 6414: Bitcoin ABC Buildbot (legacy)
Build 6413: arc lint + arc unit

Event Timeline

schancel added inline comments.
test/functional/p2p-acceptblock.py
129

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

155

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.Nov 29 2018, 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.Nov 29 2018, 22:33
This revision now requires review to proceed.Dec 3 2018, 23:26
Fabien requested changes to this revision.Dec 4 2018, 21:46
Fabien added inline comments.
test/functional/p2p-acceptblock.py
97

Move comment on top

238

dito

340

dito

345

dito

This revision now requires changes to proceed.Dec 4 2018, 21:46

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.

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

This revision is now accepted and ready to land.Dec 11 2018, 08:02
This revision was automatically updated to reflect the committed changes.