System: defer global singleton writes so view actions run read-only - #562
Conversation
huangminghuang
left a comment
There was a problem hiding this comment.
Approved after review.
system_contract and trx_priority each cached their global row in a member and wrote it back from a destructor. The dispatcher destroys the contract as soon as the action returns, so every action issued a kv_set -- including the view actions that exist to be called through a read-only transaction, which the chain refuses with "cannot store a KV record when executing a readonly transaction". The report looked contradictory because viewnodedist reported its own "account is not a node owner" error correctly: check() traps before the destructor runs, so only the success path died. Both singletons move to kv::cached_global, which defers the write to the end of the action and performs it only when something actually mutated. The _gstate members and the destructors are gone; readers use _global.get() and mutators _global.modify(), and trx_priority stamps its update time through upsert() so the row is created on first write rather than on first read. payepoch accumulates the reclaimed unpaid-block count across the producer reset loop and applies it in a single modify, so the reset costs one deferred write regardless of producer count. Requires the kv_cached header from wire-cdt; a release build of the system contracts needs a wire-cdt release that carries it.
…affected contracts trx_priority follows the cached_value rename to modify_or_create, and its comment now states the property the first-ever addtrxp/deltrxp depends on: the blank default is only a seed, and the lambda runs on the create path too, so the stamp is the current time rather than slot zero. system_contract's constructor uses the new seed_if_absent instead of set(). On a chain where the global row had never been written, set() left every action owing a kv_set -- including the read-only view actions this change exists to enable -- so the comment claiming a reading action issues none was false there. Seeding leaves the handle clean and the defaults reach storage the first time an action genuinely mutates the global. Rebuilds every contract binary whose generated code the kv::global size check changes: chalg, dclaim, epoch, msgch, opreg, reserv, system and uwrit. Leaving them stale would put the tree in the state where CI tests one binary and a release tag builds another from the same source, since PR builds run with BUILD_SYSTEM_CONTRACTS=OFF and a tag build rebuilds from source. That is the state uwrit was already in for an unrelated reason -- its committed artifact came from a different CDT install than the rest of the tree, 27KB apart from what this toolchain produces -- and it is corrected here as well. test_contracts/sendinline is deliberately left alone: it references none of the changed headers, and its output differs across rebuilds at an identical size, so committing a fresh snapshot would be churn rather than a fix. Change-Id: Ib7a52fa553b26963f27e1ecbb5c0ff7f6a887ab4
26414ed to
082487e
Compare
huangminghuang
left a comment
There was a problem hiding this comment.
[P1] Commit the remaining rebuilt contract artifacts
I rebuilt this exact head against the wire-cdt#106 headers with the same CDT toolchain. The build reproduces the committed sysio.system.wasm and sysio.msgch.wasm byte-for-byte, but sysio.{chalg,dclaim,epoch,opreg,reserv,uwrit}.wasm all differ. Each rebuilt binary contains the new kv::global: stored value size guard; the committed binary does not.
Because PR CI runs with BUILD_SYSTEM_CONTRACTS=OFF, it is currently testing these stale tracked blobs, while a tag build will rebuild and ship different artifacts—the mismatch this PR description says it prevents. Please commit those six rebuilt WASMs before merging.
| /// With kv::cached_global the write happens only when an action actually mutated the global, so a | ||
| /// query issues none. | ||
| BOOST_FIXTURE_TEST_CASE( view_actions_execute_in_readonly_transaction, sysio_emissions_tester ) try { | ||
| auto trace = push_system_action_readonly( "viewemitcfg"_n, mvo() ); |
There was a problem hiding this comment.
[P2] Exercise the missing-row path
sysio_emissions_tester has already produced post-deploy blocks, so onblock has dirtied and persisted global before this call. The earlier constructor that used set(defaults) would therefore pass this test whenever the row exists, leaving the seed_if_absent() regression from the second commit uncovered.
Please add a fresh-deploy/no-post-deploy-block case that runs the read-only view while global is absent, verifies the query does not create the row, then runs a real mutator and confirms the seeded defaults plus mutation persist.
There was a problem hiding this comment.
You were right, and I confirmed it by measurement rather than argument: reverting the constructor to the pre-fix seeding leaves both existing read-only cases passing while the new one fails.
sysio_fresh_deploy_tester deploys sysio.system and stops, so no block has run against that code and the row has never been written. The case then walks the path you described -- a succeeding action that does not touch the global must not create the row, the read-only view runs with it still missing and must not create it either, and the first genuine mutation stores defaults plus that change. For "the seeded defaults are what the contract reads" I leaned on setram's increase-only check: a decrease is rejected at the 64GiB default, which a zero-initialized cache would accept.
One thing that shaped it -- base_tester::push_action calls produce_block() once the transaction lands, and the next onblock stamps last_pervote_bucket_fill and creates the row itself. Without a no-block push path the case silently measured nothing.
Fixed in ec2db0d. Note the constructor this covers is gone as of fbed694: seeding moved onto the singleton type in Wire-Network/wire-cdt#107.
e2e gate: GREEN — all 14 flowsRun 31842724389 · head Branch combination — only
Worth noting for this PR specifically: |
Only sysio.system and sysio.msgch were rebuilt on this branch; a rebase dropped the rest. Rebuilding this head against the wire-cdt#106 headers reproduces those two byte for byte and produces different binaries for chalg, dclaim, epoch, opreg, reserv and uwrit, each carrying the kv::global stored-value-size guard the committed one lacks. PR builds run with BUILD_SYSTEM_CONTRACTS=OFF and exercise the committed binaries, while a release tag rebuilds from source, so leaving those six stale is exactly the split this branch set out to close: CI testing one artifact and a tag shipping another, from identical source. A sweep of all seventeen contracts confirms these six are the complete set. Every other contract rebuilds byte-identical, and none of them reference kv::global. No ABI changed.
The constructor seeded the global with seed_if_absent(get_default_parameters()). Both halves cost something on every action: seeding reads the row, and get_default_parameters() is an ordinary argument, so its get_blockchain_parameters host call ran whether or not the action went on to touch the global. Only five functions do -- onblock, update_ranked_producers, setram, setparams and payepoch -- so roughly forty actions were paying for a singleton they never read. The defaults now ride on the type (Wire-Network/wire-cdt#107), which leaves the constructor body empty and defers everything to first use: an action that never touches the global neither reads the row nor computes the defaults. get_default_parameters moves out of system_contract as a free default_global_state, because the alias names it as a template argument and has to exist before the class is declared. sysio.system grows 4380 bytes, and that is the price of materializing lazily rather than in the constructor, not of the defaults themselves. With a trivial default it is 4196, and routing every access through a single materialization point recovers only 367. Seeding eagerly let the optimizer prove the cache loaded once and strip the checks everywhere downstream, which is precisely what deferring gives up. One behavioural note that comes with the type: on a handle carrying defaults, exists() reports that a value is available rather than that a row is stored. Nothing here asks that question, and the idiom it affects -- create the row if exists() is false -- is the one this change exists to retire.
view_actions_execute_in_readonly_transaction runs against a fixture that has produced post-deploy blocks, so onblock has already persisted the global. With the row present, every way of materializing defaults looks alike and the read-only query passes either way, which left the absent-row path uncovered: reverting the constructor to the pre-fix "create it if exists() is false" seeding leaves both existing read-only cases passing while the new one fails. sysio_fresh_deploy_tester deploys sysio.system and stops, so no block has been produced against that code and the row has never been written. The case then walks the path the change exists for -- an action that succeeds without touching the global must not bring the row into existence, a read-only query must run with the row still missing and must not create it either, the values served must be the declared defaults rather than a zero-initialized struct (a max_ram_size decrease is rejected at the 64GiB default), and the first genuine mutation must store the defaults plus that change. Ordering is load-bearing. Each action constructs its own system_contract and flushes on return, so an action that persisted the row early would mask everything after it. Every push therefore goes through push_system_action_no_block: base_tester::push_action produces a block once the transaction lands, and the next onblock stamps last_pervote_bucket_fill and creates the row itself.
|
Confirmed and fixed in 745df70. Rebuilding this head against the wire-cdt#106 headers reproduces I also swept all seventeen contracts to check for a seventh: there isn't one. Every other contract rebuilds byte-identical and none reference Two things worth flagging since they weren't in your review. Chasing this surfaced that the constructor was charging every action for the global -- it read the row and ran |
e2e gate: GREEN — all 14 flowsRun 31885409167 ·
This branch needs wire-cdt#107 — the gate cannot pass without itWorth stating explicitly, because nothing on this PR declares it. using global_state_singleton =
sysio::kv::cached_global< "global"_n, sysio_global_state, &default_global_state >;and the three-argument A first run without the CDT override (31884995426) died there in 7 minutes, before reaching any flow. wire-cdt#107 has to merge before this one, and any gate run in between needs Note also that this result pins wire-cdt#107 at its current head. Two review findings are open against it — |
…-view-actions Change-Id: I6ddae95caa5464de56bdd344c92f584c03a8cefb
No contract source changed. The binary moves because wire-cdt#107 merged (adad4b29) and cached_value is a header-only template embedded in system_contract: the committed artifact was built in 745df70 against that PR's FIRST commit, 878b88f4, and five further commits reworked kv_cached.hpp before it landed -- remove() keying on stored-row presence, removal tracked apart from the pending erase, the read/presence state folded into one enum, flush() refused mid-mutation with the default-taking calls constrained away, and the mutation mark moved onto the stack. Compiling each sysio.system translation unit against both CDT states, five of seven differ, so what is committed is not reproducible from this source and the CDT that will build it -- and CI deploys the checked-in artifact. Only sysio.system is affected. Rebuilding all seventeen contracts against the merged CDT reproduces sixteen byte-for-byte and moves this one alone (177,928 -> 178,263), which is the expected result given kv_cached.hpp is the only functional change between the two CDT states. Every ABI is unchanged, so nothing downstream needs regenerating. contracts_unit_test --sys-vm: 636 test cases, no errors, run against this exact binary. Change-Id: I6e3fa131abf13275ad70b765f272a8743e014d6c
e2e gate: GREEN — all 14 flowsRun 31910302404 · No Includes |
huangminghuang
left a comment
There was a problem hiding this comment.
[P1] Rebuild sysio.chalg and sysio.uwrit against the final merged CDT headers
On this exact head, a clean 17-contract sweep against wire-cdt master / PR #107 merge adad4b29 reproduces 15 committed WASMs byte-for-byte, including the new sysio.system.wasm, but two still differ:
sysio.chalg.wasm: committed 67,555 bytes /b0089c83…; rebuilt 67,565 /d36d39ed…sysio.uwrit.wasm: committed 157,214 bytes /2e3bc8c9…; rebuilt 157,236 /2889580d…
A second fresh configure/build reproduced both rebuilt hashes, and every ABI is unchanged. Since PR CI consumes the checked-in blobs while tag builds compile from source, these two still leave CI and release artifacts split. Please sync current master (the PR presently has binary conflicts), run the full contract rebuild from the combined tree, commit the resulting artifacts, and correct the claim that sixteen binaries reproduce.
…-view-actions Only the four contract wasms conflicted; every source hunk merged on its own. sysio.epoch, sysio.opreg, sysio.reserv and sysio.system were rebuilt from the merged sources rather than resolved to either side: master's copies of the first three predate wire-cdt#106's kv::global guard, and sysio.system combines this branch's cached_global handle with master's claimable-payout emissions work. Sweeping all seventeen contracts against wire-cdt master (adad4b290) reproduces the other thirteen byte for byte, chalg and uwrit included. No ABI changed.
|
Merged On I deleted the 27 GB CDT build directory, moved the worktree to wire-cdt master
So the +10 and +22 track the environment the CDT itself was compiled in rather than the CDT commit or the contract source. This tree has seen that effect before at far larger magnitude on identical source, and unlike a genuinely stale artifact it does not leave CI and a tag build compiled from different source. That said, if your toolchain is the one a release tag will use, I would rather match it than argue it: tell me which wire-cdt build produced yours -- local, or the CI container -- and I will rebuild against that instead. One correction in the other direction, since it is the same kind of claim. The description says |
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed the merged head and found no remaining issues. I rebuilt the top-level contracts_project using the official Linux CDT CI package from wire-cdt commit adad4b29; all 17 tracked production WASMs and all 18 ABIs reproduce byte-for-byte. In particular, chalg and uwrit match, so I am withdrawing my earlier artifact-reproducibility finding—the prior mismatch was caused by combining the final headers with a different compiler binary. The ASan and assertions configurations have passed; the remaining CI matrix jobs are still in progress.
Summary
system_contractandtrx_priorityeach cached their global row in a member and wrote it back from a destructor. The generated dispatcher destroys the contract as soon as the action returns, so every action issued akv_set-- including the view actions that exist to be called throughsend_read_only_transaction, which the chain refuses withcannot store a KV record when executing a readonly transaction.The original report looked self-contradictory, and this is why:
viewnodedistagainst a non-node-owner account reportedaccount is not a node ownercorrectly, while the same action against a valid account died.sysio::checktraps before the destructor runs, so only the success path ever reached the offending write.Both singletons move to
kv::cached_global, which defers the write to the end of the action and performs it only when something actually mutated. The_gstatemembers and both destructors are gone: readers use_global.get(), mutators_global.modify(). No contract in the tree has a destructor any more.Defaults belong to the handle, not to the constructor
An earlier revision of this branch seeded the global from the constructor. That charges every action for a singleton most actions never touch: seeding reads the row, and
get_default_parameters()passed as an argument runs itsget_blockchain_parametershost call whether or not the action goes on to use the global. Only five functions do --onblock,update_ranked_producers,setram,setparamsandpayepoch-- so roughly forty actions were paying for a singleton they never read.The defaults now ride on the singleton type via
kv::cached_global's newMakeDefaultparameter (Wire-Network/wire-cdt#107). The constructor body is empty and everything defers to first use, so an action that never touches the global neither reads the row nor computes the defaults, and one that only reads it still owes no write.get_default_parametersmoves out ofsystem_contractas a freedefault_global_state, because the alias names it as a template argument and has to exist before the class is declared.sysio.systemgrows 4380 bytes. That is the price of materializing lazily rather than in the constructor, not of the defaults themselves: with a trivial default it is 4196, and routing every access through a single materialization point recovers only 367. Seeding eagerly let the optimizer prove the cache loaded once and strip the checks everywhere downstream, which is precisely what deferring gives up.trx_priority::touch_last_updateusesmodify_or_create(the cached wrapper's creating form, renamed fromupsertin the CDT PR to keep it distinct fromkv::table::upsert, which stores its default verbatim and skips the updater on insert). The distinction matters here: the blank default is only a seed and the lambda runs on the create path too, which is what stamps the current time on the very firstaddtrxp/deltrxprather than slot zero.Rebuilt contract binaries
Eight binaries are affected, not one. The
kv::globalsize check added in Wire-Network/wire-cdt#106 changes generated code for every contract that uses the type:chalg,dclaim,epoch,msgch,opreg,reserv,systemanduwrit. PR builds run withBUILD_SYSTEM_CONTRACTS=OFFand exercise the committed binaries, while a release tag rebuilds from source -- so leaving any of them stale would mean CI testing one artifact and a tag shipping another, from identical source.Merging
mastermoved four of them.opregnow takes the guard frommasteritself, which rebuilt it independently;epochandreservare rebuilt here becausemaster's copies still predate wire-cdt#106; andsystemis rebuilt because it combines this branch'scached_globalhandle withmaster's claimable-payout emissions work. A sweep of all seventeen contracts on the merged tree, built against a from-scratch wire-cdt master (adad4b290) toolchain, reproduces every other one byte for byte --chalg,dclaim,msgchanduwritamong them, so the rebuilds this branch carries for those stand as committed. None of them referencekv::global.uwritwas already stale for an unrelated reason: its committed artifact came from a different CDT install than the rest of the tree and sat 27KB away from what this toolchain produces from the same source. This corrects it.test_contracts/sendinlineis deliberately untouched -- it references none of the changed headers, and its output differs across rebuilds at an identical size, so committing a fresh snapshot would be churn rather than a fix.No ABI changed.
Notes for review
payepochpreviously decrementedtotal_unpaid_blocksinside the producer reset loop. It now accumulates the reclaimed count across the loop and applies it in a singlemodify, so the reset costs one deferred write regardless of producer count.Only one handle exists per singleton (
"global"_nand"trxpglobal"_n), so the deferred-write visibility rule that comes with caching does not apply -- there is no second handle that could observe a stale row mid-action. That also matters for the one nested call:onblockreads the global three times and mutates it twice, then callsupdate_ranked_producers, which mutates it twice more. Because the handle is a contract member rather than a local, those five mutations collapse into a singlekv_set. The payer for every deferred write isget_self(), unchanged from the destructor it replaces.On a handle carrying defaults,
exists()reports that a value is available rather than that a row is stored. Nothing here asks that question, and the idiom it affects -- create the row ifexists()is false -- is the one this change exists to retire.Dependency
Requires
kv_cached.hppand thekv::globalhardening from Wire-Network/wire-cdt#106, plus theMakeDefaultparameter from Wire-Network/wire-cdt#107. PR CI does not build system contracts, so this stands on its own here, but a release tag rebuilds them against the latest published wire-cdt release -- that release has to ship before the next wire-sysio tag orsysio.systemwill not compile.Regression coverage
emissions_testsgains a helper that pushes a system action as a read-only transaction, plus two cases: a view action must execute and return the same payload it returns normally, andviewnodedistmust still report its own error read-only. Swapping the pre-change wasm back into the build directory reproduces the original chain error in the first case and leaves the second passing, which is the asymmetry described above.Those two cannot reach the absent-row path, because their fixture has produced post-deploy blocks and
onblockhas already persisted the global.sysio_fresh_deploy_testerdeployssysio.systemand stops, so the row has never been written, and a third case walks that path: a succeeding action that does not touch the global must not create the row, a read-only query must run with it still missing, the values served must be the declared defaults rather than a zero-initialized struct, and the first genuine mutation must store defaults plus that change. Reverting the constructor to the pre-fix seeding leaves both existing read-only cases passing while that case fails.