From 3ffd8d5ddb47324b5d8f41aac605d656d885aa8e Mon Sep 17 00:00:00 2001 From: Harry <110472914+Cosmos-Harry@users.noreply.github.com> Date: Mon, 22 Jun 2026 16:39:41 -0400 Subject: [PATCH] qa: fix and re-enable wallet_z_shieldcoinbase tests Both z_shieldcoinbase tests were disabled pending fixes. Re-enable them. wallet_z_shieldcoinbase.py: - Activate all later NUs (NU5/NU6/NU6.1/NU6.2) at height 1 so zebra's mined coinbase commits to the consensus branch ID zallet expects. NU5-only made zallet reject the first block with "coinbase tx's claimed height doesn't match its consensus branch ID". - sync_all() before the first account-mutating RPC: z_getnewaccount returns "Wallet sync required" until the wallet has committed a block. - Fix the coinbase-maturity off-by-one (a block at height H has tip - H + 1 confirmations, so there are tip - COINBASE_MATURITY + 1 mature coinbases). wallet_z_shieldcoinbase_multi_taddr.py: - Same activation-height and wallet-sync fixes (the wallet rejected z_getaddressforaccount with "chain height is unknown"). - Redesign the mid-test miner switch. The old design assumed MAX_BLOCK_REORG_HEIGHT = 99 and that a restart drops the non-finalized tail; the real value is 1000, and zebra now restores non-finalized state from a periodic backup, so the truncate-wallet + zcash/wallet#447 SQL workaround no longer holds (history recovery exited fetching transactions zebra no longer had). Keep the pre-restart chain exactly one block long, wait for zebra to back up block 1, then restart with miner=B and only EXTEND the chain: no truncate, no orphaned wallet records, no #447 workaround. Both tests pass locally against zebrad 5.2.0 and zallet 0.1.0-alpha.3. Co-Authored-By: Claude Opus 4.8 --- qa/pull-tester/rpc-tests.py | 2 - qa/rpc-tests/wallet_z_shieldcoinbase.py | 34 +++- .../wallet_z_shieldcoinbase_multi_taddr.py | 158 +++++++++--------- 3 files changed, 101 insertions(+), 93 deletions(-) diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 85ef58ec9..931c61a9b 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -145,8 +145,6 @@ 'wallet_treestate.py', # deprecated; z_getnewaddress->z_getaddressforaccount, z_getbalance->z_getbalances 'wallet_unified_change.py', # needs z_shieldcoinbase funding step: zallet z_sendmany cannot spend mined coinbase (have 0) 'wallet_z_sendmany.py', # no zallet equiv yet: z_exportviewingkey - 'wallet_z_shieldcoinbase.py', # zallet startup crashes with "coinbase tx's claimed height doesn't match its consensus branch ID" when NU5 is activated at height 1; consensus-branch alignment between zebra and zallet needed - 'wallet_z_shieldcoinbase_multi_taddr.py', # JSON-RPC error during z_getaddressforaccount(["sapling","orchard"]); likely shielded-pool activation height issue 'wallet_zero_value.py', # deprecated; getnewaddress->z_getaddressforaccount, signrawtransaction->PCZT (wallet#99) 'addnode.py', # zebra regtest peering stalls in multi-peer topologies (zebra #10329, #10332) 'wallet_zip317_default.py', # deprecated; getnewaddress->z_getaddressforaccount, z_getnewaddress->z_getaddressforaccount diff --git a/qa/rpc-tests/wallet_z_shieldcoinbase.py b/qa/rpc-tests/wallet_z_shieldcoinbase.py index e40dbf71d..be1785005 100755 --- a/qa/rpc-tests/wallet_z_shieldcoinbase.py +++ b/qa/rpc-tests/wallet_z_shieldcoinbase.py @@ -51,8 +51,13 @@ def mature_transparent_utxos(wallet): - """Return the wallet's mature transparent coinbase UTXOs.""" - utxos = wallet.z_listunspent(COINBASE_MATURITY + 1) + """Return the wallet's mature transparent coinbase UTXOs. + + minconf=COINBASE_MATURITY matches the proposal (coinbase is spendable at + exactly 100 confirmations); minconf+1 misses a boundary UTXO the sweep + selects. + """ + utxos = wallet.z_listunspent(COINBASE_MATURITY) return [u for u in utxos if u.get('pool') == 'transparent'] @@ -151,13 +156,15 @@ def __init__(self): self.cache_behavior = 'clean' def setup_nodes(self): - # NU5 is the consensus floor for Zallet's Orchard change strategy - # used inside z_shieldcoinbase. The default zallet.toml activates - # NU5 at height 1; mirror that on the zebrad side. + # All later NUs must also be listed at height 1; otherwise zebra mines + # a coinbase committing to NU5's consensus branch ID while zallet's + # network params expect the latest NU's branch ID, and zallet rejects + # the coinbase on the first block sync. See ZebraArgs default in + # test_framework/config.py. args = [ ZebraArgs( miner_address=addr, - activation_heights={"NU5": 1}, + activation_heights={"NU5": 1, "NU6": 1, "NU6.1": 1, "NU6.2": 1}, ) for addr in self.miner_addresses ] return start_nodes(self.num_nodes, self.options.tmpdir, args) @@ -328,7 +335,10 @@ def _first_transparent_receiver(wallet, ua): def run_functional_tests(self, node, w0, w0_taddr, w0_account_uuid, w0_zaddr, w0_extra_zaddr): # Expected unspent-mature COUNT before each sweep: all coinbase goes - # to w0_taddr, so it's (tip - COINBASE_MATURITY) minus what we've spent. + # to w0_taddr, so it's (tip - COINBASE_MATURITY + 1) minus what we've spent + # — a block at height H has (tip - H + 1) confirmations, so a block is + # mature when tip - H + 1 >= 100, i.e. H <= tip - 99, giving + # (tip - 99) mature coinbases = (tip - COINBASE_MATURITY + 1). # Counts are reliable; VALUES are not derived from the snapshot: # z_listunspent and the proposal use different maturity tips, so across # the regtest subsidy halving (6.25 -> 3.125) a summed snapshot can @@ -337,7 +347,7 @@ def run_functional_tests(self, node, w0, w0_taddr, w0_account_uuid, mature_spent = 0 def expected_unspent_mature(): - return node.getblockcount() - COINBASE_MATURITY - mature_spent + return node.getblockcount() - COINBASE_MATURITY + 1 - mature_spent def confirm_and_check_balance(txid, pre_private, shielding_value): """Confirm the sweep and assert balance grew by value - fee. @@ -587,6 +597,12 @@ def run_test(self): w0_taddr = self.miner_addresses[0] + # Wait for the wallet to sync to the node tip. z_getnewaccount (and + # other account-mutating RPCs) reject with "Wallet sync required" + # until the wallet has committed at least one block, since they need + # a chain height to anchor the new account's birthday. + self.sync_all() + # Identify account 0 on wallet 0. accounts = w0.z_listaccounts() assert_true(len(accounts) >= 1, "Wallet 0 should have at least one account") @@ -620,7 +636,7 @@ def run_test(self): # re-checks this exact count via its `expected_unspent_mature` # bookkeeping. node.generate(COINBASE_MATURITY + 20) - expected_initial_mature = node.getblockcount() - COINBASE_MATURITY + expected_initial_mature = node.getblockcount() - COINBASE_MATURITY + 1 wait_for_mature_coinbase_count(w0, expected_initial_mature) print(" Mature coinbase UTXOs: {}".format(expected_initial_mature)) print(" Account 0 UUID: {}".format(w0_account_uuid)) diff --git a/qa/rpc-tests/wallet_z_shieldcoinbase_multi_taddr.py b/qa/rpc-tests/wallet_z_shieldcoinbase_multi_taddr.py index 7c635fcd4..b8310ec7d 100755 --- a/qa/rpc-tests/wallet_z_shieldcoinbase_multi_taddr.py +++ b/qa/rpc-tests/wallet_z_shieldcoinbase_multi_taddr.py @@ -16,33 +16,28 @@ # coinbase address, and instead only generates blocks with the miner address # specified in the zebrad config. So, producing coinbase on two transparent # receivers in the same account requires switching zebrad's -# `mining.miner_address` mid-test. A naive stop+restart leaves the wallet's -# SQLite block records pointing at the pre-restart non-finalized tail (lost -# when zebrad drops its in-memory state on shutdown, per -# `MAX_BLOCK_REORG_HEIGHT = 99`). Re-mining the same heights with a different -# miner address produces different block hashes, and the wallet then upserts -# those heights with conflicting data and errors out with -# `SqliteClientError::BlockConflict`. +# `mining.miner_address` mid-test, which means a stop+restart. # -# To avoid that, the test: -# 1. Mines enough blocks at miner_address_A to push block 1 (the -# one zallet-startup block) past the finalization horizon — so -# A's coinbase output at block 1 survives zebrad restart. -# 2. Stops zallet and zebrad. -# 3. Truncates the wallet's block-scan state via -# `zallet repair truncate-wallet` to discard the wallet's stale -# view of the non-finalized tail that zebrad will drop. -# 4. Restarts zebrad with `mining.miner_address = B`, then restarts -# zallet. The wallet re-scans the chain from a fresh slate; no -# records conflict. -# 5. Mines coinbase to B until both A's and B's coinbase outputs -# are mature. -# 6. Sweeps via the UUID form and asserts that the sweep covered -# coinbase across BOTH receivers. +# The wallet's SQLite block records must not end up referencing a chain state +# that zebrad no longer has after the restart, or the wallet upserts conflicting +# block hashes (`SqliteClientError::BlockConflict`) or fails history recovery +# fetching transactions that no longer exist. The robust way to guarantee that +# is to keep the pre-restart chain exactly ONE block long: +# 1. `prepare_chain` mines block 1 (coinbase to A); the wallet syncs to it. +# 2. Wait for zebrad to persist block 1 to its non-finalized-state backup — +# the only thing that carries a non-finalized block across a restart, since +# MAX_BLOCK_REORG_HEIGHT = 1000 leaves block 1 far from finalized. +# 3. Stop zallet and zebrad, restart zebrad with `mining.miner_address = B`. +# zebrad restores exactly block 1, so the wallet's single scanned block is +# unchanged and mining at B only EXTENDS the chain — no height the wallet +# holds is rewritten, so no truncate and no conflict. +# 4. Mine coinbase to B until both A's (block 1) and B's (block 2) coinbase +# outputs are mature. +# 5. Sweep via the UUID form and assert the sweep covered coinbase across +# BOTH receivers. # import os -import subprocess import time from decimal import Decimal @@ -51,6 +46,7 @@ from test_framework.util import ( assert_equal, assert_true, + node_dir, start_node, start_nodes, start_wallets, @@ -59,18 +55,11 @@ wait_and_assert_operationid_status, wait_bitcoinds, wait_zallets, - wallet_dir, - zallet_binary, ) # Coinbase outputs require 100 confirmations before they can be spent. COINBASE_MATURITY = 100 -# zebra-state's MAX_BLOCK_REORG_HEIGHT. Blocks at depth >= this value -# are committed to the finalized state and persist across zebrad -# restart; shallower blocks live in memory and are lost on shutdown. -ZEBRA_MAX_REORG_HEIGHT = 99 - # Seconds the 1+1 mature-coinbase state must hold steady before a sweep. # z_listunspent shows new coinbase before zallet's recover_history scan # task (30s idle tick, not woken on tip change) makes it spendable to @@ -98,10 +87,15 @@ def __init__(self): self.cache_behavior = 'clean' def setup_nodes(self): + # All later NUs must also be listed at height 1; otherwise zebra mines + # a coinbase committing to NU5's consensus branch ID while zallet's + # network params expect the latest NU's branch ID, and zallet rejects + # the coinbase on the first block sync. See ZebraArgs default in + # test_framework/config.py. args = [ ZebraArgs( miner_address=addr, - activation_heights={"NU5": 1}, + activation_heights={"NU5": 1, "NU6": 1, "NU6.1": 1, "NU6.2": 1}, ) for addr in self.miner_addresses ] return start_nodes(self.num_nodes, self.options.tmpdir, args) @@ -122,6 +116,12 @@ def run_test(self): # `z_listunspent`'s canonical encoding. taddr_a = self.miner_addresses[0].strip() + # Wait for the wallet to sync to the node tip. z_getaddressforaccount + # (and other account RPCs) reject with "chain height is unknown" until + # the wallet has committed at least one block, since they anchor the + # address derivation to a chain height. + self.sync_all() + accounts = w0.z_listaccounts() assert_true(len(accounts) >= 1, "Wallet 0 should have at least one account") account_uuid = accounts[0]['account_uuid'] @@ -146,67 +146,61 @@ def run_test(self): print(" miner_address A: {}".format(taddr_a)) print(" second receiver B: {}".format(taddr_b)) - # ---- Phase 1: mine to A, finalizing exactly block 1. ------- - # `prepare_chain` already mined block 1 (coinbase to A). Mine - # ZEBRA_MAX_REORG_HEIGHT (99) more to push block 1 to depth 99 - # — the finalization horizon — and leave blocks 2..100 at - # depth 0..97, all non-finalized. zebrad will drop the - # non-finalized tail on restart, leaving exactly one A - # coinbase output (block 1) preserved across the miner switch. - print("Mining {} blocks at miner=A to finalize block 1...".format( - ZEBRA_MAX_REORG_HEIGHT)) - node.generate(ZEBRA_MAX_REORG_HEIGHT) - # Chain tip = 1 + 99 = 100. Block 1 depth 99 → finalized. - - # ---- Phase 2: stop, truncate, switch miner to B. ----------- - print("Stopping wallet and node before truncate...") + # ---- Phase 1: persist block 1 (A's coinbase) for the restart. ---- + # `prepare_chain` mined block 1 (coinbase to A) and run_test synced the + # wallet to it. We deliberately mine NOTHING more before the miner + # switch: keeping the pre-restart chain exactly one block long means the + # wallet never scans a height that zebrad won't still have after the + # restart, so there are no orphaned wallet records to conflict or to + # choke history recovery. + # + # MAX_BLOCK_REORG_HEIGHT = 1000, so block 1 is nowhere near finalized; + # zebrad carries it across the restart only via its non-finalized-state + # backup, written at most every MIN_DURATION_BETWEEN_BACKUP_UPDATES (5s) + # and NOT flushed on clean shutdown. Wait for block 1's backup file to + # appear (named by block hash, in display order) before stopping, so the + # restored chain is guaranteed to contain it. + assert_equal(node.getblockcount(), 1, + "Expected exactly block 1 before the miner switch") + block1_hash = node.getbestblockhash() + backup_dir = os.path.join( + node_dir(self.options.tmpdir, 0), "non_finalized_state", "regtest") + print("Waiting for zebra to back up block 1 ({}...)".format(block1_hash[:16])) + deadline = time.time() + 60 + while time.time() < deadline: + if os.path.exists(os.path.join(backup_dir, block1_hash)): + break + time.sleep(0.5) + else: + raise AssertionError( + "zebra did not persist block 1 to its non-finalized backup " + "within 60s; cannot guarantee it survives the restart") + + # ---- Phase 2: stop, switch miner to B, restart. ---- + # No wallet truncate and no zcash/wallet#447 workaround are needed: the + # wallet has only scanned block 1, which zebrad restores unchanged, so + # mining at B only EXTENDS the chain (blocks 2+). No height the wallet + # already holds is rewritten -> no BlockConflict, and no orphaned + # transaction for history recovery to fetch. + print("Stopping wallet and node before the miner switch...") stop_wallets(self.wallets) wait_zallets() stop_node(self.nodes[0], 0) wait_bitcoinds() - # Truncate the wallet's block-scan state so it no longer holds - # records for heights zebrad is about to lose. After shutdown - # zebrad only persists blocks at depth >= ZEBRA_MAX_REORG_HEIGHT; - # the rest live in memory and are discarded. Re-mining those - # heights with a different miner address produces different - # block hashes; without truncate, the wallet upserts mismatched - # hashes -> `BlockConflict`. - # - # Truncate to 1: matches what zebrad still has post-restart - # (block 1), discards records for the about-to-be-lost tail. - # 1 is the lowest usable target — there is no `blocks` row at - # height 0. - datadir = wallet_dir(self.options.tmpdir, 0) - print("Truncating wallet block-scan state to height 1...") - result = subprocess.run( - [zallet_binary(), "-d=" + datadir, - "repair", "truncate-wallet", "1"], - capture_output=True, text=True) - if result.returncode != 0: - raise AssertionError( - "zallet repair truncate-wallet failed (rc={}):\n" - "stdout:\n{}\nstderr:\n{}".format( - result.returncode, result.stdout, result.stderr)) - - # Workaround for zcash/wallet#447: the post-restart wallet - # would otherwise query the un-mined transactions truncate - # left behind, get a "no such tx" response, and exit. Pre- - # retire those rows here so the query skips them. Remove once - # zcash/wallet#447 is resolved. - wallet_db = os.path.join(datadir, "wallet.db") - subprocess.run( - ["sqlite3", wallet_db, - "UPDATE transactions SET confirmed_unmined_at_height = 2000000000 " - "WHERE mined_height IS NULL;"], - check=True, capture_output=True, text=True) - print("Restarting zebrad with miner=B...") self.nodes[0] = start_node( 0, self.options.tmpdir, ZebraArgs(miner_address=taddr_b, - activation_heights={"NU5": 1})) + activation_heights={"NU5": 1, "NU6": 1, + "NU6.1": 1, "NU6.2": 1})) node = self.nodes[0] + # zebra must have restored exactly block 1 (A's coinbase). If the backup + # had not captured it, the restored tip would be genesis and the premise + # (one preserved A coinbase) would be invalid — fail loudly rather than + # silently re-mining height 1 under miner B. + assert_equal(node.getblockcount(), 1, + "Post-restart zebra should have restored exactly block 1") self.wallets = start_wallets(self.num_wallets, self.options.tmpdir) w0 = self.wallets[0]