feat(settlement): commit-reveal batch settlement to prevent proof front-running#95
Merged
JamesEjembi merged 3 commits intoJul 21, 2026
Conversation
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.
Closes #57
Built from scratch per maintainer direction (no pre-existing PoolManager to extend). Implemented in Rust/Soroban — this repo has no EVM/Solidity contracts anywhere, so every file, bound, and cost figure below is Rust-native, not translated from the issue's
.sol/gas framing.File mapping against the issue's navigation guide (this repo has no
contracts/directory or Foundry tooling):contracts/PoolManager.sol→src/settlement/pool_manager.rscontracts/lib/MerkleProof.sol→src/settlement/merkle.rscontracts/lib/CommitReveal.sol→ commit/reveal types and storage insrc/settlement/mod.rs, logic inpool_manager.rscontracts/test/FrontRunning.t.sol→src/settlement/tests.rs::front_runner_cannot_hijack_or_replay_a_revealed_commitmentThreat model: what a single-phase design leaks
A single
settle_batch(root, leaves, proofs)call executes the instant it's submitted — root and all leaf data are visible in the mempool in the same moment they take effect.require_auth()blocks impersonating the settler, but nothing stops an observer from copying the identical calldata and racing to get their own submission included first. Front-running requires content that's visible-but-not-yet-acted-upon; a single-phase design manufactures that window on every call.Commit-reveal closes it:
commit_settlementexposes only an opaque hash (root/salt/leaves stay hidden), andcommitment.settleris bound before anything is public. By the time the reveal's plaintext is visible in the mempool, identity is already fixed — there's no unclaimed slot to race for.Front-running test, with proof it has teeth
front_runner_cannot_hijack_or_replay_a_revealed_commitmentsimulates a front-runner copying a settler's pending reveal and submitting it first (rejected:Unauthorized), confirms no bond moved, lets the real reveal land, then has the front-runner replay the already-settled call (rejected:AlreadySettled, independent of identity).Proof this test actually catches the regression it targets (verified three times across the development and merge process): I temporarily removed sender-binding — deleted the explicit
caller != commitment.settlercheck and changed the hash recomputation to use the trusted stored settler instead of the caller-supplied one, so identity stopped affecting the hash-match gate too. Re-running the same test against the weakened code:The front-runner's copied call didn't error differently — it succeeded and settled the batch. Restored both lines (verified byte-identical against the original) and reran: clean, 25/25 settlement tests.
Deviation from the issue's blueprint: no leaf-index vector in the commitment
The issue's step 3 specifies
keccak256(abi.encodePacked(batchId, msg.sender, root, leafIndices))— packing a dynamic array is a hash-collision hazard (distinct index sets can pack identically). This design sidesteps the class of bug entirely rather than patching around it:Commitment::hash = sha256(batch_id || sha256(settler) || root || leaf_count || salt), all fixed-width fields, no index vector hashed at all. Per-leaf position is instead proven individually by each leaf's own Merkle proof, which is inherently positional.distinct_index_orderings_of_the_same_leaves_produce_distinct_rootsconfirms two trees over identical leaf content in different orders produce different roots — reordering isn't free.Batch size: 32, not the issue's literal 256
The issue's ≤256 bound was written against EVM gas economics (~50k gas/leaf × 256 ≈ 12.8M gas, comfortably under a ~30M gas block). Soroban meters CPU instructions and memory, not gas, and the cost model doesn't scale the same way:
A full 256-leaf batch is ~18× over budget. Cost isn't linear — it jumps a full tree level at each power-of-two boundary, so 32→64 nearly quadruples CPU cost and blows the budget by 22%. 32 is the largest power-of-two batch size whose worst case still lands inside a cheaper proof-depth tier — not an arbitrary "safe-looking" number; the next size up is categorically worse, not marginally worse. 33.8% CPU utilization leaves real headroom, not "barely under."
Two independent cost drivers, only one addressed here:
reveal_settlementwrites toenv.storage().instance()once per leaf, re-serializing a single growing ledger entry shared with the rest ofslashing_coreeach time. This roughly doubles the per-leaf cost and is a property of the existing storage architecture, not something this module introduced.This PR fixes (1) via the batch-size cap. (2) is out of scope — migrating bond debits to
persistent()storage or a batched write affects patterns used elsewhere in the slashing pipeline and deserves its own issue and review. Flagging for a maintainer decision.Caveat: CPU instructions measured running native Rust inside the test host are documented by the SDK as likely underestimates vs. the real WASM equivalent — these are a floor on cost, not a ceiling.
Hard bounds
depth_exactly_20_is_accepted_depth_21_is_rejectedcommit_settlement_accepts_exactly_max_batch_size/_rejects_invalid_batch_sizecommit_settlement(..., salt: &BytesN<32>)compute_commitment_hashDesign decisions
MIN_REVEAL_DELAY_SECONDS(10) /MAX_REVEAL_DELAY_SECONDS(50) approximate the issue's 2–10 block window in wall-clock terms, using Stellar's ~5s ledger close — this codebase has no block-count timing precedent anywhere, so seconds keep it consistent. Flagged in module docs as approximate. An unrevealed commitment is simply abandoned; no retry.Settleraddress can callcommit_settlementat all.SlashingDataKey::BondPoolkey the periodic monitor/executor pipeline uses, so the two subsystems can't silently diverge. Debits are capped at whatever remains, not reverted, if the pipeline already reduced the balance between commit and reveal.Merged with upstream/main
Eight upstream commits: attestation nonce-replay protection (#54), a mempool/fee priority-auction subsystem (#63 — source of the one conflict, in
src/lib.rs, resolved by keeping bothpub mod settlement;andpub mod mempool;), an unrelated fix insrc/slashing/(confirmed no overlap withBondPool/SlashingDataKey), and two housekeeping commits — "Remove all test files" and "Remove workflow rust.yml" — that auto-merged silently as ~104 deletions with no conflict.Those deletions are load-bearing:
slashing_core/slashing/mod.rsstill declarespub mod tests;after the test files were removed, soupstream/mainitself currently doesn't compile undercargo test, merged without CI running (since CI was deleted around the same time). Restored all 104 deleted files so this branch remains fully testable.Also fixed a pre-existing compile bug surfaced by the restore, in an unrelated new upstream file (
tests/attestation/proof_of_connectivity_epoch_nonce_test.rs, PR #94): aprop_assert_ne!call used f-string-style implicit variable capture, which that macro can't expand. 3-line fix to explicit positional args, no logic change — this file was merged without CI catching it.Flagging for the maintainer separately from this PR: the test-suite/CI-workflow removal pattern has now appeared on two repos in this campaign. It left
mainnon-compiling under test here, silently. Worth a direct look — nothing currently catches this class of regression before merge.Tests
Full unscoped
cargo test: 122 lib tests + all 23 integration binaries, 0 failures anywhere, including all 25 settlement tests. Weaken/restore re-verified a third time against the final merged state. Storage layout check passes, no collisions.Not fixed here — flagged for the maintainer
Migrating
BondPooldebits offinstance()storage would meaningfully raise the safe batch size above 32 and is worth its own issue.Feature Implemented. Task completed
@JamesEjembi Please Review