Skip to content

qa: fix and re-enable wallet_z_shieldcoinbase tests #140

Open
Cosmos-Harry wants to merge 1 commit into
mainfrom
zd/enable-z-shieldcoinbase-tests
Open

qa: fix and re-enable wallet_z_shieldcoinbase tests #140
Cosmos-Harry wants to merge 1 commit into
mainfrom
zd/enable-z-shieldcoinbase-tests

Conversation

@Cosmos-Harry

@Cosmos-Harry Cosmos-Harry commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Motivation

Both z_shieldcoinbase rpc-tests were in DISABLED_SCRIPTS pending 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.py

  • Consensus branch mismatch. setup_nodes activated only NU5 at 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 activates NU5/NU6/NU6.1/NU6.2 at height 1, matching the ZebraArgs default.
  • Wallet not synced before account RPCs. Added sync_all() before z_getnewaccount, which returns "Wallet sync required" until the wallet has committed ≥1 block.
  • Coinbase-maturity off-by-one in the expected-mature count.

wallet_z_shieldcoinbase_multi_taddr.py

  • Same activation-height + wallet-sync fixes (here the symptom was z_getaddressforaccount failing with "chain height is unknown").
  • Restart redesign. The old miner-switch logic 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 + data_requests sync 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 with miner=B and only extends the chain — no truncate, no orphaned wallet records, no #447 workaround.

⚠️ Known CI blocker (zaino) — affects wallet_z_shieldcoinbase.py only

wallet_z_shieldcoinbase_multi_taddr.py passes 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_state::chain_index::finalised_state::db::v1::write_core: syncing finalised blocks 185..=243
zaino_state::chain_index: Sync loop iteration failed (N/10) ... ErrorFromSource(LmdbError(MapFull))
Sync loop failed 10 consecutive times, giving up
→ Chain indexer task exited → "Exiting Zallet because an ongoing task exited"
→ the test's wallet RPC disappears → wait_for_mature_coinbase_count fails at ~563s

zaino's chain-index LMDB hits MapFull while 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 lighter multi_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.py should 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:

wallet_z_shieldcoinbase.py            -> All z_shieldcoinbase tests passed! / Tests successful
wallet_z_shieldcoinbase_multi_taddr.py -> PASSED (12.5 ZEC swept from 2 receivers) / Tests successful

In CI: multi_taddr ✅ passes; wallet_z_shieldcoinbase.py ❌ blocked by the zaino MapFull above.

AI disclosure

Used Claude Code (Opus 4.8) to diagnose the failures, implement the fixes, and draft this PR.

🤖 Generated with Claude Code

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>
@Cosmos-Harry Cosmos-Harry marked this pull request as draft June 22, 2026 20:42
@Cosmos-Harry Cosmos-Harry marked this pull request as ready for review June 22, 2026 21:51

@nullcopy nullcopy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Cosmos-Harry

Copy link
Copy Markdown
Collaborator Author

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?

updated the description, sorry it was stale

@Cosmos-Harry Cosmos-Harry changed the title qa: fix and re-enable wallet_z_shieldcoinbase tests feature/cor-1411 Jun 24, 2026
@linear

linear Bot commented Jun 24, 2026

Copy link
Copy Markdown

COR-1411

@Cosmos-Harry Cosmos-Harry changed the title feature/cor-1411 qa: fix and re-enable wallet_z_shieldcoinbase tests Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants