-
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 8 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] Keep the reclamation path reachable when this epoch is balance-blocked.
advancereturns on a failed emissions gate at lines 349-361, before this call. If a pay epoch isBALANCE_INSUFFICIENTwhile expiredwireclaimshold enough forfeited WIRE to cover the shortfall, every retry exits before transferring that WIRE back tosysio, so the gate remains stuck even though the contract already controls the needed funds.sweepclaimsonly accepts epoch or reserv authority, so an ordinary keeper cannot break the cycle and privileged intervention is required. Run the bounded maintenance reclaim before the economic gate, expose a safely capped permissionless trigger, or otherwise let gate-blocked retries execute it; add a regression where expired claims make a blocked epoch payable.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 8469473 — the sweep now runs BEFORE the emissions gate.
You are right, and it was self-inflicted: I had placed it with the other maintenance sweeps (
chklocks,pruneuwreqs), which all sit after the gate. That is fine for them because none of them can change the gate's verdict. This one can — what it reclaims lands in the very balance the gate measures — so putting it in the same block created a cycle where a BALANCE_INSUFFICIENT epoch returns before reclaiming the forfeited WIRE that would cover its own shortfall, and every retry repeats it. Your point about authority closes the escape hatch:sweepclaimstakes only epoch or reserv authority, so no ordinary keeper could break it either.It is maintenance, not economics, so ahead of the gate is where it belongs. Safe there for three reasons I checked rather than assumed: it is budget-bounded, it never throws past its auth check, and it touches no epoch state — so a blocked epoch that sweeps and still cannot pay is exactly as blocked as before, minus some expired rows. Nothing about the gate's own inputs is disturbed except the balance, which is the point.
I did not add the permissionless trigger you offered as an alternative. Running it before the gate makes the reclaim unconditional on every advance attempt, including the retries of a blocked epoch, so the cycle cannot form in the first place — a keeper-callable escape valve would be a second path to the same outcome. Happy to add one if you would rather have a manual lever independent of the epoch tick.
Worth noting for the record: the guard on this inline is load-bearing and was found the hard way. The first version was unguarded and failed 124 cases with
inline action's code account sysio.reserv does not exist— the emissions fixtures advance epochs without ever creating that account. Moving the call earlier does not change that; it is still wrapped inis_account(RESERV_ACCOUNT).I have NOT added the regression you asked for — "expired claims make a blocked epoch payable". It needs a fixture holding a balance-blocked epoch and expired
wireclaimswhose forfeited total covers exactly the shortfall, which is a more elaborate setup than the two sweep tests; the reordering itself is covered only in the sense that the full suite still passes. Say the word and I will build it.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.
Yes, please add that regression. This placement is load-bearing cross-contract control flow, while the current reserv tests call
sweepclaimsdirectly and never exercise the realadvancepath. Sinceaction.send()queues the inline action, the expected blocked-path behavior is: the first advance records the gate block and then executes the queued reclaim; the nextchkconsretry observes the increased treasury balance and advances. A test that leaves an expired claim covering the shortfall and proves the epoch advances without a manual sweep or top-up would pin the actual guarantee and catch either moving this call back below the gate or losing the retry.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.
Added in 264fe11 —
expired_wire_claims_unblock_a_balance_blocked_epoch, incontracts/tests/emissions_tests.cpp.It pins the guarantee in exactly the two-attempt shape you described:
refundwirecredit, thenproduce_block(fc::days(366))with no further credit, socredit_wire_claim's opportunistic sweep can never fire — the epoch inline is the only thing that can collect the row;period_emission - FORFEIT, i.e. short by precisely the forfeited claim;blocklog[1]recordsEMISSIONS_BLOCK_REASON_BALANCE_INSUFFICIENTwithattempted_emission == period_emissionandt5state.last_epoch_indexstill 0, and the queued reclaim runs anyway: the claim row is gone and the treasury is exactlyperiod_emission;sweepclaims, no top-up: the epoch advances to 1,blocklog[1]is pruned,last_epoch_indexis 1.I verified it is a real regression rather than assuming. Moving the inline back below the gate (where the other maintenance sweeps live) fails it at
0u == wire_claimable("lapseduser"_n)with[0 != 100000000000]— the blocked epoch returns without ever reclaiming, which is the cycle the placement exists to prevent. It would equally catch losing the retry, since the second advance is what asserts the epoch actually moves.Reaching the epoch machinery from a reserv-aware fixture turned out not to need new scaffolding:
deploy_reserv()already existed in the emissions fixture for the swap-fee fold-in test, andregreserveis legal there because the fixture is still inside the epoch-0 bootstrap window. The only addition is awire_claimable()reader forsysio.reserv::wireclaims.contracts_unit_test: 644 cases, *** No errors detected