Skip to content

feat: add pull-based vesting pallet - #646

Open
n13 wants to merge 5 commits into
mainfrom
feat/vesting-pallet
Open

feat: add pull-based vesting pallet#646
n13 wants to merge 5 commits into
mainfrom
feat/vesting-pallet

Conversation

@n13

@n13 n13 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds pallet-vesting at runtime index 22 (spec_version 142), implementing a pull-based vesting wallet. The pallet-owned pot (PalletId(*b"qvesting")) holds the unclaimed allocation and pays beneficiaries through plain keep-alive transfers only when a payout is due.

No locks, freezes, or holds touch beneficiary accounts. Wormhole/keyless addresses can therefore be beneficiaries, and funds only move from the pot to the beneficiary once.

Design

  • Schedules use globally unique sequential u64 ids and contain {beneficiary, start, cliff, end, total, claimed, last_claim_at}. An account may hold any number of schedules.
  • Vesting is zero before cliff, linear from start to end, and exactly total at end. The calculation uses 256-bit rational arithmetic and floor rounding.
  • claim(schedule_id) is permissionless, but always pays the stored beneficiary rather than the caller. This supports keyless wormhole addresses and high-security accounts.

Payout safety

  • Runtime payouts are quantized to the Wormhole leaf quantum of 0.01 QUAN.
  • The runtime minimum payout is 1 QUAN. Integrity checks require it to exceed the existential deposit, be quantum-aligned, and contain at least two quanta.
  • Successful payouts on one schedule must be at least 24 hours apart.
  • A non-final claim reserves at least one complete minimum-sized final claim. If no valid non-final payout can avoid a sub-minimum remainder, the call fails with ClaimWouldLeaveDust until the full remainder is vested.
  • The final claim pays the complete remaining balance, so no schedule obligation is stranded.
  • Schedule totals must be at least the minimum payout and quantum-aligned.

Administration

Admin operations use EnsureTreasury (the configured treasury account, with Root as break-glass):

  • create_schedule validates the schedule, transfers total from treasury to the pot atomically, and requires the pot's existential-deposit buffer to exist.
  • end_schedule pays the quantized unpaid vested amount to the beneficiary and returns everything else to treasury. A non-zero beneficiary payout below the minimum is rejected without removing the schedule.
  • retarget_schedule first settles exactly the payout a permissionless claim could currently force to the old beneficiary, then changes the beneficiary. This makes the result independent of claim/retarget transaction ordering.

Integration

  • When genesis schedules are configured, presets endow the pot with exactly the sum of schedule totals plus its existential deposit. Empty schedule tables leave the pot unendowed; it must receive its ED buffer before the first dynamically created schedule.
  • Genesis validates every schedule and rejects mismatched pot funding. dev and heisenberg include example schedules, including multiple schedules for one account and a keyless test address.
  • Every beneficiary payout is recorded through the canonical Wormhole proof recorder, including Root/scheduler execution paths. The transaction extension skips pot-sourced transfer events to avoid duplicate leaves.
  • count_transfers pre-charges claim and create_schedule for one transfer and end_schedule for two transfers in its worst case.
  • All four calls were benchmarked against the runtime Wasm with 50 steps and 20 repeats. Generated base weights are wrapped with depth-aware ZK-tree database, proof-size, and Poseidon hashing costs.
  • Try-state checks schedule validity, quantum alignment, the no-dust remainder invariant, checked obligation accounting, and pot coverage of all outstanding obligations plus ED.

Verification

  • cargo test --locked -p pallet-vesting --features runtime-benchmarks — 60 passed.
  • cargo test --locked -p quantus-runtime --test mod governance::vesting — 6 passed.
  • cargo test --locked -p quantus-runtime --lib — 37 passed.
  • cargo test --locked -p quantus-runtime --test mod — 34 passed, 1 pre-existing test ignored.
  • cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime — passed.
  • cargo clippy --locked -p pallet-vesting --features runtime-benchmarks --no-deps -- -D warnings — passed.
  • cargo +nightly fmt --all -- --check and git diff --check — passed.
  • Runtime benchmarks for claim, create_schedule, end_schedule, and retarget_schedule — passed.

Notes

  • Genesis vesting allocations raise total issuance, reducing future block rewards by the allocation divided by EmissionDivisor.
  • If the treasury multisig enables high-security mode, admin calls are blocked by the high-security whitelist; Root through governance remains the break-glass path.
  • The mainnet genesis preset remains a follow-up PR.

Add pallet-vesting (runtime index 22): a pallet-owned pot endowed at
genesis holds the entire vesting allocation and pays beneficiaries by
plain keep-alive transfers at claim time. No locks, freezes, or holds
ever touch a beneficiary account, so wormhole addresses can be
beneficiaries and the cancel-and-repurchase double-spend of the earlier
lock-based draft is impossible by construction.

- Schedules keyed by sequential u64 ids (any number per account):
  {beneficiary, start, cliff, end, total, claimed} in wall-clock ms;
  linear vesting with cliff, 256-bit exact math, floor rounding with
  exactness at end.
