-
Notifications
You must be signed in to change notification settings - Fork 11
Contracts: credit claimable balances instead of pushing transfers on never-throw paths #558
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
base: master
Are you sure you want to change the base?
Changes from 10 commits
b8fd386
db8d48d
ddc7a5c
be11600
c453c0c
76bc036
6e17468
c546b32
8469473
264fe11
8aaa053
69e34b4
cbf48d6
562542b
9948a13
369b194
a0aceec
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 |
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| #pragma once | ||
| /** | ||
| * @file claimable.hpp | ||
| * @brief Pull-payment primitives for payouts that originate on never-throw paths. | ||
| * | ||
| * `sysio.token::transfer` calls `require_recipient(from)` and `require_recipient(to)`, and the | ||
| * chain executes notified receivers with no exception isolation (`apply_context::exec`). An | ||
| * assert inside a recipient's `on_notify("sysio.token::transfer")` handler therefore aborts the | ||
| * WHOLE transaction, including every parent inline action -- and a handler can equally burn CPU | ||
| * until the enclosing action blows its deadline. | ||
| * | ||
| * That makes a pushed transfer unusable on any never-throw path. `sysio.epoch::advance` and the | ||
| * `sysio.msgch::deliver -> evalcons -> dispatch` chain both pre-validate every `check()` they can | ||
| * reach so they cannot abort (`feedback_opp_handlers_never_throw.md`), but that discipline stops | ||
| * at the contract's own guards: once value is pushed to an account the protocol does not control, | ||
| * the counterparty decides whether the transaction commits. A single uncooperative recipient can | ||
| * stall epoch advancement chain-wide. | ||
| * | ||
| * The fix is to never push. A never-throw path credits a claimable balance and emits no transfer; | ||
| * the recipient later pulls it with an action carrying its own authority. A handler that aborts | ||
| * then blocks only its own claim. | ||
| * | ||
| * `sysio.dclaim` established this pattern (`onreward` credits `pending_claims`, `claim` pays out); | ||
| * these helpers generalize it so `sysio.system`, `sysio.reserv` and `sysio.opreg` share one | ||
| * audited implementation rather than three copies. | ||
| * | ||
| * ## Row contract | ||
| * | ||
| * Each contract declares its OWN `[[sysio::table]]`-attributed row and key, because the table name | ||
| * is baked into both the attribute and the `kv::table` template argument, and because a | ||
| * `[[sysio::table]]`-attributed struct cannot be shared into `sysio.system`'s translation unit | ||
| * without corrupting that contract's read-only-action return codegen (see the note on | ||
| * `sysio.reserv::rewards_bucket`). The helpers below are templated over the table instead, and | ||
| * require only that the row expose: | ||
| * | ||
| * * `uint64_t balance` -- required, the claimable amount in atomic WIRE units | ||
|
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] Keep this helper documentation token-generic.
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 9948a13. You are right — The three sites are now generic:
On the |
||
| * * `uint32_t expires_at_sec` -- optional; when present it is maintained by `credit` and | ||
| * makes the row eligible for `sweep_expired` | ||
| */ | ||
|
|
||
| #include <sysio/action.hpp> | ||
| #include <sysio/asset.hpp> | ||
| #include <sysio/check.hpp> | ||
| #include <sysio/name.hpp> | ||
|
|
||
| #include <sysio.opp.common/safe_ops.hpp> | ||
|
|
||
| #include <cstdint> | ||
| #include <string> | ||
| #include <type_traits> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| namespace sysio::opp::claimable { | ||
|
|
||
| /// Compile-time detection of the optional `expires_at_sec` member on a claimable row. Contracts | ||
| /// whose claimable set is bounded (a producer schedule, a registered operator set) omit the field | ||
| /// and opt out of expiry entirely; contracts crediting an unbounded, caller-influenced set of | ||
| /// accounts carry it so abandoned dust rows cannot accumulate system-paid RAM forever. | ||
| template<class Row, class = void> | ||
| struct has_expiry : std::false_type {}; | ||
|
|
||
| template<class Row> | ||
| struct has_expiry<Row, std::void_t<decltype(std::declval<Row&>().expires_at_sec)>> : std::true_type {}; | ||
|
|
||
| template<class Row> | ||
| inline constexpr bool has_expiry_v = has_expiry<Row>::value; | ||
|
|
||
| /// Saturating credit, capped at `safe::depot_amount_max` (2^62-1) rather than `UINT64_MAX`. | ||
| /// | ||
| /// The cap is deliberately the `sysio::asset` magnitude limit, not the integer limit: `pay_out` | ||
| /// carries the stored balance out as an `asset`, and `asset`'s constructor `check()`-aborts above | ||
| /// `max_amount`. Saturating at the integer limit here would merely move the abort from credit time | ||
| /// (on a never-throw path) to claim time, stranding the balance permanently. Capping at the asset | ||
| /// limit keeps the row payable end to end. The cap is unreachable for any real payout. | ||
| inline uint64_t add_capped(uint64_t balance, uint64_t amount) { | ||
| constexpr uint64_t cap = static_cast<uint64_t>(safe::depot_amount_max); | ||
| if (balance >= cap) return cap; | ||
| const uint64_t room = cap - balance; | ||
| return amount >= room ? cap : balance + amount; | ||
| } | ||
|
|
||
| /// Credit `amount` to a claimable row, creating it when absent and accumulating when present. | ||
| /// | ||
| /// Never throws: a zero amount is a silent no-op and the credit saturates rather than aborting, so | ||
| /// this is safe to call from `sysio.epoch::advance` and from OPP inbound dispatch handlers. | ||
| /// | ||
| /// @param tbl the contract's claimable kv table. | ||
| /// @param payer RAM payer for a newly created row. | ||
| /// @param key primary key for the recipient. | ||
| /// @param fresh prototype row used when the key is absent; the caller pre-fills the identifying | ||
| /// fields (`account`, ...) and this function sets `balance` (and `expires_at_sec`). | ||
| /// @param amount atomic WIRE units to credit. | ||
| /// @param expires_at_sec absolute expiry stamp, ignored unless the row carries the field. Passing | ||
| /// the refreshed expiry on every credit means an account with ongoing activity | ||
| /// never expires mid-stream. | ||
| template<class Table, class Key, class Row> | ||
| void credit(Table& tbl, sysio::name payer, const Key& key, Row fresh, uint64_t amount, | ||
| uint32_t expires_at_sec = 0) { | ||
| if (amount == 0) return; | ||
|
|
||
| fresh.balance = add_capped(0, amount); | ||
| if constexpr (has_expiry_v<Row>) { | ||
| fresh.expires_at_sec = expires_at_sec; | ||
| } | ||
|
|
||
| tbl.upsert(payer, key, fresh, [&](Row& r) { | ||
| r.balance = add_capped(r.balance, amount); | ||
| if constexpr (has_expiry_v<Row>) { | ||
| r.expires_at_sec = expires_at_sec; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /// Drain a claimable row and emit the single `sysio.token::transfer` that pays it out. | ||
| /// | ||
| /// This is the ONLY place a claimable balance becomes a transfer, and it is reached only from an | ||
| /// action carrying the claimant's own authority. A recipient whose notify handler aborts therefore | ||
| /// blocks nothing but its own claim. | ||
| /// | ||
| /// The row is erased BEFORE the transfer is queued. The transfer notifies `to`, whose handler may | ||
| /// re-enter the claim action; erasing first means the re-entry observes no row and cannot double | ||
| /// spend. (Same ordering rationale as the credit-before-transfer guard in `sysio.opreg::deposit`.) | ||
| /// | ||
| /// Unlike `credit`, this DOES `check()`-throw when there is nothing to claim -- correct here, | ||
| /// because the throw reaches only the claimant who asked for it. | ||
| /// | ||
| /// @return the amount paid out, in atomic WIRE units. | ||
| template<class Table, class Key> | ||
| uint64_t pay_out(Table& tbl, const Key& key, sysio::name self, sysio::name token_account, | ||
| sysio::name to, const sysio::symbol& sym, const std::string& memo, | ||
| const char* nothing_to_claim_msg) { | ||
| auto it = tbl.find(key); | ||
| sysio::check(it != tbl.end(), nothing_to_claim_msg); | ||
|
|
||
| const uint64_t amount = it->balance; | ||
| sysio::check(amount > 0, nothing_to_claim_msg); | ||
|
|
||
| tbl.erase(key); | ||
|
|
||
| sysio::action( | ||
| sysio::permission_level{self, "active"_n}, | ||
| token_account, "transfer"_n, | ||
| std::make_tuple(self, to, sysio::asset(static_cast<int64_t>(amount), sym), memo) | ||
| ).send(); | ||
|
|
||
| return amount; | ||
| } | ||
|
|
||
| /// Total of every outstanding claimable balance, saturating. | ||
| /// | ||
| /// Callers that gate spending against a live token balance MUST subtract this: the WIRE backing | ||
|
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] Finish making this generic helper documentation token- and lifetime-generic.
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. Removed in 369b194, rather than re-documented. You gave two options and the second is the honest one, because both defects in that docblock point the same way. It is also unused: nothing in An uncalled helper, documented for a token it does not assume and a lifetime guarantee nothing satisfies, is worth less than its absence. If a genuinely lifetime-bounded claimable table ever appears, the four-line loop is easier to reintroduce correctly than to keep accurate in the meantime. ABIs verified byte-identical for epoch / opreg / system / reserv after the removal. |
||
| /// unclaimed rows is already owed, and spending it would leave a later `pay_out` unpayable. O(n) | ||
| /// over the table, so it is intended for bounded claimable sets (a producer schedule, a registered | ||
| /// operator set), not for the unbounded ones. | ||
| template<class Table> | ||
| uint64_t total_outstanding(const Table& tbl) { | ||
| uint64_t total = 0; | ||
| for (auto it = tbl.begin(); it != tbl.end(); ++it) { | ||
| total = safe::add_sat_u64(total, it->balance); | ||
| } | ||
| return total; | ||
| } | ||
|
|
||
| /// Bounded sweep of rows past their expiry, returning the reclaimed total. | ||
| /// | ||
| /// Iterates the caller's expiry-ordered secondary index so the oldest rows are visited first and | ||
| /// the scan can stop at the first live row -- a bounded scan over the PRIMARY (account-ordered) | ||
| /// index would repeatedly re-walk the same low-key live rows and might never reach an expired one. | ||
| /// | ||
| /// Expired keys are collected first and erased afterwards, rather than erasing through the | ||
| /// secondary iterator mid-walk: mutating a kv secondary index while iterating it is the same | ||
| /// foot-gun `sysio.system::payepoch` avoids with its `to_reset` snapshot. | ||
| /// | ||
| /// Never throws, so it is safe to call from the credit path as an on-write retention contract (the | ||
| /// shape `sysio.opreg::prune_dellog` uses). | ||
| /// | ||
| /// @param tbl the contract's claimable kv table. | ||
| /// @param by_expiry secondary index ordered by `expires_at_sec`. | ||
| /// @param to_key maps a row to its primary key. | ||
| /// @param now_sec current wall-clock seconds. | ||
| /// @param max_rows hard bound on rows erased in one call, keeping the caller inside its CPU | ||
| /// deadline. | ||
| template<class Table, class Index, class ToKey> | ||
| uint64_t sweep_expired(Table& tbl, Index& by_expiry, ToKey&& to_key, uint32_t now_sec, | ||
| uint32_t max_rows) { | ||
| using Key = std::decay_t<decltype(to_key(*by_expiry.begin()))>; | ||
|
|
||
| std::vector<Key> doomed; | ||
| uint64_t reclaimed = 0; | ||
|
|
||
| for (auto it = by_expiry.begin(); it != by_expiry.end() && doomed.size() < max_rows; ++it) { | ||
| // A zero stamp means "never expires"; such rows sort first, so skip rather than stop. | ||
| if (it->expires_at_sec == 0) continue; | ||
| // Index is expiry-ordered: the first live row means every later row is live too. | ||
| if (it->expires_at_sec > now_sec) break; | ||
| reclaimed = safe::add_sat_u64(reclaimed, it->balance); | ||
| doomed.push_back(to_key(*it)); | ||
| } | ||
|
|
||
| for (const auto& k : doomed) { | ||
| tbl.erase(k); | ||
| } | ||
|
|
||
| return reclaimed; | ||
| } | ||
|
|
||
| } // namespace sysio::opp::claimable | ||
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.
[P1] Define a safe multi-contract rollout order. This call and the new pay-claim reader create two unsafe mixed-version states. If
sysio.epochis upgraded whilesysio.reservis still old,is_accountpasses but the old CDT dispatcher asserts on the unknownsweepclaimsaction, so everyadvanceaborts. Ifsysio.systemis upgraded before epoch, newpayepochstarts retaining WIRE inpayclaimswhile the old gate still counts that backing as spendable, so a later pay period can double-commit it and leave claims underfunded. Deploysysio.reservthensysio.epochthensysio.system, or batch/feature-gate the upgrades atomically; document and exercise that transition so release tooling cannot choose an unsafe order.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 264fe11 — the order is documented, and the write-up is new in this PR:
docs/contract-upgrade-order.md.Both hazards you name are real, and I verified the mechanism rather than taking it on faith: the CDT-generated dispatcher ends every contract's
applywithdefault: if (r != "sysio"_n.value) sysio_assert_code(false, 1);, so an inline action a stale callee does not implement asserts — and insideadvancethat is a chain-wide epoch stall, exactly as you describe.Two things came out of writing it up, and both change the shape of the answer.
The hazard is systemic, not specific to
sweepclaims.advanceinlines into six contracts —sysio.reserv(sweepclaims),sysio.uwrit(chklocks,pruneuwreqs,drainfwq),sysio.opreg(recorddel,termcheck,flushwtdw),sysio.chalg(slashop),sysio.msgch(queueout,buildenv),sysio(accrueepoch,payepoch) — plus everything those inline transitively.pruneuwreqs(SEC-129) carried the identical exposure when it landed. A guard on this one call fixes one edge of N; a deploy procedure fixes the class.sysiois the exception in that dispatcher, and it is the worse one. Theif (r != "sysio"_n.value)means an actionsysio.systemdoes not implement is silently IGNORED rather than asserted. So thesysio.epoch-new /sysio.system-old direction does not halt — it drops the effect. Anaccrueepochthat never runs looks exactly like a chain that is working. Table reads have no dispatcher at all, so a reader meeting a stale writer sees the field default; that direction is always silent.The doc therefore leads with your second remedy rather than an ordering rule: ship a release's system contracts in ONE
sysio.msigtransaction — everysetcode/setabicommits or fails together, so there is no mixed-version window to order. The staged fallback issysio.reserv→sysio.epoch→sysio.systemwith your two edges as the reasons,sysio.opreg/sysio.uwritfree to land anywhere (they gain no inlined action and no other contract reads their new tables), and the general rule for future changes: a contract gaining anadvance-inlined action deploys BEFOREsysio.epoch; a contract whose new state the gate must reserve deploys AFTER it. The downgrade direction is written down too, since it inherits both rules reversed.Every intermediate state of that order is safe, and I checked the one that looked risky rather than asserting it: a new
sysio.epochunder an oldsysio.systemreads apayclaimtotthat has no row yet — that read is contract-side KV, so it never consults the stale ABI, andget_or_defaultyields a zero reserve, which is the correct answer while nothing is credited.What I did not do is add a structural guard, and that was a decision rather than an oversight. The cheap version — gate the inline on
sysio.reserv'swireclaimsbeing non-empty, since only the new build writes that table — would make epoch-before-reserv safe automatically in any order. I left it out for three reasons: it closes 1 of the N edges above while the procedure closes all of them; it needs a layout-compatible mirror ofwire_claiminsidesysio.epoch(the same duplicationrewards_bucketalready carries, a drift surface with its own pinning test); and pre-release, every cluster is bootstrapped fresh from one build, so the mixed-version state is a launch-time concern rather than a live one. Say the word and I will add it.On "so release tooling cannot choose an unsafe order": there is no in-tree upgrade tool to constrain — the harness bootstraps fresh clusters and deploys the whole set in one pass — so the procedure is the artifact. And a genuine mixed-version test would need a stale wasm vendored into the tree, an artifact nobody can rebuild from source, which is exactly what this repo does not commit. The nearest in-tree evidence is the guard already there and the 124 cases that failed the first time the inline went out unguarded.