From f432ceff3118b35fb7fa4b1ccc991896de0a66dc Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 14 Apr 2020 13:37:40 -0600 Subject: [PATCH 01/16] Add a test reproducing the off-by-one error. --- qa/pull-tester/rpc-tests.sh | 1 + qa/rpc-tests/sapling_rewind_check.py | 89 +++++++++++++++++++++++++ qa/rpc-tests/test_framework/mininode.py | 8 +-- qa/rpc-tests/test_framework/util.py | 9 +++ 4 files changed, 102 insertions(+), 5 deletions(-) create mode 100755 qa/rpc-tests/sapling_rewind_check.py diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index 6fb3c0f3c..99ed127f1 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -83,6 +83,7 @@ testScripts=( 'turnstile.py' 'mining_shielded_coinbase.py' 'framework.py' + 'sapling_rewind_check.py' ); testScriptsExt=( 'getblocktemplate_longpoll.py' diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py new file mode 100755 index 000000000..ae1467bf6 --- /dev/null +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -0,0 +1,89 @@ +#!/usr/bin/env python3 +# Copyright (c) 2014 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + +""" +Exercise the chain rewind code at the Blossom boundary. + +Test case is: +4 nodes. They are initialized and the chain is advanced to just +prior to Blossom activation; then, the network is split and + + + +""" + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.authproxy import JSONRPCException +from test_framework.util import assert_equal, initialize_chain_clean, \ + start_nodes, start_node, connect_nodes_bi, \ + bitcoind_processes, \ + nuparams, OVERWINTER_BRANCH_ID, SAPLING_BRANCH_ID + +import os +import shutil +from random import randint +from decimal import Decimal +import logging + +HAS_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 15)] +NO_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 100)] + +class SaplingRewindTest(BitcoinTestFramework): + def setup_chain(self): + logging.info("Initializing test directory "+self.options.tmpdir) + initialize_chain_clean(self.options.tmpdir, 4) + + # This mirrors how the network was setup in the bash test + def setup_network(self, split=False): + logging.info("Initializing the network in "+self.options.tmpdir) + self.nodes = start_nodes(3, self.options.tmpdir, extra_args=[ + HAS_SAPLING, # The first two nodes have a correct view of the network, + HAS_SAPLING, # the third will rewind after upgrading. + NO_SAPLING, + ]) + connect_nodes_bi(self.nodes,0,1) + connect_nodes_bi(self.nodes,1,2) + connect_nodes_bi(self.nodes,0,2) + self.is_network_split=False + self.sync_all() + + def run_test(self): + # Generate shared state up to the network split + logging.info("Generating initial blocks.") + self.nodes[0].generate(13) + block14 = self.nodes[0].generate(1)[0] + logging.info("Syncing network after initial generation...") + self.sync_all() # Everyone is still on overwinter + + logging.info("Checking overwinter block propagation.") + assert_equal(self.nodes[0].getbestblockhash(), block14) + assert_equal(self.nodes[1].getbestblockhash(), block14) + assert_equal(self.nodes[2].getbestblockhash(), block14) + logging.info("All nodes are on overwinter.") + + # Generate a network split longer than the maximum rewind length (99) + logging.info("Generating network split...") + self.is_network_split=True # split the network + self.nodes[0].generate(50) # generate into sapling + self.nodes[2].generate(100) # generate more on sprout + self.sync_all() + + # Stop the overwinter node to ensure state is flushed to disk. + logging.info("Shutting down lagging node...") + self.nodes[2].stop() + bitcoind_processes[2].wait() + + # Restart the nodes, reconnect, and sync the network + logging.info("Reconnecting the network...") + self.nodes[2] = start_node(2, self.options.tmpdir, extra_args=HAS_SAPLING) + connect_nodes_bi(self.nodes,1,2) + connect_nodes_bi(self.nodes,0,2) + + self.is_network_split=False # reconnect the network + self.sync_all() + logging.info("Network synced.") + +if __name__ == '__main__': + SaplingRewindTest().main() diff --git a/qa/rpc-tests/test_framework/mininode.py b/qa/rpc-tests/test_framework/mininode.py index 1e86e8bc6..4e95a38a0 100755 --- a/qa/rpc-tests/test_framework/mininode.py +++ b/qa/rpc-tests/test_framework/mininode.py @@ -31,6 +31,9 @@ from threading import Thread import logging import copy from pyblake2 import blake2b +from test_framework.util import \ + SPROUT_BRANCH_ID, OVERWINTER_BRANCH_ID, \ + SAPLING_BRANCH_ID, BLOSSOM_BRANCH_ID, HEARTWOOD_BRANCH_ID from .equihash import ( gbp_basic, @@ -53,11 +56,6 @@ OVERWINTER_VERSION_GROUP_ID = 0x03C48270 SAPLING_VERSION_GROUP_ID = 0x892F2085 # No transaction format change in Blossom. -SPROUT_BRANCH_ID = 0x00000000 -OVERWINTER_BRANCH_ID = 0x5BA81B19 -SAPLING_BRANCH_ID = 0x76B809BB -BLOSSOM_BRANCH_ID = 0x2BB40E60 - MAX_INV_SZ = 50000 diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index ffeb48808..ef702a915 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -30,6 +30,11 @@ COVERAGE_DIR = None PRE_BLOSSOM_BLOCK_TARGET_SPACING = 150 POST_BLOSSOM_BLOCK_TARGET_SPACING = 75 +SPROUT_BRANCH_ID = 0x00000000 +OVERWINTER_BRANCH_ID = 0x5BA81B19 +SAPLING_BRANCH_ID = 0x76B809BB +BLOSSOM_BRANCH_ID = 0x2BB40E60 +HEARTWOOD_BRANCH_ID = 0xF5B9230B def enable_coverage(dirname): """Maintain a log of which RPC calls are made during testing.""" @@ -591,3 +596,7 @@ def check_node_log(self, node_number, line_to_check, stop_node = True): if line_to_check in logline: return n raise AssertionError(repr(line_to_check) + " not found") + +def nuparams(branch_id, height): + return '-nuparams=%s:%d' %(hex(branch_id)[2:], height) + From a4e8945cfa7af8bb2ce361961a19d1b670c224ed Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 14 Apr 2020 13:52:50 -0600 Subject: [PATCH 02/16] Check network reunification. --- qa/rpc-tests/sapling_rewind_check.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index ae1467bf6..10b972a93 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -16,8 +16,8 @@ prior to Blossom activation; then, the network is split and from test_framework.test_framework import BitcoinTestFramework from test_framework.authproxy import JSONRPCException -from test_framework.util import assert_equal, initialize_chain_clean, \ - start_nodes, start_node, connect_nodes_bi, \ +from test_framework.util import assert_equal, assert_true, \ + initialize_chain_clean, start_nodes, start_node, connect_nodes_bi, \ bitcoind_processes, \ nuparams, OVERWINTER_BRANCH_ID, SAPLING_BRANCH_ID @@ -67,9 +67,13 @@ class SaplingRewindTest(BitcoinTestFramework): logging.info("Generating network split...") self.is_network_split=True # split the network self.nodes[0].generate(50) # generate into sapling + expected = self.nodes[0].getbestblockhash() + self.nodes[2].generate(100) # generate more on sprout self.sync_all() + assert_true(expected != self.nodes[2].getbestblockhash(), "Split chains have not diverged!") + # Stop the overwinter node to ensure state is flushed to disk. logging.info("Shutting down lagging node...") self.nodes[2].stop() @@ -85,5 +89,8 @@ class SaplingRewindTest(BitcoinTestFramework): self.sync_all() logging.info("Network synced.") + assert_equal(self.nodes[1].getbestblockhash(), expected) + assert_equal(self.nodes[2].getbestblockhash(), expected) + if __name__ == '__main__': SaplingRewindTest().main() From 59d2a6458d8fbfaf21cf3c63da054bacff3034dd Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 14 Apr 2020 17:47:29 -0600 Subject: [PATCH 03/16] Narrow down the test case. --- qa/rpc-tests/rewind_index.py | 7 ++++--- qa/rpc-tests/sapling_rewind_check.py | 9 +++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/qa/rpc-tests/rewind_index.py b/qa/rpc-tests/rewind_index.py index 54fbc876f..27d55891f 100755 --- a/qa/rpc-tests/rewind_index.py +++ b/qa/rpc-tests/rewind_index.py @@ -5,12 +5,13 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, initialize_chain_clean, \ - start_nodes, start_node, connect_nodes_bi, bitcoind_processes + start_nodes, start_node, connect_nodes_bi, bitcoind_processes, \ + nuparams, OVERWINTER_BRANCH_ID, SAPLING_BRANCH_ID import time -FAKE_SPROUT = ['-nuparams=5ba81b19:210', '-nuparams=76b809bb:220'] -FAKE_OVERWINTER = ['-nuparams=5ba81b19:10', '-nuparams=76b809bb:220'] +FAKE_SPROUT = [nuparams(OVERWINTER_BRANCH_ID, 210), nuparams(SAPLING_BRANCH_ID, 220)] +FAKE_OVERWINTER = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 220)] class RewindBlockIndexTest (BitcoinTestFramework): diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index 10b972a93..e0041f9a9 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -28,7 +28,7 @@ from decimal import Decimal import logging HAS_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 15)] -NO_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 100)] +NO_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 80)] class SaplingRewindTest(BitcoinTestFramework): def setup_chain(self): @@ -69,7 +69,8 @@ class SaplingRewindTest(BitcoinTestFramework): self.nodes[0].generate(50) # generate into sapling expected = self.nodes[0].getbestblockhash() - self.nodes[2].generate(100) # generate more on sprout + # generate blocks into sapling; if this is set to 60, the test passes. + self.nodes[2].generate(80) self.sync_all() assert_true(expected != self.nodes[2].getbestblockhash(), "Split chains have not diverged!") @@ -79,9 +80,9 @@ class SaplingRewindTest(BitcoinTestFramework): self.nodes[2].stop() bitcoind_processes[2].wait() - # Restart the nodes, reconnect, and sync the network + # Restart the nodes, reconnect, and sync the network. This succeeds if "-reindex" is passed. logging.info("Reconnecting the network...") - self.nodes[2] = start_node(2, self.options.tmpdir, extra_args=HAS_SAPLING) + self.nodes[2] = start_node(2, self.options.tmpdir, extra_args=HAS_SAPLING) # + ["-reindex"]) connect_nodes_bi(self.nodes,1,2) connect_nodes_bi(self.nodes,0,2) From 5030e73afc53fa52bda5b3126fd289a6817c6ee9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 15 Apr 2020 10:44:24 -0600 Subject: [PATCH 04/16] Make the test reproduce the actual off-by-one error in rewind length. --- qa/rpc-tests/sapling_rewind_check.py | 47 +++++++++++++++++++++------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index e0041f9a9..b879c9871 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -22,13 +22,14 @@ from test_framework.util import assert_equal, assert_true, \ nuparams, OVERWINTER_BRANCH_ID, SAPLING_BRANCH_ID import os +import re import shutil from random import randint from decimal import Decimal import logging HAS_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 15)] -NO_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 80)] +NO_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 150)] class SaplingRewindTest(BitcoinTestFramework): def setup_chain(self): @@ -41,7 +42,7 @@ class SaplingRewindTest(BitcoinTestFramework): self.nodes = start_nodes(3, self.options.tmpdir, extra_args=[ HAS_SAPLING, # The first two nodes have a correct view of the network, HAS_SAPLING, # the third will rewind after upgrading. - NO_SAPLING, + NO_SAPLING ]) connect_nodes_bi(self.nodes,0,1) connect_nodes_bi(self.nodes,1,2) @@ -70,7 +71,7 @@ class SaplingRewindTest(BitcoinTestFramework): expected = self.nodes[0].getbestblockhash() # generate blocks into sapling; if this is set to 60, the test passes. - self.nodes[2].generate(80) + self.nodes[2].generate(120) self.sync_all() assert_true(expected != self.nodes[2].getbestblockhash(), "Split chains have not diverged!") @@ -82,16 +83,40 @@ class SaplingRewindTest(BitcoinTestFramework): # Restart the nodes, reconnect, and sync the network. This succeeds if "-reindex" is passed. logging.info("Reconnecting the network...") - self.nodes[2] = start_node(2, self.options.tmpdir, extra_args=HAS_SAPLING) # + ["-reindex"]) - connect_nodes_bi(self.nodes,1,2) - connect_nodes_bi(self.nodes,0,2) + try: + self.nodes[2] = start_node(2, self.options.tmpdir, extra_args=HAS_SAPLING) # + ["-reindex"]) + except: + logpath = self.options.tmpdir + "/node2/regtest/debug.log" + found = False + with open(logpath, 'r') as f: + for line in f: + m = re.search(r'roll back ([0-9]+)', line) + if m is not None: + print(m.group(1)) - self.is_network_split=False # reconnect the network - self.sync_all() - logging.info("Network synced.") + if m is None: + continue + elif m.group(1) == "120": + found = True + break + else: + raise AssertionError("Incorrect rollback length %s found, expected 120." %(m.group(1))) + + if not found: + raise AssertionError("Expected rollback message not found in log file.") + + else: + raise AssertionError("Expected node to halt due to excessive rewind length.") + #connect_nodes_bi(self.nodes,1,2) + #connect_nodes_bi(self.nodes,0,2) + + #self.is_network_split=False # reconnect the network + #self.sync_all() + #logging.info("Network synced.") + + #assert_equal(self.nodes[1].getbestblockhash(), expected) + #assert_equal(self.nodes[2].getbestblockhash(), expected) - assert_equal(self.nodes[1].getbestblockhash(), expected) - assert_equal(self.nodes[2].getbestblockhash(), expected) if __name__ == '__main__': SaplingRewindTest().main() From 40b5d5e3ea4b602c34c4efaba0b9f6171dddfef5 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 15 Apr 2020 14:18:05 -0600 Subject: [PATCH 05/16] Fix #4119. --- src/main.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 50e650c55..7c5d01194 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4664,20 +4664,23 @@ bool RewindBlockIndex(const CChainParams& chainparams, bool& clearWitnessCaches) *pindex->nCachedBranchId == CurrentEpochBranchId(pindex->nHeight, consensus); }; - int nHeight = 1; - while (nHeight <= chainActive.Height()) { - if (!sufficientlyValidated(chainActive[nHeight])) { + int lastValidHeight = 0; + while (lastValidHeight < chainActive.Height()) { + if (!sufficientlyValidated(chainActive[lastValidHeight + 1])) { break; + } else { + lastValidHeight++; } - nHeight++; } - // nHeight is now the height of the first insufficiently-validated block, or tipheight + 1 - auto rewindLength = chainActive.Height() - nHeight; + // lastValidHeight is now the height of the last valid block below the active chain height + auto rewindLength = chainActive.Height() - lastValidHeight; clearWitnessCaches = false; if (rewindLength > 0) { - LogPrintf("*** First insufficiently validated block at height %d, rewind length %d\n", nHeight, rewindLength); + LogPrintf("*** Last validated block at height %d, active height is %d; rewind length %d\n", lastValidHeight, chainActive.Height(), rewindLength); + + auto nHeight = lastValidHeight + 1; const uint256 *phashFirstInsufValidated = chainActive[nHeight]->phashBlock; auto networkID = chainparams.NetworkIDString(); @@ -4689,7 +4692,6 @@ bool RewindBlockIndex(const CChainParams& chainparams, bool& clearWitnessCaches) uint256S("002e1d6daf4ab7b296e7df839dc1bee9d615583bb4bc34b1926ce78307532852")); clearWitnessCaches = (rewindLength > MAX_REORG_LENGTH && intendedRewind); - if (clearWitnessCaches) { auto msg = strprintf(_( "An intended block chain rewind has been detected: network %s, hash %s, height %d" @@ -4699,7 +4701,7 @@ bool RewindBlockIndex(const CChainParams& chainparams, bool& clearWitnessCaches) if (rewindLength > MAX_REORG_LENGTH && !intendedRewind) { auto pindexOldTip = chainActive.Tip(); - auto pindexRewind = chainActive[nHeight - 1]; + auto pindexRewind = chainActive[lastValidHeight]; auto msg = strprintf(_( "A block chain rewind has been detected that would roll back %d blocks! " "This is larger than the maximum of %d blocks, and so the node is shutting down for your safety." @@ -4719,7 +4721,7 @@ bool RewindBlockIndex(const CChainParams& chainparams, bool& clearWitnessCaches) CValidationState state; CBlockIndex* pindex = chainActive.Tip(); - while (chainActive.Height() >= nHeight) { + while (chainActive.Height() > lastValidHeight) { if (fPruneMode && !(chainActive.Tip()->nStatus & BLOCK_HAVE_DATA)) { // If pruning, don't try rewinding past the HAVE_DATA point; // since older blocks can't be served anyway, there's From 9fc94cc3729b2fc321d57e3ea7eef3d638d26bc9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 15 Apr 2020 14:33:41 -0600 Subject: [PATCH 06/16] The last valid height condition reads better flipped. --- src/main.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 7c5d01194..0b47790ac 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4666,10 +4666,10 @@ bool RewindBlockIndex(const CChainParams& chainparams, bool& clearWitnessCaches) int lastValidHeight = 0; while (lastValidHeight < chainActive.Height()) { - if (!sufficientlyValidated(chainActive[lastValidHeight + 1])) { - break; - } else { + if (sufficientlyValidated(chainActive[lastValidHeight + 1])) { lastValidHeight++; + } else { + break; } } From a4e80ae1abb30667fcc837bff950dc524c41ae15 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 15 Apr 2020 15:58:48 -0600 Subject: [PATCH 07/16] Restart node in a chain split state to allow the test to complete. --- qa/rpc-tests/sapling_rewind_check.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index b879c9871..d58d33418 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -91,9 +91,6 @@ class SaplingRewindTest(BitcoinTestFramework): with open(logpath, 'r') as f: for line in f: m = re.search(r'roll back ([0-9]+)', line) - if m is not None: - print(m.group(1)) - if m is None: continue elif m.group(1) == "120": @@ -105,18 +102,12 @@ class SaplingRewindTest(BitcoinTestFramework): if not found: raise AssertionError("Expected rollback message not found in log file.") + # restart the node with -reindex to allow the test to complete gracefully, + # otherwise the node shutdown call in test cleanup will throw an error since + # it can't connect + self.nodes[2] = start_node(2, self.options.tmpdir, extra_args=NO_SAPLING + ["-reindex"]) else: raise AssertionError("Expected node to halt due to excessive rewind length.") - #connect_nodes_bi(self.nodes,1,2) - #connect_nodes_bi(self.nodes,0,2) - - #self.is_network_split=False # reconnect the network - #self.sync_all() - #logging.info("Network synced.") - - #assert_equal(self.nodes[1].getbestblockhash(), expected) - #assert_equal(self.nodes[2].getbestblockhash(), expected) - if __name__ == '__main__': SaplingRewindTest().main() From 7629560a26c4458b8d1f24b622e42ee2ef35abd1 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 15 Apr 2020 16:09:19 -0600 Subject: [PATCH 08/16] Trivial comment. --- qa/rpc-tests/sapling_rewind_check.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index d58d33418..4ddf931b6 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -90,6 +90,8 @@ class SaplingRewindTest(BitcoinTestFramework): found = False with open(logpath, 'r') as f: for line in f: + # Search for the rollback message in the debug log, and ensure that it has the + # correct expected rollback length. m = re.search(r'roll back ([0-9]+)', line) if m is None: continue From c2e3454e0ad2e808132d15115aa3934461ff477a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 2 May 2020 18:30:09 -0600 Subject: [PATCH 09/16] Clean up imports in sapling_rewind_check.py Co-authored-by: Daira Hopwood --- qa/rpc-tests/sapling_rewind_check.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index 4ddf931b6..4484fffe2 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -16,10 +16,10 @@ prior to Blossom activation; then, the network is split and from test_framework.test_framework import BitcoinTestFramework from test_framework.authproxy import JSONRPCException -from test_framework.util import assert_equal, assert_true, \ - initialize_chain_clean, start_nodes, start_node, connect_nodes_bi, \ - bitcoind_processes, \ - nuparams, OVERWINTER_BRANCH_ID, SAPLING_BRANCH_ID +from test_framework.util import (assert_equal, assert_true, + initialize_chain_clean, start_nodes, start_node, connect_nodes_bi, + bitcoind_processes, + nuparams, OVERWINTER_BRANCH_ID, SAPLING_BRANCH_ID) import os import re From 2de5a2cb2de5325bbf4a769666dc823a47c4d5ac Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 2 May 2020 18:31:13 -0600 Subject: [PATCH 10/16] Use `%x` formatter for branch id hex string in test_framework/util.py Co-authored-by: Daira Hopwood --- qa/rpc-tests/test_framework/util.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index 52a9f68e3..6d4ae3292 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -599,5 +599,4 @@ def check_node_log(self, node_number, line_to_check, stop_node = True): raise AssertionError(repr(line_to_check) + " not found") def nuparams(branch_id, height): - return '-nuparams=%s:%d' %(hex(branch_id)[2:], height) - + return '-nuparams=%x:%d' % (branch_id, height) From e7eff424c191f60c33bd445157f395f778d97d33 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 2 May 2020 18:31:26 -0600 Subject: [PATCH 11/16] Update qa/rpc-tests/test_framework/mininode.py Co-authored-by: Daira Hopwood --- qa/rpc-tests/test_framework/mininode.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/qa/rpc-tests/test_framework/mininode.py b/qa/rpc-tests/test_framework/mininode.py index 10dce18cb..16935b1e5 100755 --- a/qa/rpc-tests/test_framework/mininode.py +++ b/qa/rpc-tests/test_framework/mininode.py @@ -31,10 +31,9 @@ from threading import Thread import logging import copy from pyblake2 import blake2b -from test_framework.util import \ - SPROUT_BRANCH_ID, OVERWINTER_BRANCH_ID, \ - SAPLING_BRANCH_ID, BLOSSOM_BRANCH_ID, HEARTWOOD_BRANCH_ID, \ - NU4_BRANCH_ID +from test_framework.util import (SPROUT_BRANCH_ID, OVERWINTER_BRANCH_ID, + SAPLING_BRANCH_ID, BLOSSOM_BRANCH_ID, HEARTWOOD_BRANCH_ID, + NU4_BRANCH_ID) from .equihash import ( gbp_basic, From e09f05257098c7e08cea3961241a11db74e2ae55 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 2 May 2020 18:33:01 -0600 Subject: [PATCH 12/16] Update qa/rpc-tests/sapling_rewind_check.py Co-authored-by: Daira Hopwood --- qa/rpc-tests/sapling_rewind_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index 4484fffe2..4ef82ed2b 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -66,7 +66,7 @@ class SaplingRewindTest(BitcoinTestFramework): # Generate a network split longer than the maximum rewind length (99) logging.info("Generating network split...") - self.is_network_split=True # split the network + self.split_network() self.nodes[0].generate(50) # generate into sapling expected = self.nodes[0].getbestblockhash() From 6742ba021fac9924387b4f04807ef615a7c88c59 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 2 May 2020 18:33:58 -0600 Subject: [PATCH 13/16] Add Zcash copyright to sapling_rewind_check.py --- qa/rpc-tests/sapling_rewind_check.py | 1 + 1 file changed, 1 insertion(+) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index 4ef82ed2b..e75cf1b61 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -1,5 +1,6 @@ #!/usr/bin/env python3 # Copyright (c) 2014 The Bitcoin Core developers +# Copyright (c) 2020 The Zcash developers # Distributed under the MIT software license, see the accompanying # file COPYING or https://www.opensource.org/licenses/mit-license.php . From ae86386648f8e51ab801bb5a08c46aab04b42c3a Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 4 May 2020 10:55:32 -0600 Subject: [PATCH 14/16] Update test description and clarify internal comments. --- qa/rpc-tests/sapling_rewind_check.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index e75cf1b61..cd86a0945 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -8,10 +8,21 @@ Exercise the chain rewind code at the Blossom boundary. Test case is: -4 nodes. They are initialized and the chain is advanced to just -prior to Blossom activation; then, the network is split and +3 nodes are initialized, two of which are aware of the Blossom network upgrade, +and one of which is not. On each node, the chain is advanced to just prior to +the Blossom activation; height then, the network is split and each branch of +the network produces blocks into the range of the upgraded protocol. +The node that is not aware of Blossom activation is advanced beyond the maximum +reorg length of 99 blocks, then that node is shut down. When the node is +restarted with knowledge of the network activation height the checks on startup +identify a need to reorg to come into agreement with the rest of the network. +However, since the rollback required is greater than the maximum reorg length, +the node shuts down with an error as a precautionary measure. It was noticed in +#4119 that the error message indicated an incorrect computation of the rollback +length. This test reproduces that error, and the associated change in rollback +length computation (40b5d5e3ea4b602c34c4efaba0b9f6171dddfef5) corrects the issue. """ @@ -65,13 +76,14 @@ class SaplingRewindTest(BitcoinTestFramework): assert_equal(self.nodes[2].getbestblockhash(), block14) logging.info("All nodes are on overwinter.") - # Generate a network split longer than the maximum rewind length (99) logging.info("Generating network split...") self.split_network() - self.nodes[0].generate(50) # generate into sapling + + # generate past the boundary into sapling; this will become the "canonical" branch + self.nodes[0].generate(50) expected = self.nodes[0].getbestblockhash() - # generate blocks into sapling; if this is set to 60, the test passes. + # generate blocks into sapling beyond the maximum rewind length (99 blocks) self.nodes[2].generate(120) self.sync_all() @@ -85,6 +97,8 @@ class SaplingRewindTest(BitcoinTestFramework): # Restart the nodes, reconnect, and sync the network. This succeeds if "-reindex" is passed. logging.info("Reconnecting the network...") try: + # expect an exception; the node will refuse to fully start because its last point of + # agreement with the rest of the network was prior to the network upgrade activation self.nodes[2] = start_node(2, self.options.tmpdir, extra_args=HAS_SAPLING) # + ["-reindex"]) except: logpath = self.options.tmpdir + "/node2/regtest/debug.log" From 45dab9adc04eda9dbc40a397a1d2013a690121e8 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 4 May 2020 16:57:48 -0600 Subject: [PATCH 15/16] Revert "Update qa/rpc-tests/sapling_rewind_check.py" This reverts commit e09f05257098c7e08cea3961241a11db74e2ae55. --- qa/rpc-tests/sapling_rewind_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index cd86a0945..a88a559bc 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -77,7 +77,7 @@ class SaplingRewindTest(BitcoinTestFramework): logging.info("All nodes are on overwinter.") logging.info("Generating network split...") - self.split_network() + self.is_network_split=True # generate past the boundary into sapling; this will become the "canonical" branch self.nodes[0].generate(50) From 35ff8d9bf2b95ccee0c28f2e14312f704f39988c Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 5 May 2020 09:11:31 -0600 Subject: [PATCH 16/16] Remove unused imports. --- qa/rpc-tests/feature_zip221.py | 3 ++- qa/rpc-tests/sapling_rewind_check.py | 5 ----- qa/rpc-tests/test_framework/mininode.py | 3 --- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/qa/rpc-tests/feature_zip221.py b/qa/rpc-tests/feature_zip221.py index e9fec9db9..bfe0f656b 100755 --- a/qa/rpc-tests/feature_zip221.py +++ b/qa/rpc-tests/feature_zip221.py @@ -5,9 +5,10 @@ from test_framework.flyclient import (ZcashMMRNode, append, delete, make_root_commitment) -from test_framework.mininode import (HEARTWOOD_BRANCH_ID, CBlockHeader) +from test_framework.mininode import (CBlockHeader) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( + HEARTWOOD_BRANCH_ID, assert_equal, bytes_to_hex_str, hex_str_to_bytes, diff --git a/qa/rpc-tests/sapling_rewind_check.py b/qa/rpc-tests/sapling_rewind_check.py index a88a559bc..206a28541 100755 --- a/qa/rpc-tests/sapling_rewind_check.py +++ b/qa/rpc-tests/sapling_rewind_check.py @@ -27,17 +27,12 @@ length computation (40b5d5e3ea4b602c34c4efaba0b9f6171dddfef5) corrects the issue """ from test_framework.test_framework import BitcoinTestFramework -from test_framework.authproxy import JSONRPCException from test_framework.util import (assert_equal, assert_true, initialize_chain_clean, start_nodes, start_node, connect_nodes_bi, bitcoind_processes, nuparams, OVERWINTER_BRANCH_ID, SAPLING_BRANCH_ID) -import os import re -import shutil -from random import randint -from decimal import Decimal import logging HAS_SAPLING = [nuparams(OVERWINTER_BRANCH_ID, 10), nuparams(SAPLING_BRANCH_ID, 15)] diff --git a/qa/rpc-tests/test_framework/mininode.py b/qa/rpc-tests/test_framework/mininode.py index 16935b1e5..908b0dde0 100755 --- a/qa/rpc-tests/test_framework/mininode.py +++ b/qa/rpc-tests/test_framework/mininode.py @@ -31,9 +31,6 @@ from threading import Thread import logging import copy from pyblake2 import blake2b -from test_framework.util import (SPROUT_BRANCH_ID, OVERWINTER_BRANCH_ID, - SAPLING_BRANCH_ID, BLOSSOM_BRANCH_ID, HEARTWOOD_BRANCH_ID, - NU4_BRANCH_ID) from .equihash import ( gbp_basic,