- claim(schedule_id) is permissionless; the payout always goes to the
  stored beneficiary. This is the only claim path for keyless wormhole
  addresses and high-security accounts (claim is HS-whitelisted).
- Admin (treasury account via EnsureTreasury, Root as break-glass):
  create_schedule funds the pot from the treasury atomically,
  end_schedule pays unpaid vested to the beneficiary and returns the
  unvested remainder to the treasury, retarget_schedule recovers lost
  keys.
- Genesis presets endow the pot with sum(totals) + ED (ED buffer even
  with an empty table, as on planck) and keep the keyless pot out of
  the wormhole endowment list; genesis build panics on any mismatch.
- Wormhole proof-recorder extension statically pre-charges vesting
  payout transfers; claim payouts are recorded into the ZK tree like
  any other transfer.
- Benchmarked weights, 44 pallet tests, runtime preset build tests,
  and integration tests covering the real treasury-multisig admin flow.

spec_version 141 -> 142.

Note: the genesis pre-mine raises total issuance, reducing every future
block reward by sum(totals) / EmissionDivisor. The mainnet genesis
preset (4-of-6 treasury multisig) ships in a separate PR.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: REQUEST_CHANGES — three blocking correctness and deployment issues remain in the wormhole vesting path.

Findings:

  1. [P1] Quantize payouts before advancing claimed (pallets/vesting/src/lib.rs:247-256). Wormhole leaves commit amount / 10_000_000_000, while permissionless claim transfers every positive raw-planck owed amount and advances claimed by it. The included 10,000 UNIT/year schedule accrues only 3,805,175,038 planck per 12-second block: above the 1,000,000,000-planck ED, but below one wormhole quantum. A third party can therefore claim every block, move funds irreversibly into the keyless beneficiary, and create zero-value leaves that cannot recover those funds. Require totals and every beneficiary payout to be quantum-aligned, advance claimed only by the transferred representable amount, define early-end dust handling, and add a repeated-third-party-claim test against the actual leaf/exit amount.

  2. [P1] Record payouts made through the Root admin path (runtime/src/configs/mod.rs:571, pallets/vesting/src/lib.rs:322-336). Root governance calls are enacted by the scheduler outside any signed-extrinsic lifecycle, but vesting relies on WormholeProofRecorderExtension::post_dispatch to scan transfer events. A Root end_schedule can consequently pay a keyless wormhole beneficiary without inserting a leaf, leaving the vested payout unspendable. Move proof recording to a path that runs for every dispatch origin (without double-recording signed paths), and cover a scheduled Root end in an integration test.

  3. [P1] Initialize vesting state on runtime upgrade (runtime/src/lib.rs:269-270, pallets/vesting/src/lib.rs:288-291, pallets/vesting/src/lib.rs:442-445). Heisenberg and Planck are upgraded in place, so adding pallet index 22 does not rerun the edited genesis presets. The new pallet arrives with a zero-balance pot and no schedules: create_schedule returns PotUnderfunded, try_state rejects the missing ED buffer, and Heisenberg does not receive the advertised preset schedules. Add and test an upgrade migration, or provide an explicit reset/manual initialization path that establishes the same invariant before the pallet is used.

Validation:

  • cargo test --locked -p pallet-vesting --features runtime-benchmarks — 44 passed.
  • cargo test --locked -p quantus-runtime --test mod governance::vesting — 4 passed.
  • cargo test --locked -p quantus-runtime --lib wormhole_proof_recorder_counts_vesting_calls — passed.
  • SKIP_WASM_BUILD=1 cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime — passed.
  • cargo +nightly fmt --all -- --check and git diff --check — passed.
  • All GitHub checks, including both Linux and macOS build/test matrices, are green on a86b796afd8710a742780d5a720b714207a66c58.

The test coverage is substantial, but it does not exercise payout quantization, hook-dispatched Root payouts, or pre-existing-chain upgrade state; those gaps leave the issues above blocking.

Address review findings on the vesting pallet:

- Quantize payouts to the wormhole leaf quantum (SCALE_DOWN_FACTOR,
  10^10 planck): leaves commit amount/quantum, so a sub-quantum payout
  would be committed as a zero-value leaf and strand funds on a keyless
  beneficiary. Schedule totals must now be quantum-aligned, every payout
  is rounded down to a quantum multiple, and claimed advances only by
  the paid amount (stays aligned; the final claim at end remains exact).
  end_schedule sends sub-quantum vested dust to the treasury, which is
  signature-controlled and needs no leaf. Includes a regression test for
  the reviewed griefing scenario (per-block accrual above the ED but
  below one quantum).

- Record payouts through the canonical TransferProofRecorder inside the
  pallet: transfer and proof recording are fused into a single pay_out
  helper, so payouts create ZK-tree leaves on every dispatch origin —
  including Root calls enacted by the scheduler, which run outside the
  signed-extrinsic lifecycle and are invisible to the event-scanning
  extension. The extension now skips pot-touching transfer events (no
  double-recording on signed paths) and no longer statically counts
  vesting calls; the recording cost lives in the pallet's re-benchmarked
  weights, with the depth-dependent ZK-tree augmentation following the
  reversible-transfers pattern. An integration test drives end_schedule
  through the real scheduler as Root and asserts the payout leaf.

