fix(platform): derive shielded identity-create id from the padded bundle's published nullifiers#3843
Conversation
… published nullifiers The type-20 builder derived the new identity's id from the requested spends' nullifiers, but BundleType::DEFAULT pads a single-spend bundle to the 2-action minimum — and the padding action's random dummy nullifier is published on the wire. Consensus re-derives the id over ALL published action nullifiers, so every 1-note identity creation failed the builder's own post-proving id-consistency check (after paying the full Halo 2 proving cost). Fix: new build_spend_bundle_with / prove_and_sign_bundle_with variants take a closure that computes the extra sighash data from the BUILT bundle's published nullifiers — after Builder::build fixes the action set (padding included), before the sighash is bound. The identity id is now derived inside that hook, so it always equals the consensus re-derivation. The post-build consistency assert is kept as an invariant guard. Regression tests (real Halo 2 proving): a single-spend padded build succeeds with id == derive_identity_id_from_actions(actions) and != the real-spends-only derivation; a two-spend unpadded build matches both; the closure contract (published set, on-wire order, padding included) and closure-error short-circuit are pinned. Also dedupes a doubled comment paragraph in drive-abci's type-20 state arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shielded_unshield_to rejected the parsed output address with a raw Network equality check, but bech32m parsing can only yield Mainnet or Testnet (testnet/devnet/regtest share the "tdash" HRP per DIP-0018) — so a Devnet wallet rejected every address, including its own, making unshield unusable on devnet/regtest. Compare hrp_for_network() of both sides instead, matching what the encoding can actually represent. A new test pins the DIP-0018 premise (testnet/devnet/regtest share the HRP; mainnet differs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refactors Orchard shielded bundle builders to accept closures for extra sighash computation and updates identity creation to derive IDs from published action nullifiers. HRP-based network validation is added to platform address handling and integrated into wallet logic. ChangesShielded Bundle Builder and Network Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 0bd3395) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3843 +/- ##
============================================
+ Coverage 87.04% 87.12% +0.08%
============================================
Files 2677 2641 -36
Lines 329918 327535 -2383
============================================
- Hits 287182 285370 -1812
+ Misses 42736 42165 -571
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "31fc381d66878fad5f7ac47075f9e2f924a4c9a9bb52e54ee2c3191a5bf5cc62"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two narrowly-scoped client-side fixes: (1) derive the shielded identity-create id from the built bundle's published action nullifiers via a post-build closure so it matches the consensus rederivation over padded actions, and (2) compare unshield destination by HRP instead of raw Network so devnet/regtest work. Both changes are correct, well-tested, and have no consensus surface. Only minor test-coverage and documentation nitpicks remain; nothing blocking.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/shielded/builder/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/mod.rs:685-700: Short-circuit test does not actually enforce "before proving"
The test name and intent — that the closure error propagates before Halo 2 proving runs — is the exact invariant motivating the closure refactor (the original bug burned a full ~30 s proof before failing). But the test passes `TestProver`, so it only verifies that the closure error propagates. If `prove_and_sign_bundle_with` were ever reordered to call `create_proof` before invoking the closure and then return the same closure error, this test would still pass. Use a test-only prover whose `proving_key()` panics so any accidental call into proving fails the test deterministically.
| fn prove_and_sign_bundle_with_closure_error_short_circuits_before_proving() { | ||
| let builder = output_only_builder(10_000); | ||
|
|
||
| let result = prove_and_sign_bundle_with(builder, &TestProver, &[], |_| { | ||
| Err(ProtocolError::ShieldedBuildError( | ||
| "closure rejected".to_string(), | ||
| )) | ||
| }); | ||
|
|
||
| match result { | ||
| Err(ProtocolError::ShieldedBuildError(msg)) => { | ||
| assert_eq!(msg, "closure rejected", "closure error must pass through"); | ||
| } | ||
| other => panic!("expected the closure's error to propagate, got {:?}", other), | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Short-circuit test does not actually enforce "before proving"
The test name and intent — that the closure error propagates before Halo 2 proving runs — is the exact invariant motivating the closure refactor (the original bug burned a full ~30 s proof before failing). But the test passes TestProver, so it only verifies that the closure error propagates. If prove_and_sign_bundle_with were ever reordered to call create_proof before invoking the closure and then return the same closure error, this test would still pass. Use a test-only prover whose proving_key() panics so any accidental call into proving fails the test deterministically.
source: ['codex']
2 participants
Issue being fixed or feature implemented
Two client-side shielded bugs found during the paloma E2E run of the full shielded transition matrix (types 15–20), both blocking real-world flows:
Type-20 (
IdentityCreateFromShieldedPool) was broken for single-note spends. The builder derived the new identity's id from the requested spends' nullifiers, but Orchard'sBundleType::DEFAULTpads a single-spend bundle to the 2-action minimum — and the padding action's random dummy nullifier is published on the wire. Consensus re-derives the id over all published action nullifiers, so the builder's id (and the id bound into the Orchardextra_sighash_data) diverged from the consensus derivation whenever padding occurred. The builder's own post-proving id-consistency check caught the mismatch and failed the build — but only after paying the full Halo 2 proving cost, every time, making every 1-note identity creation unusable.Unshield was unusable on devnet/regtest.
PlatformWallet::shielded_unshield_tocompared the parsed output address's network against the SDK network with rawNetworkequality — but bech32m parsing can only ever yieldMainnetorTestnet(testnet/devnet/regtest all share thetdashHRP per DIP-0018), so aDevnetwallet rejected every address, including its own.What was done?
Commit 1 — type-20 id derivation from the padded bundle (
rs-dpp):build_spend_bundle_with/prove_and_sign_bundle_withvariants that take a closureFnOnce(&[[u8; 32]]) -> Result<Vec<u8>, ProtocolError>computing the extra sighash data from the built bundle's published action nullifiers — extracted afterBuilder::buildfixes the action set (padding included) and before the sighash is bound / proof created. The existing fixed-data functions delegate to the_withvariants.build_identity_create_from_shielded_pool_transitionnow derives the identity id inside that hook (identity_id_from_nullifiers(published_nullifiers)) and binds the extra sighash data there, so the bound id always equals the consensus re-derivation. The post-build id-consistency assert is kept as a true invariant guard.state.rstype-20 arm (comment-only).Commit 2 — unshield address network check by HRP (
rs-platform-wallet):shielded_unshield_tonow comparesPlatformAddress::hrp_for_network(addr_network)againsthrp_for_network(self.sdk.network)instead of raw networks, matching what the bech32m encoding can actually represent.How Has This Been Tested?
New regression tests, all with real Halo 2 proving (the cached
TestProverpattern):single_spend_padded_bundle_derives_id_from_published_nullifiers— the bug's exact shape: a 1-spend build (witnessed via the samemerkle_path.root(cmx)anchor pattern production uses) must succeed, be padded to 2 actions, carry the real nullifier among the published set, and produceidentity_id == derive_identity_id_from_actions(actions)while!= identity_id_from_nullifiers([real_nullifier])— proving the dummy participates. Pre-fix this failed the post-proving consistency check.two_spend_unpadded_bundle_id_matches_real_nullifier_derivation— no padding: the published set equals the real spends', and both derivations agree.prove_and_sign_bundle_with_closure_receives_published_nullifiers— the closure contract: it runs once and receives exactly the authorized bundle's published nullifiers (one per action, padding included, on-wire order).prove_and_sign_bundle_with_closure_error_short_circuits_before_proving— closure errors propagate before any proving work.hrp_is_shared_across_all_non_mainnet_networks— pins the DIP-0018 premise the wallet check now relies on (testnet/devnet/regtest sharetdash; mainnet differs).cargo check --workspace --all-targetsgreen;cargo check -p dpp / -p platform-wallet --all-features --testsgreen; clippy clean on the touched crates; existing shielded builder suites still pass.Both fixes were also exercised end-to-end on devnet paloma during the shielded matrix run: post-fix, unshield (type 17) completed pool → platform-address, and the type-20 built, proved, and broadcast successfully with a single-note spend.
Breaking Changes
None. Client-side builder/wallet fixes only — no consensus, serialization, or protocol-version changes.
build_spend_bundle/prove_and_sign_bundlekeep their existing signatures (the_withvariants are additive, crate-internal).Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests