fix: address non-blocking vesting review findings - #649
Open
n13 wants to merge 1 commit into
Open
Conversation
Follow-up to #646, covering the polish items left open by the review. - One shared cost model for ZK-tree leaf inserts. `insert_leaf_weight[_at_depth]` in pallet-zk-tree composes the depth-scaled DB ops, the Poseidon path hashing and the per-key PoV, from a single `Depth` read. Vesting, reversible-transfers and the proof-recorder extension now price inserts through it instead of re-composing the parts (each of which read `Depth` again). This also gives the extension the tree PoV it was omitting. - Pin the vesting weights augmentation: it subtracts hand-maintained benchmark tree-op counts from the generated base, and a zk-tree cost-model change could silently make that an under-charge. Tests assert the augmented weight covers the benchmarked base at every depth, and that the `()` impl bounds all of them. - Reset the vesting mock's `static` config and recorded proofs per test. The harness reuses worker threads, so a `PayoutQuantum::set` leaked into whichever test ran next on the same worker. - Extract `settle`: `claim` and `retarget_schedule` duplicated the payout, `claimed` and `last_claim_at` updates. - Derive the vesting pot lazily in the event scan (it costs a Blake2b hash on every extrinsic, and most emit no `Transfer` at all). - Const-assert the two properties that keep payouts exitable: the vesting quantum equals the tree's leaf amount scale factor, and max supply stays below the leaf's u32 amount ceiling. - Deduplicate `MAX_SUPPLY`, `MILLIS_PER_DAY` and the integration tests' account helper; document the accepted inbound-pot weight overcharge. spec_version 142 -> 143.
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.
Follow-up to #646. That PR merged with all blocking findings fixed; this one works through the non-blocking remainder. Each was evaluated on its own merits — several are deliberately not fixed, with the reasoning below.
Fixed
#12 + #14(a) — one shared cost model for ZK-tree leaf inserts. Depth-aware insert pricing was re-composed in four places (vesting weights, reversible-transfers weights, the proof-recorder extension, wormhole weights), and each composition called
insert_leaf_db_ops()andinsert_leaf_hash_ref_time()separately — twoDepthtrie reads per weight evaluation, on everyget_dispatch_info.pallet_zk_tree::insert_leaf_weight_at_depth(and thePallet::insert_leaf_weightlive-depth wrapper) now composes DB ops, Poseidon path hashing and per-key PoV from one depth read, and the call sites use it. A future change to the cost model reaches every caller instead of three of four.This also fixes an omission in the extension:
per_transfer_weightcharged the tree's DB ops and hashing but declared zero proof size, while vesting, reversible-transfers and wormhole all chargetree_reads × TREE_KEY_POV. Recorded transfers now declare the PoV their leaf insert actually consumes — pinned by a new assertion inper_transfer_weight_includes_tree_hash_compute. This is the one behavior-affecting change here: per-transfer PoV goes up, materially so at deep trees, which is the honest bound validators re-execute against.#11 — weights augmentation is no longer silently breakable.
payout_weightsubtracts hand-maintainedBENCHMARK_TREE_*counts out of the generated base and adds the live-depth insert back; a zk-tree change that made an insert cheaper than the benchmark-time ops would turn that into an under-charge with no compile error and no failing benchmark. Two tests now pin it: the augmented weight covers the benchmarked base at every reachable depth, and the depth-blind()impl bounds the live-depth impl at every depth. The constants also got the doc comment explaining where they come from.#9 — mock state leaks between tests.
ExistentialDeposit,TreasuryAccount,PayoutQuantum,MinimumPayout,MinClaimIntervalandRECORDED_PROOFSall live in thread-local storage, and the test harness reuses worker threads. A test doingPayoutQuantum::set(3_000)leaked into whichever test ran next on the same worker; the suite passed by scheduling luck. The ext builder now resets every one of them to a named default.new_test_extalso computes the pot endowment from the default const rather than the live static, so there is no read-before-reset window.#14(c) —
claimandretarget_scheduleduplicated the settle block (pay out, advanceclaimed, stamplast_claim_at). Extracted toPallet::settle, so the two paths cannot drift on what a payout does to the schedule.#13 (partial) — lazy pot derivation. The event scan derived the vesting pot account (a Blake2b hash) on every successful extrinsic, purely so the filter could skip pot events that the overwhelming majority of extrinsics never emit. Now derived on first
Transferevent.#10 + #14(b) — the two properties that keep payouts exitable are now machine-checked. Const assertions in the runtime: the vesting payout quantum (anchored to
pallet_wormhole::SCALE_DOWN_FACTOR) equalspallet_zk_tree::tree::AMOUNT_SCALE_DOWN_FACTOR, the value that actually decides whether a leaf is non-zero; andMAX_SUPPLYsits below the leaf'su32amount ceiling, which is why no single payout can hit the clamp (~42.9M QUAN clamp vs a 21M QUAN cap). No cap ontotalwas added — total issuance already bounds it, and now the compiler enforces that it keeps doing so.#14(d) (partial) — deduplication.
MAX_SUPPLYandMILLIS_PER_DAYare single constants inruntime/src/lib.rsinstead of magic numbers repeated acrossconfigsandgenesis_config_presets;governance/vesting.rsusesTestCommons::account_idinstead of its own copy.#5 — documented, not fixed (see below), with the rationale recorded next to
count_transfersand inRUNTIME_SURFACE.md.Deliberately not fixed
#6 —
total == MinimumPayoutschedules cannot be ended early. This is the design, not a defect. A 1-QUAN schedule vests to its beneficiary on its own schedule; the treasury waiting it out is the intended behavior for the smallest valid grant. Loosening the guard toPayoutQuantumwould letend_scheduleemit payouts thatclaimrejects, which is a worse inconsistency than the one it removes.#5 —
count_transfersovercharges plain transfers into the pot. Resolving the destination incount_transfersmeans aLookupon the hottest call in the runtime, to spare a handful of one-off ED-bootstrap transfers. The overcharge direction is conservative. Documented in place rather than fixed.#13 (partial) —
EnsureTreasuryre-readingtreasury_account(). The suggestion was to take the account fromEitherOfDiverse::Success, but that Success isEither<(), AccountId>: the Root arm carries no account, so the pallet still has to read the storage key. No saving available without dropping Root as break-glass admin.#14(d) (partial) —
MockProofRecordercopies. Four crates have one (multisig's is a no-op stub, mining-rewards and reversible-transfers share a four-accessor API, vesting's is tuple-based), over different account and balance types. Consolidating means a declarative macro inqp-wormholeplus dev-dependency wiring in four crates — a cross-cutting test-infrastructure refactor rather than a vesting follow-up. Worth doing; not here.#15 — the pot-skip hardcodes a per-pallet exemption in runtime-wide transaction infrastructure. Real design debt, but both suggested remedies are worse today: eventless
increase_balancecannot express a payout (the pot must be debited, not just the beneficiary credited), and a recorder-side registry is over-engineering for a single pallet. Left as the documented, commented special case.#7 and #8 were already fixed before merge in
4f584733.Verification
cargo test --workspace: 2166 passed, 0 failed — including 59 vesting pallet tests (63 withruntime-benchmarks), 37 runtime lib tests and 34 integration tests.cargo clippy --locked --workspaceclean.runtime-benchmarksandtry-runtimefeature builds check. Formatted withcargo +nightly fmtandtaplo.Weights were not regenerated: the generated benchmark file is untouched, and only the augmentation around it changed.
spec_version142 → 143, since runtime weights change.