- No upgrade migration, by decision: this pallet ships on fresh chains
  whose genesis endows the pot. If it ever landed on a live chain in
  place, create_schedule fails loudly with PotUnderfunded until the
  treasury sends the pot its ED buffer — documented and covered by a
  bootstrap test.
@n13

n13 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed all three findings in 5c628a7:

1. Payout quantization (fixed). New PayoutQuantum config bound to the wormhole's SCALE_DOWN_FACTOR (10^10). Schedule totals must be quantum-aligned (validated at create_schedule and genesis), every payout is rounded down to a quantum multiple, and claimed advances only by the transferred amount — so it stays aligned and the final claim at end still drains exactly. end_schedule pays the beneficiary the quantized vested part and sends everything else (unvested remainder + sub-quantum vested dust) to the treasury, which is signature-controlled and needs no leaf. The reviewed griefing scenario is a regression test: per-block accrual above the ED but below one quantum now yields NothingToClaim instead of a zero-value-leaf payout, and a repeated-eager-claim test asserts every recorded leaf amount is quantum-aligned and nothing is ever stranded.

2. Root-path proof recording (fixed). Payouts now go through the canonical TransferProofRecorder (the same entry point the extension, mining-rewards, and reversible-transfers funnel into), invoked by the pallet itself: pay_out fuses transfer + record into one function, so no payout path can move funds without creating the leaf — on any dispatch origin, including scheduler-enacted Root calls. To avoid double-recording signed paths, the event-scanning extension now skips pot-touching Transfer events and no longer statically counts vesting calls; the recording cost moved into the pallet's re-benchmarked weights (claim 114µs, end_schedule 138µs base) with the depth-dependent ZK-tree augmentation following the reversible-transfers pattern from #639. New tests: an integration test schedules end_schedule as Root through the real scheduler (dispatched from on_initialize, no extension anywhere) and asserts the beneficiary's leaf exists, and an extension unit test asserts pot-sourced events are not recorded a second time.

3. Upgrade migration (resolved as not applicable, per deployment model). These chains launch fresh — genesis endows the pot and seeds the schedule table, so there is no in-place-upgrade path for this pallet and a migration would be dead code. The decision is documented on the storage version, and the unfunded-pot state is not a brick: create_schedule fails loudly with PotUnderfunded until the treasury sends the pot its ED buffer, covered by a bootstrap test (unfunded_pot_bootstrap).

Validation: 52 pallet tests (incl. benchmark suite), 33 runtime lib tests, 33 integration tests, build-spec --raw for dev/heisenberg/planck through the node, clippy/fmt clean. Weights regenerated with the recorder inside the calls.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: APPROVE (opinion) — I found no remaining blocking issues on 5c628a7d957b1765b4f964d51b35859d56f6c8ab.

The follow-up resolves all three findings from my previous review:

  • Claims and early termination now quantize beneficiary payouts to the wormhole leaf quantum, advance claimed only by the amount actually paid, return early-end dust safely to the treasury, and cover the repeated eager-claim scenario.
  • Vesting records pot-to-beneficiary proofs directly, so scheduler-enacted Root dispatches create leaves; the transaction extension excludes pot flows to avoid duplicate records, and the pallet weights account for the depth-dependent tree work.
  • The fresh-chain-only/no-migration policy and manual ED bootstrap for an unfunded pot are explicit, fail loudly, and have regression coverage.

Validation on the reviewed head:

  • cargo test --locked -p pallet-vesting --features runtime-benchmarks — 52 passed.
  • cargo test --locked -p quantus-runtime --test mod governance::vesting — 5 passed.
  • cargo test --locked -p quantus-runtime --lib wormhole_proof_recorder_ignores_vesting_calls_and_pot_events — passed.
  • SKIP_WASM_BUILD=1 cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime — passed.
  • cargo +nightly fmt --all -- --check and git diff --check — passed.

GitHub had not yet reported CI checks for this new head at review time.

