Skip to content

fix(state): propagate transparent index removal errors - #778

Draft
jonwalch wants to merge 5 commits into
zakura-core:mainfrom
jonwalch:research-issue-681
Draft

fix(state): propagate transparent index removal errors#778
jonwalch wants to merge 5 commits into
zakura-core:mainfrom
jonwalch:research-issue-681

Conversation

@jonwalch

@jonwalch jonwalch commented Aug 22, 2026

Copy link
Copy Markdown

Motivation

The non-indexer state migration ignores the RocksDB result from deleting transparent spending indexes. A transient deletion failure can therefore be followed by successful nullifier rewrites and removal of the +indexer marker, leaving a partially removed index that a later rebuild can mistake for complete data.

Closes #777.

Solution

  • Propagate the transparent range-delete failure so the format-change coordinator cannot clear +indexer after that write fails.
  • Inject a real rocksdb::Error at the typed batch-write boundary, ensuring the regression exercises the result returned by write_batch().
  • Exercise failed removal, successful startup retry, and the real rebuild loop with a mixed transparent/Orchard-spend transaction.
  • Keep the rebuild's cross-feature support test-only; production indexer behavior is unchanged.

Testing

The regression seeds transparent and Orchard spend locations at the same synthetic transaction location. It verifies that an injected range-delete failure preserves the +indexer marker and both locations, a retry removes both locations while retaining the consensus Orchard nullifier, and the real rebuild loop restores both locations and resolves them to the spending transaction hash.

Mutation check: temporarily restoring the original let _ = ...write_batch() made the regression fail because database startup incorrectly succeeded.

Completed checks:

  • cargo fmt --all -- --check
  • cargo test -p zakura-state range_delete_error_preserves_and_rebuilds_spend_indexes --lib
  • cargo test -p zakura-state (541 unit tests passed, 3 ignored; 2 integration tests and 1 doc test passed)
  • cargo test -p zakura-state --features indexer --lib -- --skip intra_block_self_spend_chain_in_finalized_state (540 passed, 3 ignored)
  • cargo clippy -p zakura-state --all-targets -- -D warnings
  • cargo clippy -p zakura-state --all-targets --all-features -- -D warnings
  • cargo test -p zakura-state --all-features --all-targets --no-run
  • ./scripts/changelog.py check
  • npx --yes markdownlint-cli@0.39.0 "**/*.md" --ignore node_modules --ignore target --ignore .git --config .github/.markdownlint.yaml
  • git diff --check origin/main...HEAD

The full indexer-feature library run has one pre-existing failure in intra_block_self_spend_chain_in_finalized_state; the same failure reproduces on untouched origin/main. All remaining indexer-feature library tests pass with the command above.

Changelog

Added docs/changelog/unreleased/778.md under Fixed.

Specifications & References

Follow-up Work

@evan-forbes evan-forbes 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 production change correctly propagates the initial range-delete error. It returns before the later nullifier deletions and before the coordinator clears the +indexer marker.

The regression does not yet cover the final acceptance criterion from #777. It commits genesis, inserts one synthetic transparent mapping, and checks the startup retry. It does not create any shielded spend mappings or run the index rebuild. The test therefore does not prove that a later rebuild cannot skip a height with partially removed index data.

Please add the rebuild regression described in #777. The test should inject the removal failure and confirm that the marker remains. It should then retry the operation and run the index rebuild. Finally, it should verify the rebuilt spend indexes. At minimum, please seed transparent and shielded spend mappings. Prove that the injected failure cannot leave one index removed while another remains.

The production fix looks correct. This missing acceptance coverage is my only requested change.

@jonwalch
jonwalch force-pushed the research-issue-681 branch from c579cf0 to 7ae1a7d Compare August 31, 2026 13:24
@jonwalch

Copy link
Copy Markdown
Author

Addressed in 7ae1a7d. The regression now seeds both transparent and Orchard spend locations, verifies a failed removal preserves the marker and both locations, retries removal and verifies both locations are absent while the consensus nullifier remains, then runs the real rebuild loop and verifies both locations and the spending transaction hash are restored. The mutation check still fails when the original ignored write result is restored.

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.

state: propagate errors when removing transparent index entries

2 participants