From a5a0545d52405caa1fbc78a4fd3f12529339861d Mon Sep 17 00:00:00 2001 From: nol4lej Date: Thu, 13 Aug 2026 10:20:49 -0400 Subject: [PATCH 1/3] fix(shielded-pool): prune a fixed batch per block, not the block's leftover weight --- Cargo.lock | 2 +- frame/shielded-pool/CHANGELOG.md | 26 ++++ frame/shielded-pool/Cargo.toml | 2 +- frame/shielded-pool/src/benchmarking.rs | 8 +- frame/shielded-pool/src/lib.rs | 55 ++++---- frame/shielded-pool/src/merkle/mod.rs | 153 ++++++++++++++++++++- ts-tests/node/sealed-tree-pruning.test.cjs | 4 +- 7 files changed, 206 insertions(+), 44 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 61647ad0..360658c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8104,7 +8104,7 @@ dependencies = [ [[package]] name = "pallet-shielded-pool" -version = "0.17.1" +version = "0.17.2" dependencies = [ "ark-bn254", "ark-ff 0.5.0", diff --git a/frame/shielded-pool/CHANGELOG.md b/frame/shielded-pool/CHANGELOG.md index 6ea1c580..dfb65f77 100644 --- a/frame/shielded-pool/CHANGELOG.md +++ b/frame/shielded-pool/CHANGELOG.md @@ -2,6 +2,32 @@ All notable changes to `pallet-shielded-pool` will be documented in this file. +## [0.17.2] - 2026-08-13 + +### Fixed + +- **The sealed-node sweep no longer sizes its batch from the block's leftover + weight — this halted the public testnet at block 406997.** `on_idle` received + `remaining` and divided it by the benchmarked per-node cost to pick how many + nodes to prune. Leftover weight is not consensus: once post-dispatch refunds + are in play an author and an importer measure the same block slightly + differently, so each pruned a different number of nodes and wrote a different + state. Frontier folds that state into the Ethereum block header it builds in + `on_finalize`, so the divergence surfaced as a mismatched `"fron"` digest and + `Executive::final_checks` panicked with *"Digest item must match that + calculated."* — every node rejecting every other node's block. + + Three validators, identical state at 406997 and byte-identical extrinsics in + 406998, produced three mutually unimportable blocks and the chain stopped for + 4 hours. It survived five days only because empty blocks left the same + leftover weight everywhere; the first block carrying real EVM traffic split + the network. + + The sweep now runs in `on_initialize` with a constant batch + (`PRUNED_NODES_PER_BLOCK`, unchanged at 512), which costs a fixed ~6.5 ms of a + 2 s block. Regression test: + `prune_batch_is_independent_of_block_fullness`. + ## [0.17.1] - 2026-08-11 ### Security diff --git a/frame/shielded-pool/Cargo.toml b/frame/shielded-pool/Cargo.toml index 30642092..4856166a 100644 --- a/frame/shielded-pool/Cargo.toml +++ b/frame/shielded-pool/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pallet-shielded-pool" -version = "0.17.1" +version = "0.17.2" description = "Shielded pool pallet for private transactions using ZK proofs" authors = ["Orbinum Team"] license = "GPL-3.0-or-later" diff --git a/frame/shielded-pool/src/benchmarking.rs b/frame/shielded-pool/src/benchmarking.rs index 19cd3116..c3c65514 100644 --- a/frame/shielded-pool/src/benchmarking.rs +++ b/frame/shielded-pool/src/benchmarking.rs @@ -281,11 +281,11 @@ mod benchmarks { ); } - /// Cost of one `on_idle` sweep that removes `n` sealed-tree nodes. + /// Cost of one sweep that probes `n` sealed-tree nodes. /// - /// Not an extrinsic: the sweep runs in `on_idle` with whatever weight the - /// block has left. It still needs measuring, because the hook must return the - /// weight it actually consumed — declaring less would let a block overrun. + /// Not an extrinsic: the sweep runs in `on_initialize` over a fixed batch. It + /// still needs measuring, because the hook must return the weight it actually + /// consumed — declaring less would let a block overrun. /// /// The setup seals a tree and populates its prunable levels directly rather /// than inserting 2^20 leaves, which no benchmark could run. What matters for diff --git a/frame/shielded-pool/src/lib.rs b/frame/shielded-pool/src/lib.rs index bfd31aef..b623ae7a 100644 --- a/frame/shielded-pool/src/lib.rs +++ b/frame/shielded-pool/src/lib.rs @@ -111,12 +111,13 @@ pub mod pallet { /// (`migrations::v3::MigrateToV3`); both historic-root items carry an expiry. pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(3); - /// Ceiling on how many sealed-tree nodes one `on_idle` pass may drop. + /// How many sealed-tree nodes each block sweeps. /// - /// The weight budget already bounds the sweep; this bounds the trie churn on - /// an idle chain, where the leftover weight would otherwise allow tens of - /// thousands of removals in a single block. - pub(crate) const MAX_PRUNED_NODES_PER_BLOCK: u32 = 512; + /// Fixed, never derived from the block's leftover weight — see `on_initialize` + /// for why that distinction is a consensus matter and not a tuning knob. At + /// ~12.7 µs of ref_time per node this costs ~6.5 ms of a 2 s block, and clears + /// a sealed 1 M-node tree in roughly 2 048 blocks (~3.4 hours at 6 s). + pub(crate) const PRUNED_NODES_PER_BLOCK: u32 = 512; #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] @@ -253,7 +254,7 @@ pub mod pallet { /// Resume point for the sealed-tree node sweep: `(tree_id, level, index)`. /// /// Pruning a sealed tree touches ~1M keys, far more than one block can absorb, - /// so `on_idle` walks it in bounded batches and parks the cursor here. `None` + /// so `on_initialize` walks it in bounded batches and parks the cursor here. `None` /// means the sweep is idle — either nothing has sealed yet, or every sealed /// tree is already pruned. #[pallet::storage] @@ -407,35 +408,25 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { - /// Reclaim internal Merkle nodes from sealed trees with whatever weight - /// the block has left over. + /// Reclaim internal Merkle nodes from sealed trees, a fixed batch per block. /// /// A sealed tree holds ~1M prunable nodes — orders of magnitude past one /// block — so the sweep runs in bounded batches and parks its position in - /// `SealedPruneCursor`. Doing this in `on_idle` rather than `on_initialize` - /// keeps it off the critical path: a busy block simply skips it, and the - /// work resumes when the chain has room. - fn on_idle(_now: BlockNumberFor, remaining: Weight) -> Weight { - // Size the batch from the benchmarked per-node cost, so a nearly-full - // block prunes little or nothing and an idle one prunes up to the cap. - // Deriving it from the same `WeightInfo` the hook reports with keeps the - // budget and the charge from drifting apart. - let base = T::WeightInfo::prune_sealed_nodes(0); - let per_node = T::WeightInfo::prune_sealed_nodes(1).saturating_sub(base); - - let Some(available) = remaining.checked_sub(&base) else { - return Weight::zero(); // not even the cursor read fits - }; - if per_node.ref_time() == 0 || per_node.proof_size() == 0 { - return Weight::zero(); - } - let budget = (available.ref_time() / per_node.ref_time()) - .min(available.proof_size() / per_node.proof_size()) - .min(MAX_PRUNED_NODES_PER_BLOCK as u64) as u32; - - let removed = crate::merkle::MerkleTreeService::prune_sealed_nodes::(budget); - // Charged even when nothing was removed: the cursor read happened. - T::WeightInfo::prune_sealed_nodes(removed) + /// `SealedPruneCursor`. + /// + /// The batch size is a constant and must stay one. Sizing it from the + /// block's leftover weight looks free, but leftover weight is not + /// consensus: an author and an importer measure the same block slightly + /// differently once post-dispatch refunds are in play, so each would prune + /// a different number of nodes and their state roots would diverge. That + /// halted the testnet at block 406997. + fn on_initialize(_now: BlockNumberFor) -> Weight { + crate::merkle::MerkleTreeService::prune_sealed_nodes::(PRUNED_NODES_PER_BLOCK); + // The full batch is charged, not the removals: the sweep probes + // `PRUNED_NODES_PER_BLOCK` keys either way, and a miss costs the same + // read as a hit. Charging removals would under-declare an all-miss + // pass and let the block admit extrinsics it cannot pay for. + T::WeightInfo::prune_sealed_nodes(PRUNED_NODES_PER_BLOCK) } fn integrity_test() { diff --git a/frame/shielded-pool/src/merkle/mod.rs b/frame/shielded-pool/src/merkle/mod.rs index ee311cb4..5a883356 100644 --- a/frame/shielded-pool/src/merkle/mod.rs +++ b/frame/shielded-pool/src/merkle/mod.rs @@ -1217,7 +1217,7 @@ mod prune_tests { use crate::{ Config, mock::{Test, new_test_ext}, - pallet::{LastPrunedTree, MAX_PRUNED_NODES_PER_BLOCK, SealedPruneCursor}, + pallet::{LastPrunedTree, PRUNED_NODES_PER_BLOCK, SealedPruneCursor}, storage::MerkleRepository, types::Commitment, }; @@ -1402,14 +1402,159 @@ mod prune_tests { }); } - /// The per-block ceiling has to be a real bound, not a placeholder. + /// Two nodes running the same block must end at the same state root. + /// + /// The old `on_idle` hook received the block's leftover weight, which is NOT consensus: an + /// author that filled the block differently from what the importer measures + /// hands the hook a different leftover weight, the sweep prunes a different + /// number of nodes, and the two states diverge. That halted the testnet at + /// block 406997. + /// + /// Regression: the sweep must consume a constant batch, so two nodes running + /// the same block land on the same state regardless of how full it was. + #[test] + fn prune_batch_is_independent_of_block_fullness() { + // The mock's 8-leaf tree is swept dry in one call, which would hide the + // divergence. Production seals 1_048_576-leaf trees, where the batch + // genuinely bounds the sweep — seal several mock trees to match. + fn sweep_once() -> (u32, Option<(u32, u8, u32)>) { + new_test_ext().execute_with(|| { + for _ in 0..40 { + seal_one_tree(); + } + let removed = MerkleTreeService::prune_sealed_nodes::(PRUNED_NODES_PER_BLOCK); + (removed, SealedPruneCursor::::get()) + }) + } + + // Same block, two nodes. Nothing about how full the block was may reach + // the sweep, so both must land identically. + assert_eq!( + sweep_once(), + sweep_once(), + "the sealed-node sweep is not deterministic: author and importer would \ + disagree on state and the chain would fork" + ); + } + + /// How many internal nodes the forest still holds — the sweep's whole effect. + fn surviving_nodes() -> usize { + crate::pallet::MerkleNodes::::iter().count() + } + + // ── adversarial: can anything reintroduce the divergence? ───────────────── + // + // The fix is only worth what it survives. Each of these attacks the sweep + // from a different angle, trying to make two nodes running the same block + // prune differently. + + /// Attack: run the sweep from a cursor parked anywhere, repeatedly. + /// + /// The full sweep of a forest must reach the same end state no matter how + /// the batches were carved up — otherwise a node that restarted mid-sweep + /// would land somewhere its peers never do. + #[test] + fn sweep_converges_regardless_of_how_the_batches_are_carved() { + fn sweep_to_exhaustion(batch: u32) -> (u32, Option<(u32, u8, u32)>, usize) { + new_test_ext().execute_with(|| { + for _ in 0..20 { + seal_one_tree(); + } + let mut total = 0; + // Bounded: a stuck sweep must fail the test, not hang it. + for _ in 0..10_000 { + let removed = MerkleTreeService::prune_sealed_nodes::(batch); + total += removed; + if SealedPruneCursor::::get().is_none() && removed == 0 { + break; + } + } + (total, SealedPruneCursor::::get(), surviving_nodes()) + }) + } + + // One node sweeps in tiny batches, another in large ones. Same forest, + // so the same nodes must end up gone. + let fine = sweep_to_exhaustion(1); + let coarse = sweep_to_exhaustion(PRUNED_NODES_PER_BLOCK); + assert_eq!( + fine, coarse, + "batch size changed the end state: nodes that swept at different \ + rates would hold different tries" + ); + } + + /// Attack: seal more trees mid-sweep, the way a live chain does. + /// + /// The cursor walks tree-by-tree while the forest grows underneath it. Two + /// nodes that saw the same insertions must still agree. + #[test] + fn sweep_is_stable_while_the_forest_grows() { + fn interleaved(batch: u32) -> usize { + new_test_ext().execute_with(|| { + for _ in 0..20 { + seal_one_tree(); + // A block's worth of sweeping between each sealing. + MerkleTreeService::prune_sealed_nodes::(batch); + } + for _ in 0..2_000 { + let removed = MerkleTreeService::prune_sealed_nodes::(batch); + if SealedPruneCursor::::get().is_none() && removed == 0 { + break; + } + } + surviving_nodes() + }) + } + + assert_eq!( + interleaved(PRUNED_NODES_PER_BLOCK), + interleaved(PRUNED_NODES_PER_BLOCK), + "interleaving sealing with sweeping is not reproducible" + ); + } + + /// Attack: the sweep must never touch the tree still being written to. + /// + /// This is the property that keeps today's notes spendable at O(depth); the + /// batch change moved the hook, so re-prove it rather than assume it held. + #[test] + fn active_tree_survives_an_exhaustive_sweep() { + new_test_ext().execute_with(|| { + let cap = seal_one_tree(); + // Start a second tree and leave it active with one leaf. + let mut c = [0u8; 32]; + c[..4].copy_from_slice(&cap.to_le_bytes()); + MerkleTreeService::insert_leaf::(Commitment(c)).expect("insert"); + + for _ in 0..1_000 { + if MerkleTreeService::prune_sealed_nodes::(PRUNED_NODES_PER_BLOCK) == 0 + && SealedPruneCursor::::get().is_none() + { + break; + } + } + + let active = MerkleRepository::get_tree_size::() / cap; + let cut = ::SealedTreePrunedBelowLevel::get(); + for level in 1..cut { + assert!( + MerkleRepository::get_node::(active, level, 0).is_some(), + "active tree lost node at level {level}: paths would no longer \ + be O(depth) and the sweep is eating live state" + ); + } + }); + } + + /// The per-block batch has to be a real bound, not a placeholder. /// /// A `const` block rather than a runtime assert: both operands are constants, /// so the compiler would fold an `assert!` away and the check would never run. /// This one fails the build instead. const _: () = assert!( - MAX_PRUNED_NODES_PER_BLOCK > 0 && MAX_PRUNED_NODES_PER_BLOCK <= 4096, - "MAX_PRUNED_NODES_PER_BLOCK must bound the sweep: zero disables pruning, \ + PRUNED_NODES_PER_BLOCK > 0 && PRUNED_NODES_PER_BLOCK <= 4096, + "PRUNED_NODES_PER_BLOCK must bound the sweep: zero disables pruning, \ and a value this far above the benchmarked batch would let one block \ absorb work it cannot pay for" ); diff --git a/ts-tests/node/sealed-tree-pruning.test.cjs b/ts-tests/node/sealed-tree-pruning.test.cjs index c8188bcb..9eb08e6e 100644 --- a/ts-tests/node/sealed-tree-pruning.test.cjs +++ b/ts-tests/node/sealed-tree-pruning.test.cjs @@ -1,7 +1,7 @@ // Sealed trees keep ~1M internal `MerkleNodes` entries forever (~72 MiB each) // purely to serve Merkle paths to wallets. No dispatchable reads them, so levels -// below `SealedTreePrunedBelowLevel` are pruned in `on_idle` and rebuilt from the -// leaves when a path needs them. +// below `SealedTreePrunedBelowLevel` are pruned in `on_initialize` and rebuilt +// from the leaves when a path needs them. // // Sealing a tree needs 2^20 shields — not reachable here, since // `MaxLeavesPerTree` is a compile-time constant. What this file DOES validate From 07a5dd04b3e5018b5a6886c22ff3f707aa50fc41 Mon Sep 17 00:00:00 2001 From: nol4lej Date: Thu, 13 Aug 2026 10:21:10 -0400 Subject: [PATCH 2/3] fix(runtime): derive the relay selector fallback from the precompile decoder --- template/runtime/src/lib.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index e18ff9f5..f33a9036 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -924,9 +924,12 @@ impl_runtime_apis! { let allowed_selectors = { let stored = pallet_relayer::Pallet::::allowed_selectors(); if stored.is_empty() { + // Taken from the precompile itself rather than copied: a + // literal here that drifts from the decoder fails silently, + // since a wrong selector is simply "unsupported". sp_std::vec![ - [0x47, 0xfc, 0x44, 0xa2], // unshield - [0x8c, 0x0f, 0x5d, 0x24], // privateTransfer + pallet_evm_precompile_shielded_pool::selectors::UNSHIELD, + pallet_evm_precompile_shielded_pool::selectors::PRIVATE_TRANSFER, ] } else { stored From 9de560efe8b90ca52c086b72f4283553337f3c04 Mon Sep 17 00:00:00 2001 From: nol4lej Date: Thu, 13 Aug 2026 10:21:28 -0400 Subject: [PATCH 3/3] chore(runtime): bump spec_version to 9 for the pool-tag, relay-selector and prune fixes --- template/runtime/RUNTIME_VERSIONS.md | 82 ++++++++++++++++++++++++++++ template/runtime/src/lib.rs | 2 +- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/template/runtime/RUNTIME_VERSIONS.md b/template/runtime/RUNTIME_VERSIONS.md index 67fd77cf..6b7b1b5a 100644 --- a/template/runtime/RUNTIME_VERSIONS.md +++ b/template/runtime/RUNTIME_VERSIONS.md @@ -20,6 +20,88 @@ to `spec_version` / `transaction_version` must add a row here in the same PR. The genesis reset (`69d1b837`) set `spec_version` back to 1 and `transaction_version` to 1 for the public testnet launch. +### spec 9 — tx 2 — 2026-08-13 + +Security fixes plus one consensus fix. **`transaction_version` stays at 2** — +no dispatch signature changes, so offline-signed extrinsics remain valid and +wallet and runtime do NOT have to ship together. + +**No migration, no storage change.** + +**Consensus** + +- **The sealed-node sweep no longer sizes its batch from the block's leftover + weight — this halted the public testnet at block 406997.** + `pallet-shielded-pool`'s `on_idle` divided `remaining` by the benchmarked + per-node cost to pick how many nodes to prune. Leftover weight is not + consensus: once post-dispatch refunds are in play an author and an importer + measure the same block slightly differently, so each pruned a different + number of nodes and wrote a different state. Frontier folds that state into + the Ethereum block header it builds in `on_finalize`, so the divergence + surfaced as a mismatched `"fron"` digest and `Executive::final_checks` + panicked with *"Digest item must match that calculated."* + + Three validators with identical state at 406997 and byte-identical extrinsics + in 406998 produced three mutually unimportable blocks; the chain stopped for 4 + hours. It survived five days on spec 8 only because empty blocks leave the + same leftover weight on every node — the first block carrying real EVM + traffic split the network three ways. + + The sweep now runs in `on_initialize` over a constant batch + (`PRUNED_NODES_PER_BLOCK`, unchanged at 512, ~6.5 ms of a 2 s block) and + charges the full batch rather than the removals, since a miss costs the same + read as a hit. The state transition differs only in that it is now identical + on every node. + + A governance runtime upgrade cannot deliver this fix: applying one needs a + block, and a forked network no longer agrees on any. Roll the binary out to + every validator together. + +**Security** + +- **shielded-pool 0.17.1 — pool admission tags one entry per nullifier**, in a + namespace shared with `unshield` (`ShieldedPoolSpend`). `and_provides` + contributes exactly ONE tag, so passing it a `Vec` encoded the whole + nullifier set plus the relayer into a single blob. Three consequences, each + free for an attacker since the fee is only charged on execution: reordering + the two inputs minted a second admissible entry for the same spend; two + transfers sharing only ONE note (A+B and A+C) did not collide at all, so one + note could back unboundedly many entries; and transfer/unshield used + different prefixes, so the same note could back one of each at once. Every + variant propagates and is revalidated network-wide while at most one can + execute. + + `relayer` deliberately leaves the tag. Binding it made a copy with a swapped + fee recipient a *separate* entry, so anyone could rebroadcast another user's + spend pointed at their own account; keyed on the nullifier the two are + mutually exclusive, so taking the fee requires out-bidding — which means + paying it. + + **Admission policy, not state transition** — consensus is unaffected. Nodes + on the old logic keep accepting the duplicate variants, so the mitigation + only completes as the network updates. +- **relay — the selector whitelist was stale for BOTH operations (ME-8).** The + client held `0x47fc44a2` (unshield) and `0x8c0f5d24` (privateTransfer), while + the decoder answers to `0x4e505348` and `0x66ed2cd4`. Derived by keccak, the + stale pair turn out to be real selectors from signatures two versions old. + Relaying was therefore rejecting every call as *"unsupported selector"* — + silently, because that is indistinguishable from a legitimate rejection. The + same stale literals sat in the runtime's fallback list and in + `ts-tests/test-relay-rpc.ts`, so the tests stayed green while testing nothing. + + Both now derive from `pallet_evm_precompile_shielded_pool::selectors::*` (or + from the ABI signature, in the TypeScript tests), and a unit test pins the + client constants against the decoder's. +- **relay — per-operation calldata minimums were both 228 bytes**, the shared + head up to the fee slot. Past that the layouts diverge: unshield's head is 10 + slots (324 with the selector), privateTransfer's is 8 (260). A call between + 228 and its real minimum passed validation and reached the decoder truncated. +- **relay — `gas_price` and the fee word saturate instead of panicking.** + `U256::as_u128()` panics above 2^128. The fee word is caller-controlled over + an unauthenticated RPC, so one crafted 32-byte value took down the handler; + `gas_price` comes from the runtime and is not attacker-reachable, but a panic + there still kills the relay RPC. Node-side only, no consensus effect. + ### spec 8 — tx 2 — 2026-08-08 Bundles the whole audit-remediation batch plus the config/feature work that diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index f33a9036..01bef7ae 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -201,7 +201,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: Cow::Borrowed("orbinum"), impl_name: Cow::Borrowed("orbinum"), authoring_version: 1, - spec_version: 8, + spec_version: 9, impl_version: 1, apis: RUNTIME_API_VERSIONS, transaction_version: 2,