Kv: defer singleton writes with cached_value

[heifner]

Summary

The classic singleton idiom caches state in a contract member and writes it back from the contract destructor. The generated dispatcher instantiates the contract as a temporary and destroys it as soon as the action returns, so that destructor issues a kv_set after every action -- including a pure query -- and the chain rejects any write inside a read-only transaction with cannot store a KV record when executing a readonly transaction. The failure surfaces only on the success path: an action that aborts through check() traps before the destructor runs and reports its own error instead, which makes the return path look like the culprit.

kv::cached_value wraps any singleton-shaped store, loads it at most once, serves every read from that cache, and writes back only when a mutating call actually ran. An action that only reads issues no kv_set and is therefore legal read-only by construction, rather than by remembering to guard each write site. kv::cached_global and sysio::cached_kv_singleton are the ready-made aliases over kv::global and kv_singleton; both stores gain a public value_type and a try_get.

API shape worth reviewing

The creating form is modify_or_create, not upsert. kv::table::upsert(payer, key, default_value, updater) stores default_value verbatim on the insert path and never invokes the updater there -- every existing call site passes a fully-populated default and treats the lambda as update-only. This one seeds from the default and always runs the callback. Two functions in sysio::kv sharing a name while disagreeing about the insert path get applied interchangeably sooner or later, and the mistake is silent: the row is written either way, just with different contents.

seed_if_absent() materializes a default into the cache without owing a write. That is what a contract constructor needs -- seeding through set() leaves every later action flushing a kv_set, which reintroduces exactly the read-only failure this class exists to prevent.

get() returns a reference where both raw stores return by value, so binding it aliases the live cache rather than taking a snapshot. remove() deliberately keeps the cached object alive so an outstanding reference cannot dangle.

Rejecting malformed rows

kv::global::try_get now rejects a stored row whose size does not match a fixed-serializable payload. kv_get fills min(buffer, stored) bytes but returns the full stored size, and the previous sz < 0 guard let a short row splice whatever the contract's linear memory happened to hold into the payload -- so the caller silently receives stale bytes where it expects stored data. Linear memory is deterministic, so this is not a divergence risk: every node builds the same wrong value. It is still worth trapping, because a size mismatch means the row was written by an incompatible version of the type, and a wrong value that reads as authoritative stored data is worse than an abort. This is now the sole load path for cached_value, and exists() routes through it where it used to be a bare kv_contains.

Other hardening

mutate() re-checks the pending op after the callback returns, so a remove() issued from inside a modify() lambda is no longer converted back into a store of the value the caller just retired. same_payer no longer overwrites a real payer recorded earlier in the same action: the deferred write coalesces every mutation into one kv_set, and the host rejects payer 0 when that kv_set creates the row, so a sequence that was legal uncached would abort.

remove() probes the store first. Both backing stores short-circuit an erase of an absent row, so without the probe whether an action issued a host write -- and therefore whether it was legal read-only -- depended on chain data rather than on the code.

abigen unwraps kv::cached_value to its Store while keeping the wrapper as the decl whose use marks the table as belonging to the contract. Without it, a payload lacking [[sysio::table]] declared through the cached alias silently lost its ABI table entry -- nothing fails at build time; it surfaces later as clio get table, SHiP and the generated SDK types no longer seeing the table. Verified by stripping the annotation from the test contract's payload and confirming the table still generates.

kv_singleton.hpp no longer includes contracts/sysio/system.hpp. Nothing in it, kv_multi_index.hpp or kv_table.hpp uses any symbol that header declares, and its is_feature_activated collides with the C-API one, which kept the scoped alias out of natively-run tests.

Coverage and a gap

Native unit tests drive cached_value over a mock that mirrors apply_context's KV semantics and counts every intrinsic, so "a read issued no write" is asserted directly rather than inferred. The mock now also enforces apply_context's payer-on-create and row-exists-on-erase asserts, and the CHECK_ASSERT cases assert store counters: the native tester implements sysio_assert with longjmp, so the handle's destructor never runs and those cases previously verified only the message text.

An integration suite drives a contract holding both cached types on a real chain -- a query succeeds inside a read-only transaction, a mutating action is still refused, and deferred writes persist and collapse repeated mutations into one stored result.

