qa: fix and re-enable wallet_z_shieldcoinbase tests #140
Open
Cosmos-Harry wants to merge 1 commit into
Open
Conversation
nullcopy
reviewed
Jun 23, 2026
Contributor
There was a problem hiding this comment.
The wallet_z_shieldcoinbase test is still failing as of 2b03d25. The failure is:
2026-06-22T21:29:23.105047Z ERROR zaino_state::chain_index: Sync loop failed 10 consecutive times, giving up: ErrorFromSource(LmdbError(MapFull))
Is this the same error you were fixing in the other tests?
Collaborator
Author
updated the description, sorry it was stale |
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/zallet#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 <noreply@anthropic.com>
af985be to
3ffd8d5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Both
z_shieldcoinbaserpc-tests were inDISABLED_SCRIPTSpending fixes. This re-enables them so they run in CI (follow-up to #138's CI-green work).What was wrong & the fix
wallet_z_shieldcoinbase.pysetup_nodesactivated onlyNU5at height 1, so zebra mined a coinbase committing to NU5's branch ID while zallet's params expected the latest NU's — zallet rejected the first block ("coinbase tx's claimed height doesn't match its consensus branch ID"). Now activatesNU5/NU6/NU6.1/NU6.2at height 1, matching theZebraArgsdefault.sync_all()beforez_getnewaccount, which returns "Wallet sync required" until the wallet has committed ≥1 block.wallet_z_shieldcoinbase_multi_taddr.pyz_getaddressforaccountfailing with "chain height is unknown").MAX_BLOCK_REORG_HEIGHT = 99and 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 thetruncate-wallet+data_requestssync task exits the wallet on a "no such tx" response that should be recoverable zallet#447 SQL workaround no longer held (history recovery exited trying to fetch transactions zebra no longer had). The test now keeps the pre-restart chain exactly one block long, waits for zebra to back up block 1, then restarts withminer=Band only extends the chain — no truncate, no orphaned wallet records, no #447 workaround.wallet_z_shieldcoinbase.pyonlywallet_z_shieldcoinbase_multi_taddr.pypasses in CI (shard-2).wallet_z_shieldcoinbase.py(shard-1) currently fails in CI, but not because of the test logic — it's a zaino issue:zaino's chain-index LMDB hits
MapFullwhile syncing its finalized state. This test is block-heavy (100+ maturity blocks plus per-sweep mining, ~280+ blocks total), so it crosses the limit; the lightermulti_taddr(~101 blocks) stays under it. The failure is deterministic — a re-run reproduced it identically.This is independent of the test changes here (the test passes locally against a zaino with an adequate map). CI builds zaino from
zingolabs/zaino@refs/heads/dev; the default map size claims 384 GB but is effectively not being applied in that build, so a few-hundred-block regtest chain fills it.Unblock: fix the chain-index LMDB map size on zaino dev (or expose it so integration-tests can configure it). Once that lands,
wallet_z_shieldcoinbase.pyshould pass with the fixes already in this PR. Tracking separately with the zaino team.Test evidence
Both pass locally against zebrad 5.2.0 and zallet 0.1.0-alpha.3:
In CI:
multi_taddr✅ passes;wallet_z_shieldcoinbase.py❌ blocked by the zainoMapFullabove.AI disclosure
Used Claude Code (Opus 4.8) to diagnose the failures, implement the fixes, and draft this PR.
🤖 Generated with Claude Code