WIRE-344: Slash noncanonical delivery before termination - #572
Conversation
Change-Id: I292c7b7f9b38e0aeb6af0f8223db34777032fc0a
Change-Id: If39d46c11c2a47ec968405d9fef368c96388499b
Change-Id: I27f8c04eabcafa25870d73751e4cb9630ae190d6
Change-Id: Iddbc2cacdd7aba495a17e41145bf0f93dd889e6d
Change-Id: I14c08bc8c66b64504b431cb209000906bb81c1d2
Change-Id: Iba0e1f62c248adece4c2a49ef3f43d4e681402cc
Change-Id: I69bef6855da5aa696ee5826048e01588c63b25a8
Change-Id: I99d1c2220d05d732d0491f631a05885cf737b47e
Change-Id: I8d7c438318a066d38b38ade519fafdc27bf068af
Change-Id: I1d1788e34495fffa02024101953107526a111468
Change-Id: Id1b324ed568ba62d5caf912e64b12a45815c790f
Change-Id: I1a1c7f48e3b144bb6b722f8d19b93532d98d83d0
Change-Id: I7daa38d9b8d4de069364eda4a02bebf513d330dc
Change-Id: I0cb61b2bc6176044fd77a71652607219003e0f4b
Change-Id: Ie34d67981a0c1e17e7ddc9d724306cc2af01dcea
Change-Id: Ie84885038d04ef10ae74aef1997fa2702aa3e4f6
heifner
left a comment
There was a problem hiding this comment.
Review — the fix is correct; one comment is misleading, and one artifact wants provenance
The core change is right. Moving the recorddel/termcheck sends after the to_slash flush works because inline actions execute depth-first in send order, so chalg::slashop → opreg::slash flips the operator to SLASHED before opreg::termcheck runs, and termcheck early-returns on non-ACTIVE (sysio.opreg.cpp:1734). That removes the "operator already terminated" throw that aborted advance. The test reproduces the pre-fix failure faithfully — batchop.a's dellog is T,T,F,F,T, so worst_consecutive = 2 > 1 at the first termcheck.
Both magic_enum::enum_integer swaps are safe: kv::table::index::lower_bound converts to the declared secondary_key_type (uint64_t) before encoding, so the int32_t return can't shorten the key. Dropping static_cast<ChainKind>(link_it->chain_kind) is a genuine no-op — links_s::chain_kind is already opp::types::ChainKind.
1. sysio.epoch.cpp:604 — the invariant comment is wrong, though the invariant holds
Claim 1 — "this flush runs BEFORE the window-slide below, so the operator is already SLASHED when the next tail group is formed" — is false. slashop is an inline action, so it doesn't apply until advance()'s own apply-context completes, while the window slide reads opreg::operators synchronously and still sees OPERATOR_STATUS_ACTIVE. Claims 2 and 3 don't rescue it: the expiring front group is erased first, so the offender is no longer resident, and the non-bootstrapped-first sort makes it a likely pick for the new tail. The new test shows this directly — after advance_via_consensus(), batch_op_groups[0] still contains batchop.a despite it being SLASHED.
But the invariant it protects still holds, by a different mechanism. A double slash needs the operator to appear in to_slash again, which requires did_deliver (sysio.epoch.cpp:587) — and msgch::deliver rejects non-ACTIVE operators at sysio.msgch.cpp:1210. A SLASHED operator cannot deliver, so it cannot diverge, so it cannot be slashed twice. No abort, no stall.
I want to be explicit that I first assessed this as a permanent-stall risk and that was wrong. The scenario I had — operators_per_epoch = 1, the offender scheduled as a sole group member — is self-defeating: slashing requires member_checksum != winner, and a single-member group's checksum is the winning bucket, so that operator can never be slashed. (operators_per_epoch = 1 is contract-legal, incidentally — setconfig only enforces > 0, <= 100, and the product equality — it just can't produce this fault.) At every real group size the slashed set is by construction the non-winning minority, so the surviving majority always meets the threshold: 3 → 2 healthy vs threshold 2; 7 → 4 vs 4.
So this is a documentation defect, not a correctness one — but a load-bearing one. The comment tells the next reader that safety comes from scheduling exclusion. It actually comes from the deliver-time ACTIVE gate. Anyone who later relaxes that gate — e.g. to let SLASHED operators deliver for accounting — reintroduces the double-slash abort, and this comment would have told them it was impossible. Worth rewriting claims 1–3 to name the real guarantee, and the closing line ("If any of those three scheduling facts change, this single-slash path must be revisited") should point at msgch::deliver's status check instead.
Residual real effect, minor: a just-slashed operator can still be scheduled into the new tail, where it burns a slot and accrues recorddel(delivered=false) misses. No termination follows (termcheck early-returns on non-ACTIVE). At operators_per_epoch = 3 it removes that window's fault tolerance, since the 2 healthy members must then both deliver. Filtering the new-tail pool against to_slash would close it if you think that's worth the code.
2. sysio.epoch.wasm — confirm provenance before merge
The tracked artifact grows 75,700 → 92,372 bytes (+22%) for a change that reorders two .send() calls, adds one std::vector<{name,bool}>, and adds a magic_enum include that is already transitively present via sysio.opreg.hpp. String tables in the two binaries are effectively identical, so the toolchain doesn't look changed — which makes a 16.6 KB delta larger than the diff plausibly accounts for.
This matters more than usual here because CI deploys the tracked artifact rather than rebuilding, so this binary is the thing under test. Per commit-wasm-only-with-source-change.md, worth a clean rebuild and byte-compare before merge. Happy to run that if useful — it is the same check I ran across all 17 contracts on #563 and #568.
Notes (not blocking)
contracts/tests/sysio.msgch_chain_tests.cpp:161— deployingsysio.chalginto this fixture newly activates theevalcons→chalg::opendisputeinline path, previously a silent no-op on a codeless account. Inert today: no test in this file produces 3+ distinct checksums past the boundary, somaybe_open_disputeshort-circuits atseen_checksums.size() < 3. But if a future test in this fixture does,opendisputewill throw"cannot open a dispute with no registered tier-1 node owners"(nosysio.roahere) and abort the delivering tx.slash_action_countreadsrecent_actions, which is capped at 5 newest-first.batchop.aaccumulates 2 deposits + 2 slashes = 4, so the slash entries survive eviction — correct, but with one slot of margin.
Provenance check: the committed
|
| bytes | vs base | |
|---|---|---|
base 9cde4029e9 |
75,700 | — |
| rebuilt from this PR's source | 76,069 | +369 |
| committed in this PR | 92,372 | +16,672 |
The actual source change costs +369 bytes (+0.5%), which is what you would expect: it reorders two .send() calls and adds one std::vector<delivery_observation>.
Worth stating explicitly, since "the new vector bloated it" is the natural hypothesis: it did not. std::vector was already instantiated 12 times in this file, and delivery_observation { name; bool; } is layout-identical to the std::vector<std::pair<name, bool>> already present at lines 626 and 890. The marginal cost of the new instantiation is near zero, and the rebuild confirms it.
What the committed binary actually is
Section-level comparison:
| section | committed | rebuilt | delta |
|---|---|---|---|
| code | 89,297 | 72,774 | -16,523 |
| function | 162 | 310 | +148 |
| type | 188 | 207 | +19 |
More code, fewer functions — the signature of aggressive inlining. The committed artifact looks built at a different optimization level than the repo's standard contract build. Corroborating detail: the committed binary is missing the string "unexpected error in fixed_bytes constructor" that the standard build emits, consistent with optimization proving that path unreachable and dropping the assert.
(Mismatch and section deltas are measured; the optimization-level explanation is the most consistent reading of them, not something I have proven directly.)
Why this blocks
Per this PR's own description, CI deploys the tracked artifact rather than rebuilding, so this binary is the thing under test — while a release tag rebuilds from source and would ship the 76,069-byte one. That is precisely the split 745df70141 ("fix(contracts): rebuild the binaries the kv::global size check changes") set out to close: CI testing one artifact and a tag shipping another, from identical source. It also means the on-chain CPU characteristics measured in CI are not those of the binary a release produces.
Fix is mechanical — rebuild sysio.epoch.wasm via contracts_project and re-commit it in the Sync WIRE-344 epoch contract artifact commit. Happy to hand over the 76,069-byte artifact I produced, though it is better regenerated in your own tree so the provenance is yours.
Reproduce:
git worktree add <path> f9c7e1c818 && cd <path>
git submodule update --init --recursive vcpkg libraries/appbase
cmake -B build/claude -S . -G Ninja -DCMAKE_BUILD_TYPE=Release \
-DBUILD_SYSTEM_CONTRACTS=ON -DBUILD_TEST_CONTRACTS=ON -DENABLE_TESTS=ON \
-DCDT_ROOT=<wire-cdt>/build -DCMAKE_PREFIX_PATH=<wire-cdt>/build \
-DCMAKE_TOOLCHAIN_FILE=$PWD/vcpkg/scripts/buildsystems/vcpkg.cmake
cmake --build build/claude --target contracts_project
cmp contracts/sysio.epoch/sysio.epoch.wasm build/claude/contracts/sysio.epoch/sysio.epoch.wasmChange-Id: Iaff848a24819393707e3e9c4c4d694cacfe3c9f6
heifner
left a comment
There was a problem hiding this comment.
Approving — both findings from my earlier review are resolved, and I verified the artifact fix independently rather than taking the size on trust.
Artifact provenance (was the blocking one). sysio.epoch.wasm is regenerated at 76,069 bytes, and it is byte-identical to my own clean rebuild of this tree. That is a real reproduction rather than a coincidence of size: the source delta in 737e0cb6f6 is comment-only, so the compiled output must be unchanged, and it is. The 16,303 bytes that had no source-level explanation are gone, and CI now deploys the same binary a release rebuild produces.
Invariant comment. The rewrite names the actual guarantee instead of the scheduling one. It is now explicit that the inline slashes execute only after advance returns, that the slide can temporarily seat a just-slashed operator while it still reads ACTIVE, and that this is harmless because sysio.msgch::deliver requires current ACTIVE status before accepting a delivery. Repointing the closing caveat at the deliver status gate is exactly the coupling worth pinning — that gate is now the load-bearing thing, and the next person to touch it will see why.
The dedup note added on the collection is a good addition; it closes the within-advance duplicate case that the old text left implicit.
For the record, correcting my own first pass: I initially raised the scheduling staleness as a permanent-stall risk. That was wrong. The scenario I had depended on operators_per_epoch = 1, which is self-defeating — slashing requires member_checksum != winner, and a lone group member's checksum is the winning bucket. At every real group size the slashed set is the non-winning minority, so the surviving majority always meets the threshold. The comment was still worth fixing, but as documentation, not correctness.
CI is green across asan / ubsan / gcc / asserton / ubuntu24 on this head.
Non-blocking, unchanged from before and fine to leave:
sysio.msgch_chain_tests.cpp:161— deployingsysio.chalgnewly activates theevalcons→chalg::opendisputepath in this fixture. Inert today (nothing here produces 3+ distinct checksums, somaybe_open_disputeshort-circuits), but a future test that does will throw"cannot open a dispute with no registered tier-1 node owners"since there is nosysio.roain this fixture.slash_action_countreadsrecent_actions, capped at 5 newest-first;batchop.aaccumulates 4, so it survives eviction with one slot of margin.
Summary
advance, andsysio.msgch::deliveraccepts deliveries only from operators that are currentlyACTIVE, preventing a scheduledSLASHEDoperator from creating a subsequent non-canonical observation.sysio.epoch.wasmfrom the canonicalcontracts_projectbuild. It is 76,069 bytes (SHA-2563a41534c46d4ad8141dc7bae6a5ccc37e1897e71937b7e9c18e09da7f6aa95b3); the other 16 contract artifacts reproduced byte-for-byte. CI deploys this tracked artifact, so this keeps CI behavior aligned with the source rebuilt for releases.Validation
737e0cb6f6168fd72722696fe58ea9d5c880a009.737e0cb6f6168fd72722696fe58ea9d5c880a009.sysio_msgch_chain_tests/noncanonical_delivery_slashes_before_termination— passed.