Gap worth knowing about: that integration suite does not run in CI. ENABLE_INTEGRATION_TESTS defaults OFF and nothing under .github/ sets it, so the on-chain coverage here is only ever exercised locally, and cached_kv_singleton has no automated coverage. Removing the vestigial include clears one obstacle to covering the scoped alias natively; what remains is that kv_singleton goes through kv_multi_index::find, which needs the kv_it_* intrinsics mocked. Wiring the suite into CI is a build-system change and belongs in its own PR.

Companion PR

Wire-Network/wire-sysio#562 moves sysio.system and trx_priority onto cached_global -- both carried the destructor-write defect this class removes -- and rebuilds the eight contract binaries whose generated code the try_get size check changes.

Merge order matters: a wire-sysio release tag rebuilds the system contracts against the latest PUBLISHED wire-cdt release, so this has to be released, not merely merged, before that tag is cut. Merging #562 first would leave sysio.system uncompilable on the next tag build.

[huangminghuang]

One P1 finding from the deferred-write review.

[huangminghuang]

set() bypasses the same_payer preservation that mutate() implements below. For example, set(initial, payer_a) followed by set(updated, same_payer) is legal with the uncached store (the first call creates and the second updates), but this cache coalesces the calls and flushes one create with payer 0, which the host rejects. Preserve an already-recorded nonzero payer when this call receives same_payer, and add this two-set() sequence to the regression tests.

[heifner]

Confirmed, and fixed at the root rather than in set() alone: there were exactly two writes to _payer and only mutate() guarded it, so the rule held on one write path and not the other.

8a8f39457 funnels both paths through a private record_payer() that ignores same_payer whenever a real payer is already recorded. _payer is now assigned in exactly one place, so a future write path cannot reintroduce the divergence by forgetting the rule. With no real payer recorded, 0 still reaches the host untouched, which keeps an update on its existing payer and fails a create -- matching the uncached behaviour in both cases.

Your sequence is covered from both directions. cached_same_payer_preserves_recorded_payer gains the two-set() case against the counting store, and cached_global_set_then_same_payer_set_creates_row drives the same sequence through the real kv::global and the mocked host. The second is the sharper of the two: the mock now enforces apply_context's must specify a valid account to pay for new record on the create branch, so the regression aborts inside the intrinsic rather than quietly recording the wrong payer. I checked that both cases actually bite by restoring the unguarded assignment before landing the fix -- the first fails, the second aborts.

[huangminghuang]

One P1 finding from the re-review.

[huangminghuang]

Calling remove() twice on a row that existed when the handle was opened loses the first pending erase: the first call sets _present = false and _pending = erase, then the second enters this branch and clears _pending to none. The destructor therefore leaves the stored row intact. remove() should be idempotent; retain a pending erase here and add a double-remove regression case.

[heifner]

Confirmed, and fixed in 9681b927e.

The absent branch was conflating two reasons for _present being false. A row that was never there is one; a row this handle erased earlier in the action is the other — and in the second case _present is false because of the erase that was just recorded, so clearing the debt there drops the only write that would have removed the stored row. The handle then reports the row gone for the rest of the action while storage keeps it.

It now cancels only when the pending op is not already an erase. A row that was never there can only owe a pending write, so cancelling stays correct on that path, and the erase path is idempotent: two removes owe the same single erase as one.

One case I deliberately left alone: set() on an absent row followed by remove() still owes an erase against a row that was never stored. Making the debt exact there means knowing whether the store held a row, which set() skips learning on purpose — it replaces the cache outright, so it never reads. Both backing stores already short-circuit an erase of an absent row (kv::global on kv_contains, kv_singleton on find), so that flush is a no-op rather than a spurious write. Say the word if you would rather pay the probe and have the debt be exact.

Covered from both directions. cached_remove_is_idempotent pins the counting store's call counts for a bare double remove and for one preceded by a pending write. cached_global_double_remove_erases_row drives the same sequence through the real kv::global and the mocked host, and asserts the row is actually gone from storage rather than merely reported gone by the handle — which is exactly what a dropped erase gets wrong. I checked both bite by restoring the unguarded clear before landing the fix; both fail, and nothing else in the suite moves.