Resolve genesis_config_presets conflict: keep vesting genesis schedules and
pot endowment; drop wormhole endowed_addresses (proofs now derive from
balances at block 1 per #648).

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Kimi Max Review — adversarial pass over the vesting pallet

Ran the full suites locally on feat/vesting-pallet (943e698): 48/48 pallet tests, 37/37 runtime lib tests (incl. all preset genesis builds + extension tests), 33/33 integration tests (incl. all 5 governance::vesting tests). Then tried to break it from every angle I could think of. One blocking finding, all in weights; the pallet logic itself held up.

Blocking

1. claim/end_schedule weights omit the depth-scaling Poseidon hash timepallets/vesting/src/weights.rs:40 (claim_weight) and :52 (end_schedule_weight).

The augmentation adds live-depth tree DB ops (insert_leaf_db_ops()) but not the hash compute that pallet-zk-tree explicitly says must accompany every leaf-insert pricing: "Anything that prices a leaf insert must charge this in addition to Self::insert_leaf_db_ops" (pallets/zk-tree/src/lib.rs:248-252, insert_leaf_hash_ref_time()). The sibling code this PR touches — WormholeProofRecorderExtension::per_transfer_weight (runtime/src/transaction_extensions.rs:137-146) — does include it, pinned by the per_transfer_weight_includes_tree_hash_compute regression test. The vesting weights reintroduce the same omission.

Magnitude: uncharged (min(depth+1, 32) + 2) × POSEIDON_EVAL_REF_TIME_PS (50 µs padded) per payout-recording call. At tree depth 10 that's ~600 µs undeclared against a 114 µs benchmarked base for claim — a >5x under-declaration that grows with the tree, eroding the block-weight DoS bound exactly as the tree gets big. The benchmarked base only captures hash time at benchmark-time (near-empty) depth, so it doesn't cover this.

Fix is mechanical:

// in claim_weight / end_schedule_weight, next to the db ops:
.saturating_add(Weight::from_parts(pallet_zk_tree::Pallet::<T>::insert_leaf_hash_ref_time(), 0))

(and the _at_depth(MAX_TREE_DEPTH) equivalent in the () impl). Note scripts/regenerate_weights.sh overwrites this file wholesale — the header comment warns about the augmentation, but it's worth double-checking after the next regen that both the DB-op and hash-time terms survive.

Non-blocking

2. Admin-call benchmarks measure the Root path, not the production pathpallets/vesting/src/benchmarking.rs:25-27. EitherOfDiverse::<EnsureRoot, EnsureTreasury>::try_successful_origin() resolves left-first to EnsureRootRoot (frame/support/src/traits/dispatch.rs:369, pallets/frame-system/src/lib.rs:1324), so create_schedule/end_schedule/retarget_schedule are benchmarked as Root. Production dispatches via the treasury multisig as Signed(treasury), which additionally runs EnsureTreasury::try_origin → one extra TreasuryAccount storage read (runtime/src/configs/mod.rs:559-568) that no benchmark captures. ~1 read under-declared per admin call. Marginal, but the benchmarked origin should be the more expensive of the two paths.

3. PoV term counts only tree readsweights.rs:45-46,57-58: tree_reads × TREE_KEY_POV, but insert_leaf also writes ~d+5 keys that enter the proof. Undercount ≈ (d+5) × 2600 B at depth d. Still strictly more careful than the extension (which charges no PoV here), so take it or leave it.

4. PR-body drift: the description says count_transfers "statically pre-charges claim/create_schedule (1) and end_schedule (2, worst case)" — the code deliberately charges 0 for all vesting calls since the pallet self-records (runtime/src/transaction_extensions.rs:207-211). Description-only; worth updating so the merged description matches the design.

5. Nit: genesis build early-returns on an empty schedule table (pallets/vesting/src/lib.rs:216-218), skipping the pot-endowment assertion — a hand-rolled spec with schedules = [] and an unfunded pot would build fine (fails loudly later at create_schedule with PotUnderfunded, which is documented). Shipped presets endow the ED unconditionally, so no live issue; just noting the assertion gap.

Attacked and found sound

  • Exactly-once proof recording on every dispatch path: signed claim (pallet records; extension skips pot-touching events), multisig-wrapped admin calls, scheduler-enacted Root end_schedule (no extension runs in hook context; pallet still records — integration-tested), create_schedule (no leaf for the keyless pot — correct). Since wormhole exits mint, a double-record would be unbacked mint capacity; I found no path that produces one, and no path that loses a payout leaf.
  • Quantization closure: totals, claimed, payouts, and refunds are all quantum-aligned by construction ⇒ final claim at end is exact, and every nonzero end_schedule refund is ≥ quantum. Since quantum = 10^10 > ED = 10^9, the BelowMinimum failure mode exercised in the mock is unreachable in production; end_schedule can't be permanently bricked by a dust payout.
  • Pot invariant pot ≥ Σ(total − claimed) + ED holds across all four calls; create_schedule rolls back atomically on treasury shortfall (tested); Preservation::Preserve keeps the ED buffer; beneficiary == pot and treasury == pot both blocked.
  • Vesting math: 256-bit rational, monotone, floor with exactness at end; no overflow at domain extremes (tested at u64::MAX times).
  • Miner time manipulation: closed — the local timestamp fork bounds future drift to 30 s (MAX_TIMESTAMP_DRIFT_MILLIS, pallets/timestamp/src/lib.rs:306-321).
  • HS whitelist: claim is safe — payout target fixed by storage, never the caller.
  • PalletId qvesting unique in the runtime; index 22 free; spec_version bumped 141→142; std/runtime-benchmarks/try-runtime feature wiring complete; RUNTIME_SURFACE.md matches the code (incl. vacant 10/12).
  • record_transfer_proof's bool is provably always true on these paths (amount is non-zero by the call-site guards, asset_id is None), so ignoring it is fine here.
  • retarget_schedule preserves claimed (documented); the lost-key race (old key pings claim before retarget lands) is inherent to the remedy, not a flaw.

Verdict: REQUEST_CHANGES (posted as a comment — GitHub won't let the PR author request changes on their own PR) — solely for finding 1. It's a two-line fix with an existing pattern and regression test to copy from the extension; everything else is polish. The pallet design itself is tight — the pull-based pot model closes the double-spend class by construction, and the test coverage (including the scheduler-enacted Root path) is genuinely thorough.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

CODEX Max Review

Verdict: REQUEST_CHANGES — follow-up/correction to my earlier exact-head review. Quantum alignment makes a payout representable in a leaf, but the real 4-bps circuit shows that one quantum is still not spendable. I found that blocker plus an unbounded fragmentation path, the previously noted depth-dependent weight omission, and a recovery-ordering race.

Blocking findings

  1. [P1] A one-quantum vesting leaf cannot produce any positive Wormhole output (pallets/vesting/src/lib.rs:276-305,360-390,448-457; runtime/src/configs/mod.rs:530-535,671-677).

    claim accepts every non-zero quantize_down(owed), end_schedule uses the same quantization, and schedule validation allows total == PayoutQuantum. With PayoutQuantum = SCALE_DOWN_FACTOR, such a leaf has circuit input_amount = 1. The pinned Wormhole circuit enforces:

    (output_1 + output_2) * 10000 <= input * (10000 - fee_bps)
    

    At the runtime's 4 bps, the smallest positive output would require 10000 <= 9996, which is false. The only valid total output is zero. Because claim is permissionless, a third party can force a payout exactly when one quantum has accrued, moving the beneficiary's funds from the pot into a leaf that cannot be exited under the current circuit. This recreates the stranding issue even though the leaf is non-zero.

    Require every emitted beneficiary leaf to be spendable, not merely representable. With a positive fee that means at least two quanta, and payout selection must also ensure the remaining obligation is either zero or independently spendable: e.g. paying two quanta from a three-quantum schedule leaves a final unusable quantum. Apply the same invariant to end_schedule, reject totals that cannot satisfy it, and test the real circuit fee inequality/exit rather than only the mock proof recorder.

  2. [P1] Permissionless claims allow an attacker to fragment a grant into an unbounded number of proof obligations (pallets/vesting/src/lib.rs:276-305,448-457; runtime/src/genesis_config_presets.rs:70-78; pallets/wormhole/build.rs:39-52).

    There is no minimum economic tranche, cadence, or maximum claim count. A valid no-cliff grant matching the example 10_000 * UNIT total contains 1,000,000 payout quanta, and the one-year duration has enough blocks to emit them separately. At the compiled defaults (NUM_LEAF_PROOFS = 7, NUM_PRIVATE_BATCH_PROOFS = 53), fully processing that fragmentation means roughly 1,000,000 leaf proofs, 142,858 private batches, and 2,696 public batches. The caller pays transaction fees but imposes the proof workload and note management on the beneficiary; a block producer can also self-include the griefing claims.

    Raising the spendability floor to two quanta alone still permits 500,000 leaves. Add a distinct bound such as MaxClaims, a minimum economic tranche/claim cadence, or a total-relative tranche size, and assert the worst-case leaf count in tests.

  3. [P1] claim and end_schedule omit the required depth-scaling Poseidon compute charge (pallets/vesting/src/weights.rs:36-60,69-72,88-90,102-120; pallets/zk-tree/src/lib.rs:84-100,242-255).

    Vesting adds insert_leaf_db_ops() but not insert_leaf_hash_ref_time(), despite pallet-zk-tree explicitly requiring both for every leaf insertion. At maximum depth the omitted term is (32 + 2) * 50_000_000 = 1_700_000_000 ps per payout-recording call. The transaction extension deliberately excludes these pot flows, so no other layer charges it. Permissionless underweighted claims can erode the block execution bound as the tree deepens.

    Add the live-depth hash ref_time to both runtime weights and the max-depth equivalent to the () implementation, then pin depth growth with a zero-DB-weight regression test. The admin weights also need the worst valid origin: AdminOrigin::try_successful_origin() selects Root first (benchmarking.rs:25-27), while the production signed-treasury path reads TreasuryAccount in EnsureTreasury (runtime/src/configs/mod.rs:553-568). That extra read is absent from the generated admin-call weights, including retarget_schedule's one-read total.

  4. [P2] A public retarget can be front-run by permissionless claim, defeating lost-key recovery for already-vested unpaid value (pallets/vesting/src/lib.rs:276-305,393-415).

    Retarget is documented as the remedy for a lost key, but it changes only the stored beneficiary and preserves claimed. Once a treasury multisig retarget is visible in the proposal/mempool, anyone can call claim first. That pays all currently vested value to the old lost or compromised account and advances claimed; the later retarget only protects the remainder. The pinger does not receive the funds, but can irreversibly choose the race outcome for the beneficiary.

    Define and enforce deterministic recovery semantics. If all unpaid value is recoverable, retarget needs ordering/pause protection. If vested value belongs irrevocably to the old account, retarget should settle that entitlement itself so an unrelated caller cannot decide the outcome, and the limitation should be explicit and tested.

Validation

  • cargo test --locked -p pallet-vesting --features runtime-benchmarks — 52 passed.
  • cargo test --locked -p quantus-runtime --test mod governance::vesting — 5 passed.
  • cargo test --locked -p quantus-runtime --lib wormhole_proof_recorder_ignores_vesting_calls_and_pot_events — passed.
  • cargo test --locked -p quantus-runtime --lib genesis_config_presets — 4 passed.
  • SKIP_WASM_BUILD=1 cargo check --locked -p quantus-runtime --features runtime-benchmarks,try-runtime — passed.
  • cargo +nightly fmt --all -- --check and git diff --check — passed.
  • All GitHub checks are green on 943e69835a526149fa9013fa1b7cfe3064970b09.

I rechecked dispatch atomicity, proof-recording paths, origins, vesting arithmetic, pot accounting, genesis presets, and the accepted fresh-chain/no-migration policy. Those areas held up; the findings above are cross-pallet/circuit invariants not exercised by the green tests.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Round 2 review — xhigh adversarial pass at df8a0cc0

Method: 10 independent finder angles over the full diff + enclosing code, 1-vote verification per candidate (mechanicals re-checked by hand against vendored FRAME sources), then a gap sweep. Round-1 findings re-checked: the Poseidon hash-time augmentation is in (weights.rs:28-29), benchmarks now dispatch signed-treasury (benchmarking.rs:28-33), PoV covers tree writes — all fixed. New findings below, ranked. Local test run at this head: 56/56 pallet tests pass. New findings below, ranked.

[
  {
    "file": "pallets/vesting/src/lib.rs",
    "line": 436,
    "summary": "end_schedule refunds to the treasury without the `treasury != pot` guard create_schedule has (lib.rs:370), so a treasury misconfigured as the vesting pot makes the refund a silent self-transfer no-op while ScheduleEnded reports it as returned.",
    "failure_scenario": "pallet_treasury accepts any non-zero account (pallets/treasury/src/lib.rs:163, genesis too). Treasury set to the vesting pot; admin ends a mid-vesting schedule: pay_out sends the vested part to the beneficiary and records the leaf, then `transfer(&pot, &treasury=pot, remainder, Preserve)` hits the `source == dest` early-return Ok no-op (frame/support/src/traits/tokens/fungible/regular.rs:330-334) — no funds move, no Transfer event — the schedule is then deleted, `unvested_returned` misreports the remainder as treasury-bound, and the funds sit stranded on the keyless pot (recoverable only by Root). do_try_state passes (pot over-covered), so nothing trips. Fix: apply the same guard in end_schedule."
  },
  {
    "file": "pallets/vesting/src/lib.rs",
    "line": 640,
    "summary": "do_try_state requires pot >= ED even with zero schedules, so the pallet's own documented 'unfunded pot is a supported state' fails every try-state check.",
    "failure_scenario": "planck/heisenberg are live chains running this runtime; spec 141->142 adds the pallet in place, genesis never re-runs, so Schedules is empty and the pot balance is 0. lib.rs:96-103 explicitly blesses that state ('fails loudly ... until the treasury sends the pot its existential-deposit buffer') and genesis early-returns on empty tables (lib.rs:244), but do_try_state computes required = 0 + ED and errors 'pot does not cover outstanding obligations' on every try-runtime block check against the upgraded chain until someone endows the pot. Fix: require the ED buffer only when schedules exist (or drop the 'supported unfunded pot' claim)."
  },
  {
    "file": "pallets/vesting/src/weights_generated.rs",
    "line": 31,
    "summary": "Committed weights were generated against a debug-built runtime wasm (--runtime=target/debug/wbuild/...) while scripts/regenerate_weights.sh targets the release wasm (line 6).",
    "failure_scenario": "The header's executed command (release quantus-node + target/debug wasm, WASM-EXECUTION Compiled, CPU <UNKNOWN>) means all four base ref_times were measured on unoptimized wasm and are systematically inflated — vesting extrinsics are over-charged (safe direction: no block-time risk, but fees are overpriced and block capacity underused), and the committed artifact contradicts the repo's own regeneration procedure. Regenerate per the script."
  },
  {
    "file": "pallets/vesting/src/benchmarking.rs",
    "line": 71,
    "summary": "The claim benchmark measures the cheapest path: set_time(END) with last_claim_at None skips vested_amount's 256-bit rational mul/div, the max_non_final reserve arithmetic, and the rate-limit check.",
    "failure_scenario": "The common real-world claim is mid-vesting on a schedule with a prior claim: `now >= end` early-return never runs the multiply_by_rational_with_rounding, claim_plan runs the reserve branch and the Some(last) rate-limit branch — strictly more ref_time than the benchmarked worst case, so WeightInfo::claim() under-declares the common path. Fix: benchmark at set_time(END/2) with a prior claim, mirroring end_schedule/retarget_schedule."
  },
  {
    "file": "runtime/src/transaction_extensions.rs",
    "line": 183,
    "summary": "count_transfers statically charges 1 for plain Balances transfers INTO the vesting pot that the event scan deliberately skips (line 253), overcharging the documented manual bootstrap.",
    "failure_scenario": "Treasury sends the pot its ED buffer exactly as the pallet docs prescribe on an unfunded-pot chain: weight() charges TransferCount r/w + depth-scaled insert_leaf_db_ops + insert_leaf_hash_ref_time, the scan drops the event (to == pot), and the reconciliation never refunds — the sender pays for a leaf insert that never runs, and the overcharge grows with tree depth. Fix: check the dest against the pot in count_transfers (rare op, so acceptable to wontfix, but note it)."
  },
  {
    "file": "pallets/vesting/src/lib.rs",
    "line": 422,
    "summary": "PayoutBelowMinimum locks end_schedule for essentially the whole vesting period of a total == MinimumPayout schedule, conflating the anti-spam minimum with the quantum safety floor.",
    "failure_scenario": "Treasury creates a 1-year schedule with total = 1 UNIT (= MinimumPayout, smallest valid). Once >=1 quantum vests, 0 < vested_paid < MinimumPayout holds until end, so end_schedule fails for the entire middle of the schedule — the treasury's funds are locked with no admin recourse but waiting. A payout in [quantum, minimum) is leaf-safe (commits a non-zero leaf); the safety floor is PayoutQuantum, MinimumPayout is a claim-path anti-spam rule. Fix: guard with `>= PayoutQuantum` (or exempt the case vested_paid == remaining), keeping the minimum on claim only."
  },
  {
    "file": "docs/RUNTIME_SURFACE.md",
    "line": 182,
    "summary": "The canonical runtime-surface doc (a PR file) drifted from the head commit on five points.",
    "failure_scenario": "(a) Schedules layout omits last_claim_at — an indexer hand-decoding per this doc misdecodes every schedule; (b) line 187 says genesis validates total >= ED, code requires total >= MinimumPayout (= UNIT, 1000x ED) — an operator preparing the planned mainnet table per the doc gets a genesis panic; (c) line 186 says retarget 'changes the beneficiary key only' — it now settles the full claimable payout to the OLD beneficiary and emits vested_paid; (d) the claim bullet omits MinimumPayout, the final-claim reserve, and the 24h MinClaimInterval — a claim UI built on it promises payouts the chain rejects with ClaimTooSoon/ClaimWouldLeaveDust; (e) line 250 says the pot is kept 'out of the wormhole endowment list' while genesis_config_presets.rs's own comment says the pot gets a block-1 leaf (unspendable)."
  },
  {
    "file": "pallets/vesting/src/tests.rs",
    "line": 113,
    "summary": "assert_eq!(x, true) trips clippy::bool_assert_comparison, and pm-quality-check runs clippy --all-targets --all-features with -D warnings per crate.",
    "failure_scenario": "Reproduced locally: `cargo clippy -p pallet-vesting --all-targets --all-features` emits bool_assert_comparison at tests.rs:113. If the per-crate quality gate (.github/workflows/pm-quality-check.yml:113) runs for this new crate, the PR goes red. One-line fix: assert!(...)."
  },
  {
    "file": "pallets/vesting/src/mock.rs",
    "line": 40,
    "summary": "Mock's mutable `pub static` config (ExistentialDeposit, TreasuryAccount, PayoutQuantum, MinimumPayout, MinClaimInterval) and thread_local RECORDED_PROOFS are mutated by ~10 tests and never reset — latent cross-test leak on reused harness worker threads.",
    "failure_scenario": "Rust's harness reuses worker threads across tests; a `TreasuryAccount::set(None)` or `PayoutQuantum::set(3_000)` persists into the next test on the same worker unless that test sets it first, and proof-recording tests asserting exact `recorded()` vectors accumulate earlier entries — order-dependent flakes that pass today by scheduling luck. Fix: reset statics to defaults and clear RECORDED_PROOFS in new_test_ext."
  },
  {
    "file": "pallets/vesting/src/lib.rs",
    "line": 586,
    "summary": "No ceiling on payout size vs the ZK leaf's u32 quantized-amount clamp: hash_leaf saturates amount/10^10 at u32::MAX (pallets/zk-tree/src/tree.rs:148), so a single payout above ~42.95M UNIT records a leaf committing less than the transfer.",
    "failure_scenario": "A schedule with total > 42.95M UNIT (schedule_is_valid has no upper cap) vests fully; the final claim pays it in one transfer and records one leaf clamped at u32::MAX quanta; the excess over the clamp is backed by the real transfer but has no exitable leaf — stranded on a keyless beneficiary (conservative direction, no unbacked mint). Pre-existing tree limitation, but vesting's one-shot final payouts reach it sooner than everyday transfers. Cap total per schedule or document the ceiling."
  },
  {
    "file": "pallets/vesting/src/weights.rs",
    "line": 17,
    "summary": "Manual augmentation is brittle: BENCHMARK_TREE_READS=5/WRITES=4 are untethered from the generated file, saturating_sub clamps any mismatch silently, and the benchmark-depth PoV/hash embedded in the base are double-counted (overcharge direction).",
    "failure_scenario": "A zk-tree refactor changes the tree-op counts in a regenerated weights_generated.rs; payout_weight keeps subtracting 5r/4w and over/under-corrects with no compile error and no failing test (the only test checks monotonicity). Pin the constants to the generated file's storage table in a test, or benchmark with a pre-grown tree so no subtraction is needed."
  },
  {
    "file": "pallets/vesting/src/weights.rs",
    "line": 45,
    "summary": "Each payout weight evaluation reads ZkTree::Depth twice (insert_leaf_db_ops() and insert_leaf_hash_ref_time() each call Depth::<T>::get()).",
    "failure_scenario": "Every get_dispatch_info for claim/end_schedule/retarget_schedule performs two identical trie reads where one suffices: read d = Depth::<T>::get() once and call the _at_depth(d) variants (the '()' impl's own pattern)."
  },
  {
    "file": "runtime/src/transaction_extensions.rs",
    "line": 240,
    "summary": "Per-dispatch waste: the extension derives the pot account (Blake2b hash) on every successful extrinsic even with zero Transfer events; TreasuryAccount is read twice per signed admin call; retarget derives the pot twice.",
    "failure_scenario": "The overwhelming majority of extrinsics never emit a Balances::Transfer, yet pay the pot derivation in post_dispatch solely so the filter can skip pot events that never occur — derive lazily inside the Transfer arm. configs/mod.rs:561 reads treasury_account() in EnsureTreasury::try_origin, then lib.rs:364/415 re-read the same key (EitherOfDiverse::Success already carries the AccountId on the signed arm). lib.rs:458 and :465 each hash the pot id."
  },
  {
    "file": "pallets/vesting/src/lib.rs",
    "line": 338,
    "summary": "DRY cluster (user-level coding rule 'Duplicate code must be avoided at all costs'): leaf-insert pricing exists in 4 places; the quantum constant in 3; the claim/retarget settle block is copy-pasted; test helpers are triplicated.",
    "failure_scenario": "(a) Depth-aware leaf-insert pricing is re-composed in reversible-transfers weights, wormhole weights, the extension's per_transfer_weight, and now payout_weight — belongs as one helper in pallet-zk-tree (a cost-model change updated in 3 of 4 places silently misprices the 4th). (b) VestingPayoutQuantum anchors to pallet_wormhole::SCALE_DOWN_FACTOR while the actual leaf quantum is pallet_zk_tree::tree::AMOUNT_SCALE_DOWN_FACTOR — if they ever diverge, sub-quantum payouts strand funds; add at least a const assert tying them. (c) claim's payout block (lib.rs:338-346) and retarget's settle block (lib.rs:463-475) duplicate pay_out + claimed + last_claim_at. (d) MockProofRecorder is the 3rd copy (mining-rewards, reversible-transfers) instead of a shared std-gated helper in qp-wormhole; governance/vesting.rs:23's account() duplicates TestCommons::account_id (runtime/tests/common.rs:9); 86_400_000 appears 3x in the runtime crate."
  },
  {
    "file": "runtime/src/transaction_extensions.rs",
    "line": 253,
    "summary": "Design note: the pot-skip hardcodes a per-pallet exemption in runtime-wide transaction infrastructure; the next self-recording pallet must edit the extension too.",
    "failure_scenario": "A future pallet-grants with its own keyless pot copies the vesting pattern; forgetting to extend this skip yields double-recorded payouts (two exitable leaves per real transfer = unbacked mint capacity). Deeper fixes: a recorder-side registry the extension queries, or eventless `increase_balance` payouts (the mining-rewards pattern) that need no extension special case."
  }
]

Refuted along the way (checked, not bugs): retarget skipping settlement on TooSoon/WouldLeaveDust — matches the documented contract ("settle any payout a permissionless claim could force"; claim_plan is shared, so settle ⇔ claimable by construction); the use qp_wormhole::TransferProofRecorder import — used by the T::ProofRecorder::record_transfer_proof call syntax, cargo check clean; missing pot guards on TransferOnHold/ReserveRepatriated/Minted arms — no path can hold/reserve/mint on the keyless pot; pay_out ignoring the recorder's bool — false is unreachable (amount >= MinimumPayout > 0, native asset), an ensure! would be defense-in-depth only; end_schedule's arithmetic and refund alignment — refund is always a quantum multiple > ED, BelowMinimum unreachable; timestamp monotonicity; pot-solvency invariant across all four calls; exactly-once recording on signed/batch/multisig/scheduler-Root paths; mock/runtime/benchmark constant divergence — intentional, benchmarks derive from T.

Verdict: REQUEST CHANGES (posted as a comment — GitHub won't let the author request changes on their own PR). Round 1's blocker is properly fixed; this round's should-fix set is findings 1–4 (guard asymmetry, try-state vs documented unfunded-pot state, debug-wasm weights artifact, claim benchmark measuring the cheapest path), plus 7 (doc drift in a PR file) and 8 (clippy gate) as cheap hygiene. Findings 5, 6, 9–15 are non-blocking polish. The pallet core — vesting math, quantization closure, pot invariant, exactly-once recording, claim_plan — held up under everything I threw at it.

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