contracts: bound chklocks' sweep (epoch-stall risk) + materialize the per-bucket locked total - #563
contracts: bound chklocks' sweep (epoch-stall risk) + materialize the per-bucket locked total#563heifner wants to merge 8 commits into
Conversation
`sysio.epoch::advance` inlines four maintenance sweeps, and every one is budgeted against advance's hard, uncatchable transaction CPU deadline -- except `chklocks`, which walked every expired lock in one unbounded pass. MAX_FWQ_DRAIN_PER_EPOCH's own comment already names chklocks as sharing that ceiling; no budget was ever passed to it. The exposure is sharper here than for its siblings because lock expiry is bursty by construction: every lock is stamped `now + collateral_lock_duration_ms` when its race is won, so one epoch's settlements all fall due inside one epoch, exactly one lock-duration later. Each expired lock costs an inline `opreg::releaselock` dispatch plus an erase. A large enough burst aborts `advance` -- and because those locks are still expired at the next advance, it aborts identically every epoch thereafter: a permanent chain-wide epoch stall, not a transient one. chklocks now takes `max_rows` and walks `byexpire` ascending, so the bound is a FIFO drain and no lock starves behind a sustained burst. The remainder drains across later epochs, which is harmless: the challenge window has already closed by the time a lock is swept, so a late release costs only a brief overstatement of the underwriter's reserved collateral. advance passes MAX_LOCK_RELEASE_PER_EPOCH (32), matching MAX_UWREQ_PRUNE_PER_EPOCH and MAX_FWQ_DRAIN_PER_EPOCH. Tests: the two existing chklocks cases carry the new signature, a zero-budget no-op case joins its pruneuwreqs twin, and sysio.dispatch_tests drives a real won race to two live locks, ages past expiry without an advance, then asserts budget 0 releases nothing, budget 1 releases exactly one, and the remainder drains on the next sweep. 621/621 contracts_unit_test green. Change-Id: I721ac180d18ffb82739d0ac7caf14e84fff86031
`sysio.opreg::available()` = balance - locked - pending, and the `locked`
half was derived by WALKING every lock row the underwriter held --
`sum_locks_inline`, implemented twice (here and in sysio.opreg), each
documenting the assumption that "per-underwriter lock counts are O(1)-ish
so the scan is cheap".
That assumption does not hold. Locks are held for the full wall-clock
challenge window (`collateral_lock_duration_ms`, 12h default) and are
never released by delivery, so a bucket's live lock count is
(settlement rate x lock duration) -- unbounded within the window. And the
scan ran on the worst possible path: per candidate inside
`try_select_winner` (up to MAX_UWREQ_CANDIDATES per uwreq), several
uwreqs per dispatch transaction, inside the same consensus-dispatch CPU
budget whose overrun stalls the chain.
Adds a `locksums` KV table holding the materialized sum per
(underwriter, chain_code, token_code) bucket, keyed by a checksum256 over
the packed triple -- the SAME derivation as `lock_entry::by_underwriter_ck()`,
so the key that says which bucket a lock belongs to and the key that
addresses that bucket's total cannot diverge.
It has exactly one writer: the only two paths that can change a bucket's
total both live in sysio.uwrit -- `try_select_winner` (one lock per
required leg, on a win) and `chklocks` (release at expiry, the sole erase
path). A row is erased once its total reaches zero, so an absent row
reads as zero and the table holds only live buckets. Both
`sum_locks_inline` rollups become a single row read.
Overflow handling differs by direction, deliberately:
* increment SATURATES, matching the scan it replaces -- a wrap would
understate `reserved` and overstate availability, the one direction
that admits an overcommit. Saturating overstates `locked`, which
fails closed.
* decrement CLAMPS at zero -- a wrap would strand a colossal `locked`
on the bucket and zero that underwriter's `available()` for good,
and it runs inside `chklocks`, inline in advance, which must never
throw.
The cache is checked against its source rather than trusted: the
dispatch-test fixture gained `scan_lock_total()`, which recomputes a
bucket from the authoritative `locks` rows, and the lock-lifecycle test
asserts rollup == scan after the win, after a partial (budgeted) sweep,
and at zero once the bucket empties. Keeping the oracle in the test
avoids a second production derivation that could itself drift.
621/621 contracts_unit_test green.
Change-Id: Ib774bab79e3d57b47442fe570e170420ba608c8a
huangminghuang
left a comment
There was a problem hiding this comment.
Two inline findings from the lock-rollup review.
| auto idx = locks.template get_index<"byuw"_n>(); | ||
| uwrit::locksums_t sums(self); | ||
| uwrit::lock_sum_key pk{underwriter, chain_code, token_code}; | ||
| return sums.contains(pk) ? sums.get(pk).amount : 0; |
There was a problem hiding this comment.
[P1] Backfill the cache before switching readers. This upgrade creates locksums empty, but pre-existing unexpired rows in locks are never materialized. This reader then reports those buckets as zero, so available()/try_select_winner can reuse already locked collateral; a queued withdrawal can also drain it before the old lock expires. Add a deployment migration (or a transition that keeps scanning legacy locks until every live bucket is materialized) and an upgrade test seeded with pre-upgrade locks.
There was a problem hiding this comment.
Not needed here — there is no deployed chain, so there are no pre-existing locks rows to backfill. Every cluster this contract runs on is bootstrapped from genesis, and per the platform's standing pre-release rule nothing refactored before 2026-09-09 carries migration or back-compat machinery: there are no deployed clusters, no persisted artifacts in the field, and no external consumers pinned to the old shape, so upgrade-path code would be handling a state that cannot exist.
Worth saying that the mechanism you describe is exactly right, and it is the reason this is worth being explicit about rather than hand-waving: a reader that returns zero for a bucket which actually holds live locks makes committed collateral look spendable, and available() gates both try_select_winner and the withdraw queue — so it would be an overcommit, not just a stale number. If we ever do take a chain live across this change, that migration is mandatory, and the "keep scanning legacy locks until every live bucket is materialized" transition you sketch is the right shape for it.
Leaving as-is for now on the no-deployed-chain grounds. Happy to reopen if you disagree that pre-release applies here.
| name underwriter; | ||
| sysio::slug_name chain_code; | ||
| sysio::slug_name token_code; | ||
| checksum256 primary_key() const { |
There was a problem hiding this comment.
[P2] Use one shared bucket-key helper. lock_sum_key::primary_key() duplicates lock_entry::by_underwriter_ck() byte-for-byte, even though the rollup's correctness depends on both encodings remaining identical. Extract the 24-byte hash derivation into a shared helper and call it from both sites; otherwise a later change can silently separate lock rows from the cache bucket used by the readers.
There was a problem hiding this comment.
Fixed — and it was worse than it looked: there were three copies of that derivation, not two. Alongside lock_entry::by_underwriter_ck() and lock_sum_key::primary_key(), sysio.uwrit.cpp carried a private compose_account_chain_token_ck() with the same 24-byte packing and zero callers — vestigial from the v6 split-index change, kept "for any caller that still needs to derive the same key".
Rather than coin a new name I promoted that existing one into the header as the single source, since it is already this repo's name for the concept:
static checksum256 compose_account_chain_token_ck(name account,
sysio::slug_name chain_code,
sysio::slug_name token_code);by_underwriter_ck() and lock_sum_key::primary_key() are now one-line calls to it, and the dead .cpp copy is deleted. Its doc comment carries the consequence you identified, so the next person to touch it sees why it is shared: if the two derivations diverge, the rollup is keyed differently from the rows it summarizes and every reader silently observes zero locked — collateral committed to a live lock looks spendable.
Your framing is the better one and I have adopted it. My original comment said the encodings "must not diverge", which is a note asking a future reader to be careful; making them one function means they cannot. Contracts unit suite re-run green after the change.
Review follow-up on #563. The `(account, chain_code, token_code)` digest that says which collateral bucket a lock belongs to was written out three times: * `lock_entry::by_underwriter_ck()`, * `lock_sum_key::primary_key()` (added by the rollup commit), * a private `compose_account_chain_token_ck()` in sysio.uwrit.cpp with ZERO callers -- vestigial since the v6 split-index change, kept "for any caller that still needs to derive the same key". The rollup's correctness depends on the first two agreeing byte for byte: `by_underwriter_ck` identifies a lock's bucket and `primary_key` addresses that bucket's materialized total, so if they ever diverged the rollup would be keyed differently from the rows it summarizes and every reader would silently observe zero locked -- collateral committed to a live lock would look spendable to `available()`, which gates both `try_select_winner` and the withdraw queue. Promotes the existing `compose_account_chain_token_ck` name into the header as the single source of that encoding (the repo already names this concept; no new name coined), reduces both call sites to one-liners, and deletes the dead .cpp copy. A comment that ASKS a future reader to keep two encodings identical is replaced by one function that makes divergence impossible. Behaviour-preserving: the rebuilt sysio.uwrit.wasm and .abi are byte-identical to the previous commit's, so no artifact is restaged. 621/621 contracts_unit_test green. Change-Id: I49a2dfa9a44670098c1d9f9408506534c349fd1e
huangminghuang
left a comment
There was a problem hiding this comment.
Re-review after the underwriter-challenge work landed on master found two integration blockers that must be handled while resolving the current conflicts.
| std::vector<lock_entry> expired; | ||
| for (auto it = idx.begin(); | ||
| it != idx.end() && it->expires_at_ms <= now_ms; ++it) { | ||
| it != idx.end() && it->expires_at_ms <= now_ms && expired.size() < max_rows; |
There was a problem hiding this comment.
[P1] Count challenged locks against the epoch budget. Current master now skips expired locks whose challenge_id is non-zero and collects their challenges for chkuwchal. If this condition remains based on expired.size() during conflict resolution, held locks never increment the counter: an arbitrary number can still be scanned and every distinct challenge can fan out an inline crank, recreating the unbounded advance work this PR is meant to remove. Bound rows examined / challenge work as well as released locks, and add a test with more than the budget of held locks plus ordinary expired locks to prove both bounded execution and eventual progress.
There was a problem hiding this comment.
Fixed in 0ff2bdc — you called the conflict resolution exactly right, and it is now bounded on rows EXAMINED rather than locks released.
The naive merge was the one you predicted: master's continue for a held lock skips expired.push_back, so under expired.size() < max_rows a held row costs budget nothing. An unbounded run of them could be scanned and every distinct challenge among them could fan out its own inline chkuwchal — which is the unbounded advance work this branch exists to delete, reintroduced through the one path the bound did not cover.
One examined counter, incremented before the challenge check, bounds all three quantities at once: the scan, the release fan-out, and the poke fan-out (open_challenges.size() <= max_rows now follows from it rather than being hoped for).
chklocks_budget_counts_held_locks_and_still_drains covers both halves you asked for. Two HELD locks from a challenged commitment, two ordinary expired locks queued behind them, swept with a budget of 1:
- bounded execution — the budget is spent inside the held run, and nothing is released; in particular the ordinary locks behind it are never reached.
- eventual progress — the pokes the budget did afford lapse the challenge, and the same bounded sweep then drains the entire backlog over the following ticks, ending with both uwreqs COMPLETED and the rollup at zero.
That budget-smaller-than-the-held-run shape is deliberately the discriminator: I re-ran it against the release-counting form and it fails on the first assertion, because that version walks past both held rows and releases an ordinary lock in the same call.
On the starvation question underneath this — a held lock is skipped, not erased, so it stays at the head of the byexpire window and rows behind it wait while more than max_rows challenges are open. That is bounded and self-clearing rather than a stall: each sweep pokes the challenges it can see, and resolution either erases the locks (sweeplocks) or clears the hold (freelocks), letting the window advance. I documented that explicitly on the declaration so the next reader does not have to re-derive it.
| /// A CACHE of the `locks` table with exactly ONE writer: the only two | ||
| /// code paths that can change a bucket's total both live in this | ||
| /// contract — `try_select_winner` (one lock per required leg, on a win) | ||
| /// and `chklocks` (release at expiry, the sole erase path). A row is |
There was a problem hiding this comment.
[P1] Update the rollup from sweeplocks too. Current master adds a third lock-erasure path: an upheld challenge calls sweeplocks, which erases every held lock outside chklocks. After the rebase, each of those erases must call sub_locked_total; otherwise locksums remains positive after the authoritative rows are gone, sumlocks is permanently wrong, and a terminated operator can be considered settled/pruned while the stale cache blocks collateral on a later registration. Extend the challenge uphold test to assert rollup == scanned locks and zero after the sweep.
There was a problem hiding this comment.
Fixed in 0ff2bdc — sweeplocks now calls sub_locked_total on each erase.
You are right that it is a third erasure path and therefore carries the rollup obligation independently of chklocks. I checked the whole file rather than just this site: there are exactly two locks.erase calls (chklocks and sweeplocks) against two add_locked_total calls in winner selection, and both erase sites now decrement. freelocks and holdlocks only flip challenge_id, so they leave the rollup alone correctly.
Your description of the consequence is what makes this worth more than a missing line: the damage is permanent and silent. locksums is authoritative for available(), so a bucket left positive after its last row is gone suppresses that collateral forever — nothing ever decrements it again, because the rows that would have are already erased. A re-registering operator then finds deposited collateral unusable with nothing holding it, and sumlocks reports phantom locked value with no lock to point at.
chkuwchal_uphold_slashes_and_returns_bond now asserts both halves you asked for, right after the locks are confirmed gone: the rollup equals an authoritative scan of the lock rows, and it is zero. I verified it is a real regression rather than a passing assertion — with the sub_locked_total removed it fails scan_lock_total == get_lock_sum with [0 != 198], the scan reading zero while the cache still holds the swept 198.
I also left a comment at the erase naming it as the third path and stating the invariant, since the next erasure path added will have the same obligation and nothing structural enforces it.
contracts_unit_test: 638 cases, *** No errors detected
E2E gate: green — 13/13 flowsRun 31712204244 —
Branch combinationRecording it because a gate result only means something against the combination that produced it:
Why the full suite rather than one flow
Two caveats, stated plainly
|
…d-per-epoch-sweep Master landed the underwriter-fault challenge (WIRE-297) on the same `chklocks` sweep this branch bounds, which is both the conflict and — as huangminghuang flagged on the PR — two ways the naive resolution would have been wrong. **The budget now counts rows EXAMINED, not locks released.** Master skips an expired lock whose `challenge_id` is non-zero and collects its challenge for an inline `chkuwchal` poke. Merging that `continue` under this branch's `expired.size() < max_rows` would have left held rows uncounted: an unbounded run of them could be scanned, and every distinct challenge among them could fan out its own inline crank — exactly the unbounded `advance` work this branch exists to delete. A single `examined` counter bounds the scan, the release fan-out and the poke fan-out together (`open_challenges.size() <= max_rows` follows from it). **`sweeplocks` now decrements the rollup.** An UPHELD challenge erases locks outside `chklocks`, so it is a third erasure path and carries the same obligation as the other two: `locksums` is authoritative for `available()` and must never outlive the rows it summarizes. Without the `sub_locked_total` there, the bucket stays permanently positive after its last lock is gone — `sumlocks` reports phantom locked collateral forever, and a re-registering operator finds that collateral unusable with nothing holding it. There are exactly two `locks.erase` sites and both now call it. Tests — both verified as real regressions by reverting each fix and re-running: * `chklocks_budget_counts_held_locks_and_still_drains` — two HELD locks with two ordinary expired locks queued behind them, swept with a budget of 1. Bounded execution: the budget is spent inside the held run and nothing is released. Eventual progress: the pokes it did afford lapse the challenge, and the same bounded sweep then drains the whole backlog, ending with the rollup at zero. Against a release-counting budget it fails — the sweep walks past both held rows and releases an ordinary lock in the same call. * `chkuwchal_uphold_slashes_and_returns_bond` — extended to assert the rollup equals an authoritative scan and is zero once the sweep has taken every lock. Against the missing decrement it fails `[0 != 198]`. `sysio_uwchal_tester::chklocks()` now defaults to the budget `advance` passes (`kMaxLockReleasePerEpoch`) and delegates to `chklocks_direct`, so the existing challenge tests sweep exactly as production does; `make_confirmed_uwreq`'s uwreq tail is factored into `add_confirmed_uwreq` so a case can add a second commitment without re-bootstrapping. `sysio.opreg.wasm` moves because `sysio.opreg.cpp` includes `sysio.uwrit.hpp`, which this merge changes; its own source is unchanged from this branch. contracts_unit_test: 638 cases, *** No errors detected Change-Id: I99f2ba705bc3a7549fd81d01bda5b97797a84926
| /// Materialized Σ `lock_entry.amount` for one (underwriter, chain_code, | ||
| /// token_code) bucket — the "locked" half of `sysio.opreg::available()`. | ||
| /// | ||
| /// A CACHE of the `locks` table with exactly ONE writer: the only two |
There was a problem hiding this comment.
[P3] Include sweeplocks in the rollup invariant. After merging WIRE-297, sweeplocks is a second erase/decrement path outside chklocks, so the “only two paths” / “sole erase path” claim is now false—the exact omission that caused the previous rollup bug. Update this block, and the matching README/PR description, to list all three mutation sites: additions in try_select_winner, and decrements in chklocks and sweeplocks.
There was a problem hiding this comment.
Fixed in 6b31e07.
You are right, and the framing is the part worth keeping: that stale sentence is not merely inaccurate, it is what made the omission easy. The block asserted chklocks was the sole erase path, so sweeplocks could be added without anything prompting the author to check the rollup — and the previous commit is that bug.
The lock_sum block now enumerates all three with their direction — try_select_winner ADDS, chklocks DECREMENTS, sweeplocks DECREMENTS — and records the consequence rather than just the count, so the next erase path added meets an argument instead of a list: the rollup is authoritative for available(), so a bucket left positive after its last row is gone suppresses that collateral permanently, because the rows that would decrement it are already erased.
I swept for the same claim rather than fixing only the block you flagged, and two neighbours were false for the same reason:
- the header's
chklocksdoc, "This sweep is the ONLY lock-release path" - the
.cppsection banner, "chklocks — ... (the ONLY release path)"
Both now say the only HEALTHY release path, and point at sweeplocks as the other eraser.
README: the locksums row carries the same three-way enumeration. While there I found its actions table never gained holdlocks / freelocks / sweeplocks when WIRE-297 landed on master — so the row I was writing referenced an action the document did not define. Those three rows are added; that gap is pre-existing rather than something this PR introduced, but leaving it would have made the new text dangle.
PR description's "One writer" section updated to match.
All comment-level in compiled code: sysio.uwrit.wasm and sysio.opreg.wasm rebuild byte-identical, so this commit carries no artifact.
Review finding (huangminghuang, PR #563): after the WIRE-297 merge the rollup's own documentation still said `chklocks` was the sole erase path — the exact staleness that made `sweeplocks` easy to miss when it was added, and that produced the rollup bug fixed in the previous commit. The `lock_sum` block now enumerates all three, with their direction: `try_select_winner` ADDS, `chklocks` DECREMENTS, `sweeplocks` DECREMENTS. It also records why the omission mattered rather than just correcting the count: the rollup is authoritative for `available()`, so a bucket left positive after its last row is gone suppresses that collateral permanently, because the rows that would decrement it are already erased. A new erase path inherits the obligation and nothing structural enforces it. Two neighbouring claims were false for the same reason and are corrected: `chklocks` is described as the only HEALTHY release path (it is not the only path that erases) in both the header and the .cpp section banner. README: the `locksums` row carries the same three-way enumeration. Its actions table also never gained `holdlocks` / `freelocks` / `sweeplocks` when WIRE-297 landed on master, so a reader had no entry for the action this row now names — those three rows are added. The PR description's "One writer" section is updated to match. Comment-only in compiled code: `sysio.uwrit.wasm` and `sysio.opreg.wasm` rebuild byte-identical, so no artifact is committed here. Change-Id: I4ca138367d6fb7d0f424958ebd5e6bc92d2c126b
| | `pruneuwreqs` | `sysio.epoch` or self | Expire timed-out PENDING uwreqs and erase terminal rows past their retention window | | ||
| | `holdlocks` | `sysio.chalg` | Mark a commitment's winning locks as held by an OPEN underwriter-fault challenge (WIRE-297); held locks are skipped by `chklocks` instead of released | | ||
| | `freelocks` | `sysio.chalg` | Clear the hold after a REJECTED or LAPSED challenge, so the next `chklocks` releases the locks normally | | ||
| | `sweeplocks` | `sysio.chalg` | Erase a commitment's held locks after an UPHELD challenge — the underwriter is already SLASHED, so each `releaselock` takes its deferred-slash branch. Decrements `locksums` like `chklocks` does | |
There was a problem hiding this comment.
[P3] Qualify the early upheld-challenge sweep. This new row documents that sweeplocks erases and debits held locks after an UPHELD challenge, potentially before expires_at_ms, but the Responsibility summary above still says every lock is held for the full window and only chklocks sweeps it after expiry. Qualify that summary as the normal healthy path and mention the early slash-sweep so the two sections describe the same lifecycle.
There was a problem hiding this comment.
Fixed in f3e23c2. The Responsibility summary now marks the chklocks route as the healthy path and names the early sweeplocks erase alongside it, so it and the actions table describe the same lifecycle.
While there I found the chklocks row in that table stale in the same way and fixed it too: it read as a bound on locks released, which is what the budget counted before this branch made it challenge-aware. It counts rows EXAMINED now.
Fuller write-up: #563 (comment)
| // The discriminator is a budget SMALLER than the run of held locks, with an ordinary expired lock | ||
| // queued behind them: counting examined rows stops inside the held run and releases nothing; | ||
| // counting releases walks past every held row and reaches the ordinary lock in the same call. | ||
| BOOST_FIXTURE_TEST_CASE(chklocks_budget_counts_held_locks_and_still_drains, |
There was a problem hiding this comment.
[P3] Refresh the PR description with the current validation. This challenge-aware budget regression and the upheld-sweep rollup regression arrived with the master merge, bringing the contract suite to 638 cases, but the description still lists only the earlier budget test and reports 621. It also opens with “Two commits” although the PR now has five, and describes companion wire-libraries-ts#67 as +18/-1 although its current generated diff is +152/-2. Re-read the current diff and commit list and update the handoff so reviewers see the behavior, tests, and downstream scope they are actually approving.
There was a problem hiding this comment.
Done — description updated. The opener no longer claims two commits: it describes the two changes and says the rest of the branch is the master merge and the follow-ups it required. The Tests section now carries chklocks_budget_counts_held_locks_and_still_drains and the rollup half added to chkuwchal_uphold_slashes_and_returns_bond, each with the mutation that makes it a regression rather than a passing assertion. The companion is described at its current size, 152 insertions / 2 deletions.
One deviation from what you asked: I dropped the bare pass count rather than moving it from 621 to 638. It goes stale on every merge, which is how it came to be wrong in the first place, and what the tests actually cover is the part worth reading.
Fuller write-up: #563 (comment)
The Responsibility summary said a lock is held for the full wall-clock challenge window and that chklocks is what sweeps it once expires_at_ms passes. The actions table one screen below already documented sweeplocks erasing a commitment's held locks on an UPHELD challenge, which can land well before that, so the two halves of the same document described different lifecycles. The summary now marks the chklocks route as the healthy path and names the early erase alongside it. The chklocks row was stale in the same way: it read as a bound on locks released, which is what the budget counted before this branch made it challenge-aware. It counts rows EXAMINED now, which is what bounds the held-lock scan and the challenge pokes it fans out, so the row says so.
|
Both fixed in f3e23c2, and the description is updated. Qualify the early upheld-challenge sweep. Right, and the two halves of that document had drifted apart: the Responsibility summary described a lifecycle without the challenge path while the actions table one screen below already carried it. The summary now marks the While there I found the Refresh the PR description. Done on all three counts. The opener no longer claims two commits: it describes the two changes and says plainly that the rest of the branch is the master merge and the follow-ups it required. The Tests section now carries One deviation from what you asked: I dropped the bare pass count rather than moving it from 621 to 638. It goes stale on every merge, which is how it came to be wrong in the first place, and what the tests actually cover is the part worth reading. |
| @@ -64,7 +67,8 @@ chain deregistered) refund in full. | |||
| |-------|----------|-------------| | |||
| | `uwconfig` | `uw_config` | Singleton: `fee_bps`, `collateral_lock_duration_ms`, `min_fromwire_amount`, `fromwire_revert_fee_bps`, `uwreq_pending_timeout_epochs`, `uwreq_retention_epochs` | | |||
| | `uwreqs` | `uw_request_t` | One row per swap intent — race state in `commits_by`, `winner`, lifecycle status, mirrored `variance_tolerance_bps`. Retained for `uwreq_retention_epochs` after ANY terminal transition — `COMPLETED` (after `chklocks` sweeps the final collateral lock; the reserve settlement itself already happened at winner selection, which is what made the row CONFIRMED), `REJECTED` (immediate failure via `reject_and_refund`), or `EXPIRED` (pending timeout, same path) — then erased by `pruneuwreqs` | | |||
There was a problem hiding this comment.
[P3] Include sweeplocks in the COMPLETED transition. This row still defines COMPLETED only as the result of chklocks removing the final lock, but an UPHELD challenge calls sweeplocks, which erases the held locks and invokes the same finalize_settled_uwreqs tail before expiry. Update this row and the matching winner-selection comments in sysio.uwrit.cpp that still say locks are released only by chklocks, so all lifecycle documentation includes the early upheld-challenge path.
There was a problem hiding this comment.
Fixed in 0902f94. You were right, and finalize_settled_uwreqs was already saying so from the inside — its own doc reads "Shared by chklocks (natural expiry) and sweeplocks (an UPHELD underwriter challenge's slash-sweep)". The function knew it had two callers; the lifecycle documentation around it did not.
I swept for the claim rather than fixing only this row, and there were four: the README's uwreqs row, try_select_winner's doc block, the inline comment at the lock push, and pruneuwreqs' note on why CONFIRMED never reaches it. A fifth, the collateral_lock_duration_ms field doc, was silent on the window being cut short and now says so.
Comments and markdown only — no __LINE__ or __FILE__ anywhere in the contract, so codegen is untouched and the committed artifacts stand.
Fuller write-up: #563 (comment)
The COMPLETED transition was documented as what happens once chklocks takes the last collateral lock. sweeplocks takes it too, on an UPHELD challenge, and runs the same finalize_settled_uwreqs tail — that function's own doc comment already said as much — so a row can reach COMPLETED well before expires_at_ms by a route none of the lifecycle documentation mentioned. Four places said or implied chklocks was the only way out: the README's uwreqs row, both winner-selection comments where the lock is written, pruneuwreqs' note on why CONFIRMED never reaches it, and the lock-duration config field. All four name the early erase alongside the expiry sweep now. Comments and markdown only. The contract has no __LINE__ or __FILE__, so codegen is untouched and the committed artifacts stand.
|
Fixed in 0902f94. You are right, and I swept for the claim rather than fixing only the row you flagged, and there were four:
A fifth, the Worth noting what the run of these has in common, since this is the third round of it: each one was written when Comments and markdown only — no |
huangminghuang
left a comment
There was a problem hiding this comment.
Final re-review of head 0902f94: no actionable findings. Verified the challenge-aware examined-row budget, both locksums decrement paths, shared bucket-key derivation, regression coverage, generated ABI/artifact scope, and the completed lifecycle documentation follow-ups.
…d-per-epoch-sweep Change-Id: I779d54f169bc3d8df9857378d6b926661557b47a
Two changes, both consequences of the same underlying fact: uwrit locks are held for the full wall-clock challenge window (
collateral_lock_duration_ms) and are never released by delivery, so an underwriter's live lock set is (settlement rate × lock duration) — a quantity with no bound in either the sweep that releases it or the scan that reads it. The contract default is 12h; dev clusters shorten it, which changes the magnitude but not the shape. One commit fixes the resulting sweep cost, the other the read cost. They are separate commits so the rollup can be reverted without losing the stall fix. The rest of the branch is a merge of master and the review follow-ups it required — sharing the bucket-key derivation, making the sweep budget challenge-aware, decrementing the rollup fromsweeplocks, and the docs those changes invalidated.1.
contracts: bound chklocks' per-epoch lock releaseThe defect
sysio.epoch::advanceinlines four maintenance sweeps. Every one is budgeted against advance's hard, uncatchable transaction CPU deadline — exceptchklocks, which collected and released every expired lock in one unbounded pass.The codebase already knew the hazard.
MAX_FWQ_DRAIN_PER_EPOCH's own comment states it, and names chklocks as sharing the ceiling:chklocks was counted as a consumer of that ceiling but never given a budget.
Why it's the sharpest case, not the mildest
Lock expiry is bursty by construction: every lock is stamped
now + collateral_lock_duration_mswhen its race is won, so a burst of settlements inside one epoch produces a burst of expiries inside one epoch, exactly one lock-duration later. Per expired lock the sweep does an inlineopreg::releaselockplus an erase. Unbounded, a large enough burst abortsadvance— and those locks are still expired at the next advance, so it aborts identically every epoch thereafter. A permanent chain-wide epoch stall; a wedge, not a transient.The fix
chklocks(max_rows)walksbyexpireascending, mirroringpruneuwreqs/drainfwq: oldest-expiry-first so the bound is a FIFO drain and no lock starves,max_rows == 0is a no-op, andadvancepassesMAX_LOCK_RELEASE_PER_EPOCH(32). An oversized burst now costs bounded release latency — harmless, since the challenge window has already closed by the time a lock is swept.2.
contracts: materialize the per-bucket locked total in uwritThe defect
available()= balance − locked − pending, and thelockedhalf was derived by walking every lock row the underwriter held —sum_locks_inline, implemented twice (uwrit and opreg), each documenting the assumption that "per-underwriter lock counts are O(1)-ish so the scan is cheap".That assumption is false for the reason above. Worse, the scan ran on the worst possible path: per candidate inside
try_select_winner(up toMAX_UWREQ_CANDIDATESper uwreq), several uwreqs per dispatch transaction, inside the same consensus-dispatch CPU budget whose overrun stalls the chain.The fix
A
locksumsKV table holding the materialized sum per(underwriter, chain_code, token_code), keyed by achecksum256over the packed triple — the same derivation aslock_entry::by_underwriter_ck(), so the key identifying a lock's bucket and the key addressing that bucket's total cannot diverge.One writer. Every path that can change a bucket lives in uwrit. There are THREE:
try_select_winnerADDS (one lock per required leg, on a win),chklocksDECREMENTS (healthy release at expiry), andsweeplocksDECREMENTS (erasing a commitment's held locks on an UPHELD underwriter-fault challenge, WIRE-297). A row is erased at zero, so an absent row reads as zero.sweeplocksruns OUTSIDEchklocks, which is why it was missed on the first pass at the master merge — this section previously calledchklocksthe sole erase path, andsweeplockserased rows without decrementing. Getting it wrong is permanent and silent: the rollup is authoritative foravailable(), so a bucket left positive after its last row is gone suppresses that collateral forever, because the rows that would decrement it are already erased. Any new erase path inherits the obligation; nothing structural enforces it. Bothsum_locks_inlinerollups become one row read.opreg::has_active_locksis unchanged — existence-only, already O(1).Overflow direction differs by operation, deliberately:
reservedand overstate availability, the one direction admitting an overcommit; saturating overstateslocked, failing closed.lockedand zero that underwriter'savailable()permanently, insidechklocks, which runs inline inadvanceand must never throw.Checking the cache against its source
Rather than keeping a second production derivation that could itself drift, the dispatch-test fixture gained
scan_lock_total(), recomputing a bucket from the authoritativelocksrows. The lifecycle test asserts rollup == scan after the win, after a partial (budgeted) sweep, and at zero once the bucket empties.Tests
chklockscases carry the new signature;chklocks_zero_budget_is_noopjoins itspruneuwreqstwin.chklocks_budget_bounds_the_sweep_and_backlog_drainsdrives a real won race to two live locks, ages past expiry without an advance, then asserts budget 0 releases nothing, budget 1 releases exactly one, the remainder drains on the next sweep, the bond is left whole (releaselockis a no-op for a healthy operator), and the rollup tracks the scan at every stage.chklocks_budget_counts_held_locks_and_still_drainscovers the challenge-aware budget that arrived with the master merge: two HELD locks from a challenged commitment with two ordinary expired locks queued behind them, swept with a budget of 1. The budget is spent inside the held run so nothing is released and the ordinary locks are never reached; the pokes it affords lapse the challenge, and the same bounded sweep then drains the backlog over later ticks. Budget-smaller-than-the-held-run is the discriminator — the release-counting form of the bound fails its first assertion.chkuwchal_uphold_slashes_and_returns_bondgained the rollup half: once the swept locks are confirmed gone, the rollup must equal an authoritative scan of the lock rows, and be zero. Removing thesweeplocksdecrement fails it with the scan reading zero against a cache still holding the swept amount.Provenance
Found while investigating WIRE-340. That ticket's root cause is still open — a hypothesis I posted there (underwriter bond exhaustion) was refuted by direct measurement and I have retracted it. These two defects are independent of it:
chklocksbeing the only unbudgeted sweepadvanceinlines, andavailable()scanning the lock table per candidate, are both properties of the contract that hold regardless of what stalled those runs.Corrected from an earlier revision of this description, which claimed WIRE-340's ~50-minute runs "never reached the 12h expiry" so neither path had been exercised at volume. That was wrong: the harness overrides the window to 10 minutes (
ClusterBuildDefaults.ts:55,CollateralLockDurationMs = 600_000), so those runs swept locks roughly five times each. Both paths have run — and survived, because the bursts were small. Measured peak on those clusters was ~122 concurrent locks for the underwriter, so a sweep released on the order of a hundred rows and the per-candidate scan walked a similar count. That is comfortably inside the CPU budget, which is exactly why nobody has hit either problem.Both remain latent rather than theoretical: the quantity that matters is (settlement rate × lock duration), and neither the sweep nor the scan had a bound. At the 12h contract default, or at higher throughput, the same code paths grow without limit — the sweep into an
advanceabort that repeats every epoch, the scan into the consensus-dispatch CPU budget.Notes for review
ABI:
chklocksgainsmax_rows: uint32; uwrit gains thelocksumstable. Regenerated downstream in sdk-core: regenerate SysioContractTypes for the uwrit lock changes wire-libraries-ts#67 (generated output only, 152 insertions / 2 deletions).wire-tools-tsneeds no source change — nothing there callschklocksandlocksumsis additive; verified by building and running its gate against the regenerated sdk-core.(Correction to an earlier revision of this description: it claimed the checked-in
SysioContractTypes.tswas already out of sync with master's uwrit ABI, listingfreelocks/holdlocks/sweeplocks. That was wrong —origin/master's generated file matches master's ABI exactly. Those actions come from thefeat/underwriter-challengebranch, which is what I had checked out when I looked.)Artifacts: only contracts whose source changed are refreshed —
sysio.uwrit+sysio.epochin commit 1,sysio.uwrit+sysio.opregin commit 2.sysio.opreg.abiis untouched (no ABI surface change, internal reader only).