-
Notifications
You must be signed in to change notification settings - Fork 11
contracts: bound chklocks' sweep (epoch-stall risk) + materialize the per-bucket locked total #563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
2084159
262a599
548790a
0ff2bdc
6b31e07
f3e23c2
0902f94
55dd7e9
2dd6e1b
bc881dd
20e9ed6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,7 +64,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` | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P3] Include
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 0902f94. You were right, and I swept for the claim rather than fixing only this row, and there were four: the README's Comments and markdown only — no Fuller write-up: #563 (comment) |
||
| | `locks` | `lock_entry` | Flat per-leg lock vector consulted by `sysio.opreg::available()`. The `byexpire` secondary index lets `chklocks` sweep expired locks in one pass | | ||
| | `locks` | `lock_entry` | Flat per-leg lock vector consulted by `sysio.opreg::available()`. The `byexpire` secondary index lets `chklocks` sweep expired locks oldest-first, up to its per-epoch budget | | ||
| | `locksums` | `lock_sum` | Materialized Σ `lock_entry.amount` per `(underwriter, chain_code, token_code)` bucket — the "locked" half of `sysio.opreg::available()`, read O(1) instead of scanning `locks`. Written only by `try_select_winner` (on a win) and `chklocks` (on release); a bucket's row is erased once its total reaches zero, so an absent row reads as zero | | ||
| | `fwqueue` | `fromwire_q` | Escrowed swap-from-WIRE requests awaiting drain. `byepoch` secondary index | | ||
| | `uwcounters` | `uw_counters` | Monotonic id allocators (uwreq ids, lock ids) | | ||
|
|
||
|
|
@@ -77,7 +78,7 @@ chain deregistered) refund in full. | |
| | `rcrdcommit` | `sysio.msgch` | Record an underwriter's per-leg `UNDERWRITE_INTENT_COMMIT` bytes; resolves the race once both legs are present | | ||
| | `swapfromwire` | `user` | Escrow WIRE and enqueue a swap-FROM-WIRE request | | ||
| | `drainfwq` | `sysio.epoch` or self | Drain the from-WIRE queue: settle what prices, revert the rest (charging the revert fee on caller-fault causes) | | ||
| | `chklocks` | `sysio.epoch` or self | Sweep collateral locks whose wall-clock window has expired | | ||
| | `chklocks` | `sysio.epoch` or self | Sweep collateral locks whose wall-clock window has expired, oldest-first, at most `max_rows` per call (`advance` passes `MAX_LOCK_RELEASE_PER_EPOCH`); an oversized expiry burst drains across later epochs rather than aborting `advance` | | ||
| | `pruneuwreqs` | `sysio.epoch` or self | Expire timed-out PENDING uwreqs and erase terminal rows past their retention window | | ||
| | `sumlocks` | read-only | Sum an underwriter's active locks for a `(chain, token)` bucket — the lock half of `sysio.opreg::available()` | | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,28 @@ namespace sysio { | |
| // subsequent epochs. | ||
| static constexpr uint32_t MAX_UWREQ_PRUNE_PER_EPOCH = 32; | ||
|
|
||
| // Locks `sysio.epoch::advance` passes to `chklocks` each epoch. Sized | ||
| // like MAX_UWREQ_PRUNE_PER_EPOCH / MAX_FWQ_DRAIN_PER_EPOCH, and for the | ||
| // same reason — but the exposure here is sharper than either, because | ||
| // lock EXPIRY is inherently bursty. Every lock is stamped | ||
| // `now + collateral_lock_duration_ms` at creation, so a burst of | ||
| // settlements inside one epoch produces a burst of expiries inside one | ||
| // epoch, exactly one lock-duration later. Sustained swap traffic | ||
| // therefore presents `chklocks` with a whole epoch's settlements at | ||
| // once, and per expired lock the sweep does an inline | ||
| // `opreg::releaselock` dispatch plus an erase — all inside advance's | ||
| // hard, uncatchable transaction CPU deadline. | ||
| // | ||
| // Unbounded, a large enough expiry burst aborts `advance`; and because | ||
| // those same locks are still expired at the next advance, it aborts | ||
| // identically every epoch thereafter — a PERMANENT chain-wide epoch | ||
| // stall rather than a transient one. Bounded, an oversized burst is | ||
| // just release latency that drains across subsequent epochs, which is | ||
| // harmless: the challenge window has already closed, so a lock freed an | ||
| // epoch or two late costs only a brief overstatement of the | ||
| // underwriter's reserved collateral. | ||
| static constexpr uint32_t MAX_LOCK_RELEASE_PER_EPOCH = 32; | ||
|
|
||
| // ── UWREQ row-growth rails (SEC-129 / WSA-223) ───────────────────────── | ||
| // Every uwreqs `modify` re-serializes the whole row inside the | ||
| // never-throw evalcons / advance dispatch surfaces, so per-field byte | ||
|
|
@@ -346,8 +368,19 @@ namespace sysio { | |
| /// | ||
| /// This sweep is the ONLY lock-release path: locks are a wall-clock | ||
| /// challenge window (12h default) and are never released by delivery. | ||
| /// | ||
| /// Budget-bounded, mirroring `pruneuwreqs` / `drainfwq`: walks the | ||
| /// `byexpire` index in ascending `expires_at_ms` and releases at most | ||
| /// `max_rows` locks (`max_rows == 0` is a no-op). Inlined from | ||
| /// `sysio.epoch::advance` with `MAX_LOCK_RELEASE_PER_EPOCH`; also | ||
| /// invocable by `sysio.uwrit` itself with a caller-chosen budget for a | ||
| /// manual backlog drain. Ascending-expiry order makes the bound a FIFO | ||
| /// drain — the oldest locks always release first, so no lock can starve | ||
| /// behind a sustained burst. NEVER throws past the auth gate: it runs | ||
| /// inline inside `advance`, where an abort stalls epoch progress | ||
| /// chain-wide. | ||
| [[sysio::action]] | ||
| void chklocks(); | ||
| void chklocks(uint32_t max_rows); | ||
|
|
||
| /// Bounded UWREQ lifecycle sweep (SEC-129 / WSA-223). Inlined from | ||
| /// `sysio.epoch::advance` each epoch with `MAX_UWREQ_PRUNE_PER_EPOCH`; | ||
|
|
@@ -454,6 +487,30 @@ namespace sysio { | |
| /// a slash, the outpost routes seized collateral to that reserve via | ||
| /// `ReserveAmount`, even when multiple reserves exist for the same | ||
| /// `(chain_code, token_code)` pair. | ||
| /// The `(account, chain_code, token_code)` collateral-bucket digest: | ||
| /// the three uint64 identities packed little-endian into 24 bytes and | ||
| /// hashed. 3 × uint64 = 192 bits does not fit `uint128_t`, so the triple | ||
| /// is hashed to land in a `checksum256`. | ||
| /// | ||
| /// SINGLE SOURCE for that encoding, and it must stay that way. | ||
| /// `lock_entry::by_underwriter_ck()` says which bucket a lock row | ||
| /// belongs to; `lock_sum_key::primary_key()` addresses that bucket's | ||
| /// materialized total. If the two derivations ever diverged, the rollup | ||
| /// would be keyed differently from the rows it summarizes and every | ||
| /// reader would silently observe zero locked — collateral already | ||
| /// committed to a live lock would look spendable. Both call this, so | ||
| /// they cannot diverge. | ||
| static checksum256 compose_account_chain_token_ck(name account, | ||
| sysio::slug_name chain_code, | ||
| sysio::slug_name token_code) { | ||
| std::array<uint8_t, 24> buf{}; | ||
| uint64_t acc_v = account.value; | ||
| std::memcpy(buf.data() + 0, &acc_v, 8); | ||
| std::memcpy(buf.data() + 8, &chain_code.value, 8); | ||
| std::memcpy(buf.data() + 16, &token_code.value, 8); | ||
| return sysio::sha256(reinterpret_cast<const char*>(buf.data()), buf.size()); | ||
| } | ||
|
|
||
| struct lock_key { | ||
| uint64_t lock_id; | ||
| uint64_t primary_key() const { return lock_id; } | ||
|
|
@@ -476,17 +533,11 @@ namespace sysio { | |
| /// `byexpire` so `chklocks` sweeps expired locks in ascending order. | ||
| uint64_t expires_at_ms = 0; | ||
|
|
||
| /// Composite checksum index for opreg's `available()` rollup: | ||
| /// `sha256(underwriter.value || chain_code.value || token_code.value)` | ||
| /// packed as 24 little-endian bytes. 3 × uint64 = 192 bits doesn't | ||
| /// fit `uint128_t`, so we hash the triple to land in `checksum256`. | ||
| /// Which collateral bucket this lock belongs to — see | ||
| /// `compose_account_chain_token_ck`, the single source of that | ||
| /// encoding, shared with `lock_sum_key::primary_key()`. | ||
| checksum256 by_underwriter_ck() const { | ||
| std::array<uint8_t, 24> buf{}; | ||
| uint64_t uw_v = underwriter.value; | ||
| std::memcpy(buf.data() + 0, &uw_v, 8); | ||
| std::memcpy(buf.data() + 8, &chain_code.value, 8); | ||
| std::memcpy(buf.data() + 16, &token_code.value, 8); | ||
| return sysio::sha256(reinterpret_cast<const char*>(buf.data()), buf.size()); | ||
| return compose_account_chain_token_ck(underwriter, chain_code, token_code); | ||
| } | ||
| /// Split-index for cheap per-operator scans (plan §B.2). Callers | ||
| /// pull all rows for a given underwriter and filter on | ||
|
|
@@ -517,6 +568,57 @@ namespace sysio { | |
| sysio::const_mem_fun<lock_entry, uint64_t, &lock_entry::by_expires_at_ms>> | ||
| >; | ||
|
|
||
| /// Primary key of `locksums`: one (underwriter, chain_code, token_code) | ||
| /// collateral bucket, addressed by the SAME digest | ||
| /// `lock_entry::by_underwriter_ck()` uses to say which bucket a lock row | ||
| /// belongs to — both call `compose_account_chain_token_ck`. | ||
| struct lock_sum_key { | ||
| name underwriter; | ||
| sysio::slug_name chain_code; | ||
| sysio::slug_name token_code; | ||
| checksum256 primary_key() const { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Use one shared bucket-key helper.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — and it was worse than it looked: there were three copies of that derivation, not two. Alongside 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);
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. |
||
| return compose_account_chain_token_ck(underwriter, chain_code, token_code); | ||
| } | ||
| SYSLIB_SERIALIZE(lock_sum_key, (underwriter)(chain_code)(token_code)) | ||
| }; | ||
|
|
||
| /// 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P3] Include
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The I swept for the same claim rather than fixing only the block you flagged, and two neighbours were false for the same reason:
Both now say the only HEALTHY release path, and point at README: the PR description's "One writer" section updated to match. All comment-level in compiled code: |
||
| /// 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Update the rollup from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 0ff2bdc — You are right that it is a third erasure path and therefore carries the rollup obligation independently of Your description of the consequence is what makes this worth more than a missing line: the damage is permanent and silent.
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.
|
||
| /// erased when its total reaches zero, so an absent row reads as zero | ||
| /// and the table holds only live buckets. | ||
| /// | ||
| /// It exists because the derivation it replaces does not scale. Both | ||
| /// `sum_locks_inline` rollups (here and in sysio.opreg) previously | ||
| /// walked every lock row an underwriter held, each documenting the | ||
| /// assumption that "per-underwriter lock counts are O(1)-ish so the scan | ||
| /// is cheap". That is false under sustained swap traffic: 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 × lock duration) — unbounded within the window. The | ||
| /// scan ran per candidate inside `try_select_winner` (up to | ||
| /// MAX_UWREQ_CANDIDATES of them per uwreq), i.e. inside the same | ||
| /// consensus-dispatch CPU budget whose overrun stalls the chain. | ||
| /// | ||
| /// `sumlocks` reads this rollup, so it stays the cheap external answer | ||
| /// to "how much of this bucket is locked"; the authoritative recompute | ||
| /// is the `locks` table itself, which the contract tests scan and | ||
| /// compare against this total. | ||
| struct [[sysio::table("locksums")]] lock_sum { | ||
| name underwriter; | ||
| sysio::slug_name chain_code; | ||
| sysio::slug_name token_code; | ||
| uint64_t amount = 0; | ||
| SYSLIB_SERIALIZE(lock_sum, (underwriter)(chain_code)(token_code)(amount)) | ||
| }; | ||
|
|
||
| using locksums_t = sysio::kv::table<"locksums"_n, lock_sum_key, lock_sum>; | ||
|
|
||
| /// Per-underwriter race entry inside an UWREQ row. Tracks when each | ||
| /// leg of a dual-COMMIT pair arrived so `try_select_winner` can | ||
| /// resolve the race deterministically. Each leg's COMMIT is an | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] Keep this on the three-path invariant. This says “two sites,” but the rollup is mutated at three:
try_select_winneradds,chklocksdecrements, andsweeplocksdecrements after an upheld challenge. That third path was the source of the stale-cache bug fixed in this PR, so leaving the old count here undercuts the invariant documented inlock_sum. Please update this to say three sites and name them. The adjacentlocks_tcomment insysio.uwrit.hppalso still saysopreg::available()scansbyunderwriter; it should identifylocksas the authority andlocksumsas the O(1) read cache.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in bc881dd.
You are right on both, and the second one is the more useful catch: the
locks_tcomment was still describing the scan thatlocksumsexists to replace, so the file taught the old model at the point a reader is most likely to look it up.sum_locks_inline's comment now enumerates all three with direction, and states why the count matters at this site specifically — it is the cross-contract reader, it trusts the rollup completely, and a bucket left positive after its last row is gone suppresses that collateral permanently.locks_tnow nameslocksas the AUTHORITY andlocksumsas the O(1) read cacheavailable()reads, with the reason the old scan does not scale: locks are held for the full wall-clock challenge window and are never released by delivery, so a bucket's live count is (settlement rate × lock duration).I swept for the claim rather than fixing only the two sites you named, and three more had
available()reading the lock rows:lock_entrydoc, which called the triple the indexing surfaceavailable()uses and pointed cross-contract atlocks_trather thanlocksums_tlocksrow ("consulted bysysio.opreg::available()") and its integration note — both contradicting thelocksumsrow one line below, which already said the read is O(1) instead of a scanTwo adjacent bits of staleness turned up while there. The header bullet described the composite as one of "two secondary indexes",
byuwckandbyunderwriter— neither exists; the composite is deliberately not a table-managed index (thelocks_tcomment says so directly), and the uint64 indexes arebyuw,byuwreqandbyexpire. Andlock_entrysaid its rows are erased byrelease, an action this contract does not have — the erase sites arechklocksandsweeplocks, which is also where the twosub_locked_totalcalls live.Comments and markdown only.
sysio.uwrit.wasm,sysio.opreg.wasmand both.abifiles rebuild byte-identical, so this commit carries no artifact.