WIRE-291: prevalidate underwriter UIC signatures - #543
Conversation
ReviewRead the full diff (contract, plugin, new shared header, both test files, README) plus the surrounding call chain ( The core fix is right: replacing Three items below — one design suggestion on the accepted signature set, and two findings. 1. The accepted set should be the fixed-size recoverable variants, not K1 aloneThe justification in Every fixed-size variant supports the identical
Supporting K1/R1/EM/ED costs a
WA and BLS genuinely can't be supported, and should keep returning
Why this matters concretely, and not just in principle: the plugin's provider gate is The new preflight makes the narrowing loud rather than silent, which is the right call if K1-only stays. But an R1 operator now hits a hard startup failure for no protocol reason. Suggested resolution, in preference order:
2.
|
|
@heifner Thanks for the detailed review. All three items are addressed in the published branch.
The follow-up also bounds hostile UIC protobuf input to 2 KiB before parse/hash/recovery, derives plugin tags and packed sizes from canonical Published follow-up commits:
Validation at current head
Review has been re-requested from you on the updated head. |
heifner
left a comment
heifner
left a comment
There was a problem hiding this comment.
Re-review
Re-read at b67f55c1 — the three fix commits plus the master merge. All three items from the previous round are addressed, and the four-variant generalization is correct on both the fc and CDT sides. Three new findings below: one that makes this round's EM/ED support unreachable on a real node, one replay vector, and one malleability issue. Details on what I verified as correct are at the end.
1. EM and ED providers can never pass preflight, so the new depot support is unreachable
chain_plugin::plugin_initialize runs this unconditionally, on every node type:
if (!sig_plug.has_signature_providers(std::array{crypto::chain_key_type_wire})) {
sig_plug.register_default_signature_providers({crypto::chain_key_type_wire});
}has_signature_providers matches on chain_key_type_t and ignores target_chain, and the auto-created default is built with target_chain = chain_kind_wire and a K1 key. So the default appears exactly when no provider has key_type == chain_key_type_wire.
The new preflight then does:
auto wire_providers = underwriter_detail::select_uic_signature_providers(
sig_plug.query_providers(std::nullopt, fc::crypto::chain_kind_wire));
if (wire_providers.size() != 1) { ... return false; }and select_uic_signature_providers keeps every target_chain == wire provider whose key is K1, R1, EM, or ED.
Follow this PR's own error message — "EM key-type=ethereum, or ED key-type=solana" — and the node has no key_type=wire provider, so chain_plugin creates wire-default. The selector now keeps both it and the EM provider, since K1 and EM are both supported. wire_providers.size() == 2, preflight fails with "expected exactly 1 WIRE UIC signature provider, got 2", the cron is never registered, and the underwriter is inert. The identical guard in create_signed_uic_bytes would abort every commit regardless. Same for ED via key-type=solana.
K1 and R1 are unaffected, because configuring a key-type=wire provider suppresses the auto-default — which is why this doesn't show up in practice yet. The only configuration that threads the needle is a key_type=wire provider whose target_chain is not wire, which is semantically meaningless.
uic_provider_selection_reaches_all_supported_native_key_formats cannot catch this: it initializes signature_provider_manager_plugin standalone, so chain_plugin never runs and wire-default never exists. The test asserts exactly the property the real node violates.
Suggested fix, your choice of shape: filter the selection to explicitly-configured providers — the manager already exposes is_explicitly_configured_provider, and register_default_signature_providers deliberately calls create_provider rather than create_configured_provider, so the auto-default is already distinguishable — or replace "exactly one" with an explicit --underwriter-uic-signature-provider <key-name> selector. Either way the regression test needs chain_plugin in the loop, or it will keep passing while the node fails.
2. A valid signature authenticates the bytes, not the submission — any ACTIVE underwriter can replay a competitor's UIC to drive that competitor's lifecycle
verify_uic_signature establishes that the named underwriter signed these bytes. Nothing establishes that the named underwriter asked for them to be submitted now.
UnderwriteIntentCommitcarriesuw_account,uw_ext_chain_addr,uw_request_id,signature,token_code,chain_code,reserve_code. No nonce, no timestamp, no expiry.- The bytes are public:
commit_entryreflectssource_uic_bytes/dest_uic_bytesinto theuwreqsrow, soget_table_rowshands any observer the verbatim signed blob. They are also public Ethereum calldata. OperatorRegistry.commit(bytes)gates only onmsg.senderbeing an ACTIVE underwriter and relays the bytes opaquely. It emitsUnderwriteCommitRelayed(msg.sender, uicBytes), so the relayer identity exists on the outpost and is deliberately not carried into the attestation.rcrdcommithas no "this leg is already recorded" guard — themodifyoverwrites and refreshes*_received_at_msunconditionally — and its tail re-invokestry_select_winneron every accepted record once both legs are present. That is the only path that re-evaluates a race which previously skipped.
Scenario: honest underwriter B lands both legs. try_select_winner runs, hits the momentary insufficient-reserve-liquidity skip, and returns; the request stays PENDING with B holding both legs. Attacker A (any ACTIVE underwriter) reads B's source_uic_bytes off chain state and relays them verbatim through the same outpost. Verification passes — they are B's genuine bytes — the row is refreshed, and try_select_winner(B) re-runs at A's chosen moment.
What A gets to choose:
- Kill the swap outright. Fire once the live quote has drifted past
variance_tolerance_bpsand the race-time recheck reachesreject_and_refundwith "variance exceeded tolerance at race resolution". The request goes terminally REJECTED, the user is refunded, every candidate is released — including B, who never asked to be evaluated then. - Evict B and take the swap. Fire once B's mirrored collateral is committed elsewhere, so the bond check disqualifies B. A then lands its own pair and wins. This is the previous round's eviction outcome through a different door — A never needs B's key, only B's published bytes.
- Force B into a position. Fire at a moment where B does win, locking B's collateral for
collateral_lock_duration_ms. B signed the intent, not the timing.
Bounded by A having to be an ACTIVE bonded underwriter, one outpost transaction per replay, and the pending-timeout window.
Suggested fix, in preference order:
- Carry the relayer into the attestation. The outpost already has it in
UnderwriteCommitRelayed; thread it through_sendAttestationand havedispatch_underwrite_commitrequire the relayer's registered WIRE account to equaluic.uw_account. That makesuw_accountprovenance-bound the same way WSA-005 boundchain_code, and retires the "claim" caveat from the design entirely. - Failing that, add an expiry or nonce inside the signed digest and reject stale UICs in
rcrdcommit.
Worth noting a byte-equality dedup does not work as a substitute: fc K1 signing is deterministic, so B's own legitimate re-commit reproduces byte-identical bytes and would be silently dropped.
3. The stored "verbatim" UIC bytes are malleable — unknown protobuf tags pass verification and inflate the row
zpp-bits does not require the input to be fully consumed, and for an unknown field number it returns success without consuming the field payload:
if constexpr (Index >= number_of_members<type>()) {
if (!field_num) [[unlikely]] { return errc{std::errc::protocol_error}; }
return errc{};
}So a byte 0x48 — field 9, wire type 0, which UnderwriteIntentCommit does not declare — is consumed and ignored, one per loop iteration. Appending a run of them to a genuine ~100-byte UIC yields a blob that decodes to an identical message, blanks to an identical digest, and therefore verifies identically, right up to the 2 KiB MAX_UIC_LEG_BYTES cap. rcrdcommit then stores it verbatim, roughly twenty times the real payload, with RAM billed to sysio.
This does not reach a hard denial of service — 32 candidates x 2 legs x 2048 stays under MAX_UWREQ_ROW_BYTES, so honest entries cannot be crowded out — but the header describes these as "the verbatim UIC bytes" and the challenge-evidence design reads as though they were canonical, and they are not: what is retained through the lock window is whatever arrived last.
A weaker instance of the same class: K1/R1/EM recovery does not enforce low-s, so the standard (r, n-s, recid^1) transform gives a byte-different 65-byte body that recovers the same key.
Suggested fix: after the blank-and-re-encode step, also re-encode with the signature restored and require byte equality with uic_bytes, rejecting as a new typed result. That is one extra zpp::bits::out over already-decoded data, it closes both variants, and it makes "verbatim" true.
4. validate_candidate_uic's justification is wrong, and the branch it guards is now untested
The comment and the README both describe this as defense for "commit bytes already stored before pre-validation was introduced" / "legacy stored rows". That cannot happen: source_uic_bytes / dest_uic_bytes have exactly one writer, the modify in rcrdcommit, which this PR now gates behind verification. Every other reference is a .clear(). On a chain deploying this wasm there are no legacy rows, so as written the comment says the block is dead code and invites a future cleanup to delete it.
There is a real reason to keep it, and it should be the stated one: verification runs against the current active/owner permission, so re-checking at winner selection catches an underwriter rotating keys between the source leg landing and the destination leg arriving. That is structural, not a migration artifact.
Two follow-ons. It doubles recovery work on the consensus path — a confirming two-leg swap now performs four full verifications where the pre-fix code performed two, all inside the evalcons dispatch chain. And the branch has no test at all: the rework removed every case that reached it, so create_uic_signature_rejection_reason and the whole invalid underwrite-intent-commit <leg> signature: <result> durable-reason format are unexercised. A test that stores a valid source leg, rotates active, then delivers the destination leg would cover the real behaviour and document the real reason.
5. Test regressions worth reverting
- The typed
uic_signature_resulttaxonomy is asserted nowhere.swap_malformed_signature_shapes_are_ignored_before_storagereplaced its per-caseexpected_reasonassertions with a singlereq["commits_by"].get_array().empty(), identical for all eight rows. It can no longer distinguish rejection for the right reason from rejection by an unrelated earlier guard, which is exactly what the case comment claims it establishes. Ifenum_caststopped matching tag 3, EM would silently collapse frominvalid_signature_lengthtounsupported_signature_typeand every row would stay green. - The over-length direction was dropped for all four variants. The pre-fix table had an explicit
oversized_67; the rewrite is all-short. Addingtag(k1)+ 66 zero bytes andtag(ed)+ 97 restores it cheaply. - A pre-existing test was silently made vacuous by the reorder.
rcrdcommit_matched_leg_non_underwriter_names_do_not_grow_rowpassesstd::vector<char> uic(8, '\x00'), which the new signature check now rejects on its own — so the test passes even with theis_active_underwritergate removed, and no longer isolates the feature it names. The same call site pattern was correctly updated inrcrdcommit_candidate_cap_bounds_row; this one was missed. - Smaller gaps:
unauthorized_keyis covered for K1 only, so the newly-permitted R1/EM/ED have "accepted when authorized" but no "rejected when not authorized"; there is no ED test that corrupts the body while leaving the embedded pubkey intact, which is the one case that would catch an ED recovery regression; WebAuthn and BLS appear only as bare one-byte tags, so an implementation accepting tag 5 at exactly 193 bytes would pass; and there is no cross-variant length confusion case (ED tag with a 66-byte body).
6. Smaller code items
- Nothing binds the hardcoded variant indices to CDT. The sizes are derived (
std::tuple_size_v<sysio::ecc_signature>), butk1 = 0, r1 = 1, em = 3, ed = 4are bare literals, and the file has nostatic_assert. Because indices 0, 1, and 3 are allecc_signatureinsysio::signature, a CDT reorder would still compile and still pack 65 bytes — it would just silently recover against the wrong curve. Only ED would fail to compile. A fewstatic_assert(std::is_same_v<std::variant_alternative_t<...>, ...>), including one pinning the WebAuthn gap at index 2, make that a build break instead. - The K1/R1/EM arms are byte-identical triplicates, differing only in the
emplaceindex, so the ECC size check exists in three places and the ED arm's different constant is easy to miss (CLAUDE.md invariant 1). A small local helper templated on the index, or acase k1: case r1: case em:fallthrough emplacing viamagic_enum::enum_integer, collapses it. switch (*signature_variant)has nodefault:arm. Unreachable today, but adefault: return unsupported_signature_type;keeps a future enumerator from falling through to the value-initialized K1 alternative.- The candidate-roster rail is a cheap
any_ofover at most 32 entries on an already-loaded snapshot, and it runs after the protobuf decode, hash, and key recovery. Moving it ahead of verification avoids a secp256k1 recovery per record that is going to be discarded anyway. The row-size projection should stay where it is. create_signed_uic_of_sizehardcodesPACKED_K1_SIGNATURE_SIZE = 66in the same fixture wherecreate_truncated_uic_signature's comment says the size deliberately "comes from fc's real signer/serializer rather than a duplicated protocol constant."
Verified correct
Stating these explicitly since they are the load-bearing claims, and several were confirmed by running code against this tree's libfc rather than by reading:
- Tag/index mapping is exact on both sides. CDT
signature = std::variant<ecc, ecc, webauthn, ecc, ed, bls>and fc'ssig_typederived from the same ordering; measuredk1=0 r1=1 wa=2 em=3 ed=4 bls=5.public_keymatches. Noteprivate_key::key_typehas a different order with no webauthn — the PR never mixes them, anduic_signature_type_matches_provider_keycompares onlypublic_key::key_typeagainstsig_type, guarded by fourstatic_asserts. - Sizes 66/66/66/97 are exact and off-by-ones are rejected. CDT packs
std::array<T,N>element-wise with no length prefix; the tag is LEB128, one byte for 0..127. Cross-tag lengths are rejected too, since each arm compares against its own constant. - WebAuthn and BLS are unreachable by any length coincidence. Enumerating
enum_cast<uic_signature_variant>over the full 0..255 domain accepts exactly{0,1,3,4}; tags 2 and 5 reject beforeparsed_sigexists, regardless of payload. Non-canonical multi-byte varint tags reject too, since the leading byte lands above the enum range. - Memory-safe on every hostile shape. Empty, one-byte, valid-tag-wrong-length, and cross-tag-length all exit before reading past index 0; ECC reads 1..65 of a 66-byte buffer and ED reads 1..96 of a 97-byte buffer.
- Public-key sizes match across the host boundary for all four curves (33/33/33/32 bodies), all fit the optimistic buffer with margin, and
std::variant::operator==comparesindex()first so the duplicate ECC alternatives cannot cross-match.authorization_manageradmits K1/R1/WA/EM/ED in permission authorities, so an EM or ED key really can sit onactive. - The non-throwing contract holds.
recover_keywraps both the unpack and the recovery incatch (const fc::exception&) -> return -1, and the still-throwing speculative-block guard keys onvariable_size(), which is 0 for all four accepted variants — unreachable even at limit 0. ED is not a forgery vector:signature_shim::recoverassertsverifybefore returning the embedded key. - The eviction primitive from the previous round is genuinely closed. Full mutation inventory of
rcrdcommit: verification strictly precedes the singlemodifyand thetry_select_winnercall, every earlier gate is read-only, the RAM payer never changes, and there is no exception path. The digest bindsuw_request_id,token_codeandreserve_code, andchain_codeis provenance-pinned, so a forged claim cannot land anywhere its bytes were not signed for. The candidate-slot denial variant is closed with it — a slot now costs a signature over that specific request. - Checked-in
sysio.uwrit.wasmwas genuinely rebuilt — containsinvalid_signature_length,chain_code=,claimed_underwriter=, and contains neitherinvalid_k1_lengthnoroutpost_id=. The ABI is correctly byte-identical, since the entire header diff is comment lines. No other committed wasm needed rebuilding. - The
swap_forged_claim_cannot_overwrite_honest_candidatetest is a real regression test — traced against the pre-fix contract it fails at two independent points, not one. The per-curve acceptance test genuinely discriminates, since a broken arm would recover a non-matching key and leave the request PENDING. - Item 3 from the previous round is fixed: the marker prints
chain_code=alongsideclaimed_underwriter=.
|
@heifner Thanks for the detailed re-review. All six finding groups are addressed on the current branch. The main follow-up is f276482; f1a8487 is the additive integrated-build correction discovered by the full platform gate.
No new depot or outpost configuration is introduced. Because this is pre-launch, the outpost storage/behavior changes are intended for coordinated disposable redeployment. Validation at exact head f1a8487:
The PR description has been refreshed for the complete current diff and validation. Re-review is requested on the updated head. |
heifner
left a comment
heifner
left a comment
There was a problem hiding this comment.
Re-review
Re-read at f1a8487a4c. Most of this round is genuinely fixed, and several pieces are fixed well — details at the end, because they deserve stating. I'm keeping Request Changes for a sequencing reason rather than a defect in the depot code, plus one new operator-facing regression.
I independently verified the validation claims: Linux, macOS and OPP Bundles all green at this exact head, and the platform E2E genuinely dispatched BRANCH_WIRE_SYSIO / BRANCH_WIRE_ETHEREUM / BRANCH_WIRE_SOLANA at WIRE_BUILD_TYPE=Release, finishing 13/13 with swap-with-underwriting passing.
1. BLOCKING — merge order is load-bearing: a parser differential defeats the outpost caller binding on its own
The caller binding in wire-ethereum#178 and wire-solana#416 is correctly implemented, but it is not sufficient by itself, because the outpost decoders and the depot decoder disagree about unknown fields.
Both outposts skip an unknown length-delimited field by consuming its body:
// ProtobufRuntime.sol::_skip_field, wire type 2
uint64 len;
(len, newPos) = _decode_varint(data, pos);
newPos = newPos + uint256(len);// protobuf_runtime.rs::skip_field, wire type 2
let (len, new_pos) = decode_varint(data, pos)?;
let end = new_pos + len as usize;The depot's zpp-bits does not — on an unknown field number it returns success without consuming the payload, so the body is then re-parsed as further top-level fields:
if constexpr (Index >= number_of_members<type>()) {
if (!field_num) [[unlikely]] { return errc{std::errc::protocol_error}; }
return errc{};
}So an attacker who is any ACTIVE underwriter submits:
[field 1 = WireAccount{name: "<attacker>"}]
[0x7a] // tag: field 15, wire type 2 — undeclared
[varint(len(V))]
[V] // the victim's genuine, published UIC bytes, verbatim
The outpost parses field 1, skips the wrapper, resolves uw_account to the attacker, matches msg.sender, and relays the blob unchanged. The depot consumes only the wrapper tag, reads the length varint as another unknown tag, and then parses V as top-level fields — reconstructing the victim's UIC exactly, signature included. verify_uic_signature sees the victim's genuine preimage, the signature verifies, and rcrdcommit runs with underwriter = victim, re-invoking try_select_winner against them at the attacker's chosen moment. That is the original replay finding, restored intact through the mechanism intended to close it. This was built and executed against the real generated depot struct and the real zpp_bits.h, and I confirmed the differential directly in all three decoders.
What blocks it is the canonical re-encode check in this PR — the crafted blob is longer than its canonical re-encode, so it lands as non_canonical_uic and is dropped before any mutation.
That makes the merge order load-bearing. If #178 and #416 land before this PR, the binding presents as closed while remaining fully exploitable. Both companion PR bodies currently describe the binding as self-sufficient, which is what I'd most like corrected. Concretely:
- Gate all three on this PR landing first (or together), and say so in the two companion descriptions.
- Consider making each outpost self-sufficient by rejecting non-canonical bytes locally — the generated models already expose
encode, so it is a comparison against the received bytes, and on Ethereum akeccak256comparison is cheaper than a memory compare. - Note the standing obligation: any future outpost that omits the canonical check reopens this by itself, since the depot deliberately does not bind the relayer.
Neither PR has a test for the wrapper shape. If you adopt the outpost-side check, the regression to add on each chain is an attacker-named prefix wrapping a victim UIC in an unknown length-delimited field, asserting rejection with no queue mutation.
2. The cross-generator test — the gate the whole canonical design rests on — never runs pre-merge
Adding test_uic_cross_generator.cpp was exactly the right call, and its content is good: it compiles the host protobuf model and the CDT zpp model into one translation unit and asserts byte agreement on both the blanked and full encodings across K1, R1, EM and ED.
But in libraries/opp/CMakeLists.txt it is removed from the glob and re-added only under if(TARGET ${TARGET_CDT_MODELS_NAME}). That target exists only when BUILD_SYSTEM_CONTRACTS is ON, and the Linux workflow sets SYSIO_BUILD_SYSTEM_CONTRACTS to ON only for refs/tags/v*; the macOS path never enables it. So on every PR and every master push the file is silently dropped from test_opp and never compiled or executed — it becomes live only on a tag build, after merge.
Host/CDT byte agreement is the single assumption the entire non_canonical_uic enforcement rests on, and right now nothing verifies it before merge. The skip path also emits no diagnostic, so it is invisible in configure output. At minimum emit a message(WARNING ...) when skipping, and arrange for at least one pre-merge CI configuration to build with CDT.
3. New regression: anonymous --signature-provider entries are now silently excluded
The explicit-provider filter is correct for the EM/ED collision it targets, and the regression test properly registers defaults through the same manager API chain_plugin calls. But the predicate keys on having an explicit name, not on being auto-registered:
.has_explicit_name = num_parts == 5 && !key_name.empty(),and the manager fully supports the four-field anonymous form — SYS_ASSERT(num_parts == 5 || num_parts == 4, ...), with the option help advertising "name to use when referencing this provider, if empty then auto-assigned".
So an operator running --signature-provider wire,wire,SYS...,KEY:... today has a working underwriter. That provider has key-type wire, which suppresses creation of the auto default, so there is no ambiguity to resolve — yet it is never marked explicit, the filter drops it, wire_providers.size() is zero, preflight returns false, and the cron never registers. The node comes up and the underwriter is silently dead.
The diagnostics then point the wrong way. The error says to "configure exactly one supported --signature-provider entry targeting chain=wire" — which the operator did — and the README contrasts "explicitly configured" only against "automatically registered defaults", which theirs is not. Nothing tells them the entry must carry a name.
Cleanest fix is to filter on what is actually meant: have register_default_signature_providers record the names it generates and exclude those, which admits anonymous operator specs and still excludes defaults. If the named requirement is deliberate instead, it needs to be stated in the error text and the README, and called out as config-breaking.
4. Smaller items
The variant-index assertions cannot detect the failure they were added for. sysio::signature declares alternatives 0, 1 and 3 all as ecc_signature, so static_assert(std::is_same_v<std::variant_alternative_t<0, sysio::signature>, sysio::ecc_signature>) and its siblings are satisfied by any permutation of K1/R1/EM. If the tags were reordered, parse_uic_ecc_signature<R1_SIGNATURE_VARIANT_INDEX>(..., p256, prefix, ...) would still compile while handing P-256-validated bytes to a secp256k1 recovery. Only indices 2, 4 and 5 are genuinely pinned. The host-side block does discriminate, because the fc shims are distinct types. What the contract side needs is a runtime assertion — recover a known-good vector per variant and check the key — rather than a type identity that cannot distinguish them.
${CDT_ROOT}/include leaks the WASM toolchain header tree into a host binary. opp_cdt_models is a non-imported interface target, so its include dirs are emitted as plain -I, while vcpkg's arrive as -isystem, and compilers search all -I first. The result is that headers in test_opp resolve out of the CDT tree — protobuf, magic_enum and Boost.PP among them — with the mixed shape being the hazard: headers present in the CDT tree shadow the vcpkg copies while absent ones fall through. It builds today only because both trees happen to pin the same protobuf and magic_enum versions, which is coincidence rather than a constraint. Scope it to the one source file, or add only the directory that actually supplies zpp_bits.h and mark it SYSTEM.
One claim in the response does not hold. Item 5 lists "a valid-signature fixture for the non-underwriter roster rail", but rcrdcommit_matched_leg_non_underwriter_names_do_not_grow_row has zero occurrences anywhere in this PR's diff and still passes eight zero bytes, with a comment asserting they are "dropped before the bytes are ever read" that is no longer true. It remains vacuous — with the roster gate removed, the payload simply falls through to the signature check and the test still passes. In fairness the gate itself is covered by two other pre-existing tests using real signed UICs, so the exposed branch is only "no operator row at all", and the fix is one line.
No test covers an input the canonical check newly blocks. In swap_noncanonical_uic_cannot_replace_valid_leg, the appended-unknown-field case asserts malformed_uic, and the comment explains this as proof "that this decoder does not silently discard extensions". That is backwards — it errors only because the trailing payload byte is re-read as a tag with field number zero. Drop that byte and the same input decodes cleanly and is stopped solely by the new equality check. The three cases that do assert non_canonical_uic are all proto3-default shapes, which were already unverifiable before this change. Worth adding the actually-reported vector — a 0x48 run appended to the leg cap — asserting non_canonical_uic, and correcting the comment.
Durable DISQUALIFIED now also covers transient conditions. Removing the re-arm is right for key rotation, since stale evidence must not become winnable. But the same terminal state is now reached for insufficient bond and the missing destination authex link, which are recoverable situations — an underwriter whose collateral is briefly committed to a concurrent winning request is permanently excluded from the second one. Availability rather than safety, and not attacker-triggerable as far as I can tell, but worth a deliberate decision rather than falling out of the change.
Duplication. The s' = n - s construction, the scalar size, and the recovery bases 31 and 27 are written twice across the two new test translation units, both of which already include the shared canonical header where the curve orders live. push_trace is a near-copy of push, which is itself already duplicated across six test files; the clean shape is one push_trace in the shared tester with push wrapping it.
Verified correct
Stating these because they are load-bearing and several were the risky parts:
- The ECDSA canonicality work is correct and safe to ship, and it deliberately left recovery-byte ranges alone, documented as outside the helper. That mattered more than it may appear: K1 and R1 sign with base 31 while EM and the KMS path sign with base 27, so a narrowing would have bricked signers. All four curve constants verify digit-for-digit against the real orders and half-orders; the boundary
s == n/2is accepted, consistent with libsecp256k1's normalization, the R1 path and fc's EM canonicality check; the comparison is big-endian over all 32 bytes withuint8_tcasts, socharsignedness and wasm32-versus-x86-64 are both non-issues. Every signer reachable from the UIC path — K1 and EM local, R1 via explicit normalization, EM via KMSnormalise_low_s— produces low-sby construction, so no configuration is rejected. Contract and daemon run literally the same header rather than two transcriptions. - The canonical protobuf check is comprehensive. Beyond the reported padding vector, it rejects non-minimal varints, duplicate singular fields in both value orders, out-of-order fields, non-minimal nested length prefixes, wire-type confusion, and unknown fields nested inside submessages. Only a byte-for-byte match with zpp's deterministic output survives, which is the right property.
- It does not widen the production failure surface. The blanked and full encodings agree or diverge together for every shape tested, so any message whose full encoding diverges already had a diverging blanked digest and could never have produced a verifying signature. The check converts a baffling
unauthorized_keyinto a named one. The zero-scalar divergence is also unreachable in practice —uw_request_idnever starts at zero, a non-empty slug always packs nonzero, and the WIRE-side reserve uses an explicit sentinel — though a cheap non-throwing guard increate_signed_uic_bytes, mirroring the existingkindtreatment, would make that a guarantee rather than a convention. - The ordering rework holds up.
just_verified_legcannot be wrong: it is derived from the sameis_source/is_destthat select the storage slot, verification runs on exactly the bytes then written, andtry_select_winnerhas a single call site. Single-leg, re-commit, and second-leg-after-rotation all revalidate the leg that needs it. The roster cap now genuinely precedes cryptographic work, andrcrdcommit_candidate_cap_bounds_rowpins that ordering by asserting the console shows the cap message and no rejection marker. - The test round is strong. The taxonomy assertions read real action-trace console output and fail closed; the two key-rotation tests are the best new material in the PR, asserting the leg-specific durable reason, unchanged bytes and timestamps, and no lock; the low-
sfixture carries an executable proof that its alternate really is a valid representation rather than corruption; and full-size WebAuthn and BLS payloads and cross-variant length confusion are now covered. - The committed
sysio.uwrit.wasmwas genuinely rebuilt. The new taxonomy strings are present and absent from the previous head, and the four raw 32-byte curve constants from the new header are embedded in its data segments — so the binary CI exercises really is built from this source. The ABI correctly did not need regeneration, since the header change is doc comments only. - On the Solana side the checked name conversion is complete: empty, over-13-byte, out-of-alphabet, and 13th-character-above-the-low-nibble are all rejected, which closes the lossy-conversion collision at that trust boundary.
|
@heifner Thanks again for the re-review. The current coordinated heads address the full finding set and the PR bodies now reflect the required merge/deployment coupling.
Published final heads:
Validation is green on those exact heads:
No new depot or outpost configuration was added; this is still intended for pre-launch disposable coordinated redeployment. |
heifner
left a comment
heifner
left a comment
There was a problem hiding this comment.
Re-review
Re-read at 19601de4f2. The blocking parser-differential finding is genuinely closed — both outposts now carry a real canonical check, correctly ordered, with tests that pin the exact wrapper shape. This is the strongest round so far and most of the previous list is resolved well; the credited items are at the end.
I'm keeping Request Changes, but the character has changed: what remains is one design decision, a correction to the deployment-order framing, and a set of cleanups. Nothing here is a live security hole.
Validation independently verified: Linux, macOS and OPP Bundles green at this head, and the shared E2E pinned exact commit SHAs this time — all three match the heads in the response — 13/13 flows with swap-with-underwriting passing.
1. The newly-signed uw_ext_chain_addr uses a different EVM encoding than every other underwriter address the depot produces
uic_construction_detail.hpp sizes the EVM caller address as fc::crypto::chain_address_size_ethereum and the Ethereum client fills it from get_signer_address() — the 20-byte keccak address.
Every other depot-produced ChainAddress for an underwriter on EVM is the 33-byte compressed key. try_build_swap_remit sets remit.underwriter.address = sysio::pubkey_to_bytes(it->pub_key); pubkey_to_bytes is documented as producing the key bytes so they "can be packed into an opp::types::ChainAddress.address field"; and sysio.msgch::public_key_from_op_address, its declared inverse, gates EVM on if (bytes.size() != 33) return pk;.
The comment immediately above that assignment says the destination outpost "cross-references it against the UNDERWRITE_INTENT_COMMIT it already saw", and the README edited in this round repeats the claim. For an EVM destination those two depot-produced values for one identity on one chain can never be byte-equal — 20 bytes against 33. queue_swap_remit runs after reserv::applyswap and after both locks are written, so if anything ever does perform that cross-reference, the rejection lands post-settlement.
This is invisible today for two compounding reasons: on SVM both forms are the same 32-byte Ed25519 key, so they agree; and the only end-to-end test touched this round uses a SOL destination. Nothing in this repo reads uw_ext_chain_addr at all — dispatch_underwrite_commit decodes only uw_account, chain_code, token_code and reserve_code — so the new signed field currently has no in-repo consumer and the only assertions on it are on its length.
Either the cross-reference is aspirational, in which case the comment and README should stop asserting it, or it is real and the two encodings need to agree. Worth an explicit decision rather than leaving a signed field whose meaning differs by chain from its documented counterpart.
2. The deployment-order constraint is a liveness requirement, and both companion PR bodies state the weaker version
Both bodies say the outposts "should land together with, or after, the depot/sysio PR because the depot canonical/signature validation closes the cross-outpost parser differential for the whole system" — i.e. framed as defence in depth.
The binding constraint is stronger. Before this round the daemon deliberately left the address empty, and the comment removed here explains why that was thought safe: "The address bytes vector stays empty — empty containers are skipped by BOTH encoders, so that's safe." That is true of host protoc and CDT zpp, but not of the generated outpost encoders, which emit every declared field unconditionally. So a kind-only UIC serializes as 12 02 08 02 host-side while ChainAddressCodec.encode produces a four-byte body — and every honest commit would fail the outposts' new equality check.
Outpost-before-depot is therefore a total underwriting outage, not a weakening. The caller-address signing in this round is precisely what makes the outpost check satisfiable. Please restate the coupling in both bodies in liveness terms so nobody reorders on the belief that it only costs defence in depth.
3. Solana ships a different model version than the one the description says was validated
programs/liqsol-core/Cargo.toml declares wire-opp-solana-models = { version = "*", registry = "wire", features = ["borsh"] }, and Cargo.lock at the published head resolves 1.0.38. The PR body cites 1.0.45. The Ethereum side is ^1.0.39, pinned in practice only by its lockfile.
That matters more than usual now: the outpost's generated encoder is the definition of canonical, and the property making the outpost check sufficient is that the outpost model's UIC field set stays a subset of the depot's. A wildcard on that exact artifact is the one way that invariant breaks silently. Recommend an exact pin on both, tied to the depot's proto revision, and either bumping the lock or correcting the claimed version.
4. The required gate is fail-open when the test does not exist
ctest --test-dir build --output-on-failure -R '^test_uic_cross_generator$'
ctest exits 0 when the regex matches nothing — confirmed against the local toolchain, which prints No tests were found!!! and returns success. The target is created only under if(TARGET ${TARGET_CDT_MODELS_NAME}), and every FATAL_ERROR guard added here is itself conditioned on BUILD_UIC_CROSS_GENERATOR_TEST. So the whole gate hangs on one flag surviving the chain from workflow env through build-sysio.sh to the CMake option; break that link and configure succeeds, no guard fires, the target is absent, ctest returns 0, and all-passing goes green having verified nothing. --no-tests=error closes it.
5. There are now four definitions of "canonical" and only one pair is tested
test_uic_cross_generator.cpp compares host protoc against CDT zpp. The Solidity and Rust generated encoders are a third and fourth definition and are compared against nothing. The property the whole coupling rests on — that the daemon's bytes are simultaneously depot-canonical and outpost-canonical — has no regression test.
The three encoders agree only on the subset where every scalar is non-zero and every string and bytes field is non-empty, because host protoc omits proto3 defaults, zpp emits scalars and nested messages but skips empty containers, and the outpost encoders emit everything. Production UICs sit inside that subset, and every divergence is fail-closed in both directions, so this is availability rather than a hole. But a future field that can legitimately be empty or zero would silently break honest commits at one side, and both outpost suites would still pass because their fixtures build non-default values. A shared golden byte vector for one production UIC asserted in all four generators would pin it; minimally, extend the cross-generator test with the outpost expected bytes, which are trivially derivable since the outpost form is "all seven fields, always".
6. Retryability removed a cost rail on the consensus path
Making bond and destination-authex failures retryable is right, and I verified key-rotation durability is intact — validate_candidate_uic still disqualifies and runs before the bond check, replaying stale bytes fails verification with no mutation, and re-signing either leg immediately revalidates the other still-stale leg into a durable DISQUALIFIED. Signature failures did not become retryable.
What was lost is the existing_it->status == UNDERWRITE_STATUS_DISQUALIFIED early return, which sat at the very front of rcrdcommit — ahead of the leg-size check, the decode and canonical re-encode, key recovery, both permission reads, pack_size over a row that can reach the row cap, and the whole-row modify. An underwriter that failed the bond check used to pay that once; now it pays on every replay, plus try_select_winner's mirror scans, quote and reserve lookups. Since these records are dispatched inside the consensus-applied envelope transaction that every node executes, an ACTIVE underwriter holding no bond can repeat that cycle at roughly 290 records per full envelope. A non-durable "last attempt failed" marker or a bounded retry counter on commit_entry would restore the early-out without re-arming durability.
7. The provider predicate now narrows configuration in the other direction
The allowlist mechanism is correct — defaults are created through create_provider and never marked, and a name collision fails loudly at set_provider's duplicate assert rather than mis-classifying. But producer_plugin loads every chain_key_type_wire provider as a block-signing key, supplied through the same option and commonly in the anonymous form. A node configured with a UIC provider plus a wire-target block-signing provider now yields two operator-configured wire providers and aborts preflight with "expected exactly 1 WIRE UIC signature provider, got 2". The README documents the new selection rule but not that an underwriter node may now carry at most one wire-target --signature-provider entry in total. Worth stating explicitly, since it is config-breaking in the opposite direction from the bug it fixed.
8. Two unrelated contract binaries changed with no source basis
contracts/sysio.authex/sysio.authex.wasm and contracts/sysio.chalg/sysio.chalg.wasm are byte-identical from the PR base through the previous head and changed only in this round, while no source file under either directory changed anywhere in the PR — the base-to-head diff for those trees is the .wasm alone.
They are semantically identical: zero differences in the string tables, byte-identical data, table, memory, global and export sections, identical export sets including the apply index, and an identical import set in a different order. That signature is non-deterministic wasm-ld output, not a source-driven change. Recommend reverting both to the base blobs rather than shipping two unreviewable recompiled contracts in a security PR.
9. The roster-gate test is still vacuous, on the third attempt
rcrdcommit_matched_leg_non_underwriter_names_do_not_grow_row now builds a real signed UIC instead of eight zero bytes, which addresses the previous objection — but alice, bob and carol are never created as chain accounts. The fixture's create_accounts list does not include them and neither bootstrap helper adds them, so with the roster gate deleted control reaches verify_uic_signature, get_permission returns nothing for a non-existent account, and the result is unauthorized_key with no mutation. commits_by stays empty and the assertion still holds. Only the rejection reason moved. Adding create_accounts({"alice"_n, "bob"_n, "carol"_n}); makes the roster gate the sole rejecting condition.
Smaller items
- The shape that motivated the caller-address change is characterized nowhere. The commit message justifies it by host protobuf omitting empty byte fields where the outpost encoders emit them, but no test covers
kind = EVM, address = "". Revertcreate_signed_uic_bytesto the old kind-only shape and nothing fails. One line in the cross-generator default-shapes case closes it. test_outpost_client_interface.cpp's new assertions test the file's own mock.minimal_outpost_clienthardcodes the 20/32 length being asserted, so no production code is exercised. The Ethereum and Solana plugin tests are legitimate wiring checks by comparison.uic_provider_selection_ignores_default_for_explicit_k1/_r1still overclaim. Both passautomatic_default_coexists = false, so the filter is never exercised and the trailing emptiness assertion is over an empty vector. The EM/ED variants do cover the feature; renaming these two to what they actually prove — that a WIRE-native operator provider suppresses the default in its bucket — would close it.repeated_unknown's expected disposition rides on a parity coincidence.0x48is both a complete key varint and a complete value byte, so the filler parses as pairs only while2048 - honest.size()stays even. Any width change to a UIC field flips it odd, the trailing byte becomes a truncated varint, and the case silently degrades tomalformed_uic— testing the old, wrong thing. Appending explicit two-byte pairs plus an assertion that the padded payload decodes equal to the honest one would make the intent enforced rather than incidental.push_tracedid not move to the shared tester. The consolidation happened inside the dispatch tests only;pushstill has six near-identical copies across the contract test files andpush_traceis a seventh variant.make_high_s_alternategenuinely is shared now, which is the more valuable half.ProtobufRuntime._decode_varintis unguarded on a now-security-relevant path. Nopos < data.lengthcheck and no mask on the accumulator, so a final byte at shift 63 sets bits above 64 in auint64return. Every such input is still rejected by the equality check and the library ispure, so I found no exploitable consequence — but the fix belongs in the model generator now that this path gates commits.uw_ext_chain_addris signed but never verified by either outpost. Now that it is guaranteed present and the caller is already authenticated, binding it would be nearly free and would turn an encoding artifact into an actual assertion.- Minor:
set_uic_authenticated_callerdocuments its return but none of its parameters, and the reworded comment block intry_select_winneris indented six spaces mid-paragraph against three elsewhere.
Verified correct
- The parser differential is genuinely closed at both outposts. Ethereum compares
keccak256(UnderwriteIntentCommitCodec.encode(uic))againstkeccak256(uicBytes); Solana comparesuic.encode() == uic_bytes. Both are complete comparisons, both run before any queue or buffer mutation, and both sit ahead of the identity binding. Both suites build the exact wrapper shape — attacker prefix plus an unknown length-delimited field wrapping a victim UIC — using the caller's own roster name, so only the canonical check can produce the asserted error, and both assert the queue depth is unchanged. - No input survives outpost-canonical while still parsing differently at the depot, given that the outpost model's field set is a subset of the depot's. An outpost-canonical blob contains exactly the seven declared field numbers, so zpp never reaches its non-consuming unknown-field branch. That invariant holds today across the proto, both generated models and the CDT output.
- The CI gate is real and required. The new job carries no conditional, runs on pull requests and master pushes, installs CDT unconditionally, and is decoupled from
BUILD_SYSTEM_CONTRACTSby the new flag. It is inall-passing'sneedsand its failure condition, and withif: always()a skipped gate also fails the required check. The skip path now emits a warning naming the test and the flag rather than dropping it silently. The CDT include tree is off the broad interface target and scopedSYSTEM PRIVATEto the gate target alone. - The coredump-policy commit is legitimate. The check keyed on the first
container:block and the new job displaced it; the fix replaces the positional marker with the identifying one. Both whole-file assertions are untouched, and the failure modes stay fail-closed. - The anonymous-provider fix is correct and well covered, with positive, negative and default-exclusion cases that each detect a revert.
- Variant-index pinning now works. The vacuous type-identity asserts were removed with a comment explaining why they could not distinguish K1/R1/EM, and the replacement drives fixed known-good vectors per variant through to
CONFIRMEDwith the expected winner — a permuted index recovers a differently-typed key, fails the permission match, and fails the test. - The canonical check finally has genuinely newly-blocked inputs: a filler run to the leg cap that decodes to an identical message, and the parser-differential wrapper. Both are rejected solely by the equality check, and the previously misleading comment was corrected.
sysio.uwrit.wasmis a genuine rebuild — the strings the source diff removed are absent and the ones it added are present, with section-level deltas matching. The ABI correctly needed no regeneration.- The Solana checked name conversion remains complete, and the roster-generation freshness check on Ethereum still fails closed at generation zero.
|
@heifner The current three heads incorporate the last re-review findings plus the subsequent pre-launch scope reduction:
Published and locally validated heads:
Validation passed: isolated full Release platform build, 582 depot contract tests, 12/12 generated Rust tests, 3/3 generated Solidity tests, 12/12 focused Ethereum tests, 261/261 Solana library tests against the exact generated sysio model, artifact parity, and 13/13 local platform E2E flows. Current-head CI is running, and the exact-head remote Release E2E is https://github.com/Wire-Network/wire-platform-build-system/actions/runs/31126193851. |
heifner
left a comment
heifner
left a comment
There was a problem hiding this comment.
Re-review — three repos
Re-read at wire-sysio@3fbcea73da, wire-ethereum@534f76b337, wire-solana@e51b03a5.
Most of the previous round is genuinely resolved, and the four-generator canonical gate is resolved better than I asked for. But the scope reduction deleted the Solana-side regression tests for the parser-differential guard — the highest-severity issue in this series, and one I explicitly credited as closed. Two earlier findings also went backwards.
Keeping Request Changes, on items 1 and 2 only. Both are small.
Closed, verified
1 — uw_ext_chain_addr encoding. Resolved by taking the "the cross-reference is aspirational" branch and saying so at the schema level: attestations.proto now documents uw_ext_chain_addr as the authenticated caller (20/32 bytes) and SwapRemit.underwriter as the authex settlement key, explicitly "not the originating transaction caller." The misleading claim is gone from both the try_build_swap_remit comment and the README. That is the explicit decision I asked for, recorded where both consumers see it.
5 — four definitions of canonical. Fully closed, and well. Sealed golden vectors (EVM + SVM) carrying exact full_hex, signing digest, signature and sha256, asserted byte-identically in all four generators — I diffed the hex across test_uic_cross_generator.cpp, uic_cross_generator_rust.rs and UICCrossGenerator.t.sol and they match. sealed_record_checksum over (vector_id, schema_revision, full_hex, full_sha256) is a good addition: an accidental edit fails loudly instead of silently re-baselining.
7 — provider predicate. Fixed better than documented-around. Selection now requires the provider key to be a direct key of the underwriter's current owner/active permission, which excludes block-signing providers structurally rather than by convention, and multiple qualifying providers fail closed as ambiguous.
8 — unrelated wasm. Confirmed by blob OID: sysio.authex.wasm and sysio.chalg.wasm at head are identical to both the merge base and master; only sysio.uwrit.wasm differs. Matches the claim exactly.
9 — vacuous roster test. create_accounts({"alice"_n, "bob"_n, "carol"_n}) added; the roster gate is now the sole rejecting condition.
Smaller items. Empty-address shape characterized, and correctly annotated as agreeing host/CDT but not a four-encoder production shape. repeated_unknown rebuilt from explicit two-byte pairs with a decode-equality assertion. push/push_trace consolidated into contract_test_support.hpp.
The varint guards are correct. I checked the Solidity assembly rather than taking it on faith: end derives from mload(data), the lt(ptr,end) check precedes every mload, and moving shift := add(shift, 7) after the break is what makes eq(shift, 63) land on byte index 9 rather than one past it. Rust mirrors it. Both cover truncation and overflow.
Both outposts' binding logic is sound. Ethereum: canonical check, then keccak(address_) == keccak(abi.encodePacked(msg.sender)), then roster resolution requiring entryGeneration == rosterGeneration, nonzero address, rosterAddress == msg.sender, UNDERWRITER, ACTIVE — all before _sendAttestation. Solana: signer pre-gate before decoding hostile bytes, canonical check, address bound to the signer, then resolve-by-claimed-name-then-bind, which is the right order for the shared-signer case. checked_wire_name_to_uint64 rejecting trailing-dot aliases closes a real gap: abc. and abc encode identically, so without it two textually distinct signed UICs map to one identity.
I verified the assumption Ethereum's generation design rests on: sysio.epoch.cpp:628 builds the OPERATORS attestation as a full roster every epoch, so bumping the generation on each application cannot strand an account. Solana needs no equivalent because handle_operators does registry.operators.clear() and repopulates.
Regressed since the last round
1. MEDIUM — the Solana negative tests for the canonical check were deleted
ff5e6cfe63 ("Reduce WIRE-291 validation scope") removed roughly 1060 lines of Solana tests. At e51b03a5, git grep finds no test anywhere in wire-solana referencing NonCanonicalUnderwriteIntentCommit (6066) or UnderwriterExternalAddressMismatch (6067); the only hits are the IDL-patch script's code-range bookkeeping.
Last round I wrote, and credited: "Both suites build the exact wrapper shape — attacker prefix plus an unknown length-delimited field wrapping a victim UIC — using the caller's own roster name, so only the canonical check can produce the asserted error, and both assert the queue depth is unchanged." The Ethereum half survives (commit() rejects malformed, absent, and cross-account claims before queue mutation). The Solana half is gone.
"Retains only its consumer-level canonical fixture" in practice means a positive-only test: opp.test.ts builds a well-formed UIC and asserts the happy path relays it verbatim. Nothing exercises rejection.
deposit.rs:313-322 is still correct, so this is a coverage regression rather than a live hole. But it is exactly the regression test for the highest-severity finding of this series, deleted after being credited, on the one side with no other negative coverage. Restoring the wrapper case costs a few lines.
2. MEDIUM — the merge-ordering constraint is now documented nowhere
Last round both companion bodies carried the weak version ("should land together with, or after … because the depot canonical/signature validation closes the parser differential"), and I asked for it restated in liveness terms. Both bodies have instead dropped the statement entirely — they now list companion PRs with no coupling at all.
The constraint is unchanged and still real: the outposts require encode(decode(bytes)) == bytes under generated codecs that emit every declared field, so outpost-new against daemon-old — empty uw_ext_chain_addr, omitted by host protoc — reverts every honest commit. The safe order is wire-sysio first or simultaneous; the reverse is a total underwriting outage.
"Disposable pre-launch redeployment" answers the deploy half but not the merge half. These are three repos with independent CI, and the e2e gate builds branch combinations; merging an outpost first makes master-against-master fail with an opaque revert.
3. LOW — the model version pin is unchanged, and the new gate does not cover it
Still wire-opp-solana-models = { version = "*", registry = "wire" } with Cargo.lock at 1.0.38. Ethereum is ^1.0.39, lockfile-pinned to 1.0.39.
This is sharper now than when I first raised it. The new four-generator gate compiles build/opp/solana/src and remaps Solidity to build/opp/solidity — the freshly generated models from the depot's proto. Both outposts compile against published packages the gate never touches. So the gate proves the generator is self-consistent; it does not prove the deployed outposts use a model consistent with the depot. The npm caret is at least lockfile-pinned; the Cargo wildcard can move on any cargo update.
4. LOW — the gate's hard guard was removed along with the job
The specific mechanism from my previous finding is fixed: the ctest -R line that returned 0 on no match is deleted. And the host/CDT test does genuinely run on PRs — SYSIO_BUILD_SYSTEM_CONTRACTS is OFF for non-tag refs and CDT is installed on ubuntu24, so TARGET opp_cdt_models AND NOT BUILD_SYSTEM_CONTRACTS holds and the target registers under normal CTest.
What is gone is any assertion that it exists. BUILD_UIC_CROSS_GENERATOR_TEST and every FATAL_ERROR guard were deleted; if the CDT install step ever yields an unusable toolchain, CMake takes the elseif(ENABLE_TESTS) message(WARNING …) path, configure succeeds, the target is absent, and the job goes green having verified nothing. The new run-uic-cross-generator-tests.sh is properly fail-closed — missing rustc, forge, or generated models each exit 1. The host/CDT half is the one now without a backstop. A single ctest … -R '^test_uic_cross_generator$' --no-tests=error step on ubuntu24 restores it.
New
MEDIUM — durable disqualification now swallows a genuinely transient condition
retryable was replaced wholesale by disqualified, so insufficient collateral now calls disqualify_candidate durably (sysio.uwrit.cpp:1454).
That restores the cost rail I asked for, but it goes past what I proposed — I suggested "a non-durable 'last attempt failed' marker or a bounded retry counter … without re-arming durability", and last round I affirmatively verified that "making bond and destination-authex failures retryable is right."
Missing destination authex is a configuration error, so durable disqualification there is defensible. Collateral is not: available_via_mirrors is balance minus active locks, so an underwriter with capital locked by another in-flight race is durably removed from this one and cannot recover for that request even seconds later when the lock clears. That is normal high-utilization operation, not misconfiguration, and it quietly advantages idle underwriters over busy ones. Worth splitting the two cases, or gating disqualification on a bounded retry count.
LOW — Ethereum roster stamping adds a per-epoch, roster-sized SSTORE cost
operatorRosterEntryGeneration[accountKey] = rosterGeneration writes a changing value for every operator on every OPERATORS application — a guaranteed nonzero-to-nonzero SSTORE (~2900 gas) per operator per epoch, where type/status/address are usually same-value writes at ~100. The enclosing loop was already O(roster), but its marginal cost was near zero; this makes it linear and recurring. At a few hundred operators that is several hundred thousand gas added to every epochIn, paid by the submitting batch operator, and epochIn failing is an OPP-halt class event. Fine at launch scale — flagging so it is not discovered at scale.
Nit — run-uic-cross-generator-tests.sh ignores its own build-dir argument for model discovery
The script takes BUILD_DIR="${1:-$REPO_ROOT/build}" and uses it for output paths, but pins SOLANA_MODELS / SOLIDITY_MODELS to $REPO_ROOT/build/opp/... unconditionally. In CI the two coincide. A developer following this repo's build/claude convention gets either a confusing "Generated OPP models are missing" or a silent test of the stale build/ models while believing they tested build/claude.
Reviewed statically across all three heads. I did not rebuild; the isolated Release platform build, 13/13 local E2E, and current-head CI cover that.
Reject malformed, noncanonical, unsupported, and unauthorized UIC evidence before candidate state mutation. Bind daemon callers and pin canonical generator behavior across supported fixed-size signature variants. Refs: WIRE-291 Change-Id: I2d69c644c60c9ddeaf670e5268f7e8ca1a7cef16
Change-Id: Iaad6430d10b5953c914cff87b37baabb0ccfbf6d
Change-Id: I8cb726e218aebd6713f86656a4320e9e2eab58b4
Change-Id: I1f999934c70c8c21b42ed39700ad5da4f01184df
Change-Id: Iae518a8e431df01ab3e14cab78e7c078f0456910
Change-Id: Ie2f12a718cc399e20b326809c5d95e0d4d6169ce
Change-Id: I2f9031e9b6cd30960c4b244115bb50d1d897fce3
Change-Id: If9a3d11f3763c8fde6f4708a0f86095432c1b3cb
Change-Id: I2cea4c7e79f14ba147d4bbfdd223df10a343810f
Change-Id: I8d216b30d045a407d722ecef441857cf51dd341d
Change-Id: Ic2fb79b851904aca3d996b08bcca7415d6a61663
Change-Id: I4b05c0bfd30364d44b8ea68b8c5c22606274b6fb
Change-Id: Ibd0433bc921d7e3fd22377928620c25d4feb2d4f
Change-Id: I90f0a02a7e967b4f175252033d544d5e8b834cbd
Change-Id: I51b2e5eaf1a5acd8dc654ef4ddf672ccd6ab4c9b
Change-Id: I382eb048eca3293e18c3db075c6efdc2e75af7a9
Change-Id: I337ced37f2cae16e6f646ad0a0cd5cbfbd8a4b05
Change-Id: I6ebb8372a90af56185669e287a66e591aef50d7d
Change-Id: I6bb53d165fd9bbd1e6ce0e4a9d050a4b7975c0bf
Change-Id: I6b3cbfe818cd288d51f5cd968e43322bce3e6f15
Change-Id: I335887771341a88d54e22f4d9004a95e33a01aba
Change-Id: If8b31207fe0f03a651b979899fccb83efa3e7a18
Change-Id: I53322ceef1166937bf06109f521710700c810cc1
Change-Id: I28694ce85da88ba2bc8646d2f49964d9e71e403c
Change-Id: I962360549b08396133a5c4e83ed2d1a36996a77e
Change-Id: I56ddc64d81f0c7476426b9825257d950fe1fed04
Change-Id: I3e6d7d6d62f78d4179de3b4e515966d2fe863328
Change-Id: I60a8566de97d1bf25a8990064460c383cfcc0203
3fbcea7 to
c0b3e57
Compare
Change-Id: I7037108df23abe444abbd1f743fc47d108005f5a
heifner
left a comment
heifner
left a comment
There was a problem hiding this comment.
Code review — 5 findings
Reviewed all 63 files (+5296/−929) against merge-base 03b610de.
Theme of the three medium findings: this PR converts several transient or operator-fixable conditions in try_select_winner from "log + skip, row stays PENDING" into terminal reject+refund or durable DISQUALIFIED. Each is individually defensible as fail-fast, but together they remove the recovery paths the previous code documented — and the reserve-liquidity one is externally triggerable by any bonded underwriter.
Verified correct (the risky parts)
- ECDSA canonicality policy (
uic_signature_canonical.hpp) matches libfc exactly: K1/R1 prefix recovery base 31 (elliptic_impl_priv.cpp:86,elliptic_r1.cpp:169), EM suffix base 27 (elliptic_em.cpp:317), and all four curve-order/half-order constants are byte-correct. All three signers normalize to low-s, so the low-s gate cannot spuriously reject. - Packed-size math (
1+65ECC,1+96ED),magic_enum::enum_castbounds, and the duplicate-alternativeemplace<Index>are sound;ce_ptrpoints into a local copy of the row, so thereqs.modifycalls insidedisqualify_candidatedo not dangle it. dst_amount→target_amountinverify_source_depositis a genuine bug fix — confirmed againstwire-solanaliqsol-coreopp/mod.rs::swap_correlation_hash, which hashestarget_amount.uw_request_idcan never be 0 (mint_att_idusesmax(1, available_primary_key())), so the new byte-exact canonical-encoding check cannot trip on an omitted proto default.push_contract_actionlosing its implicitproduce_block()is safe — all five callers were updated to the correct variant.
| // replay and reserve activation does not call `try_select_winner`. Treat | ||
| // the unprovisioned route as request-global and terminal once a candidate | ||
| // has passed eligibility, signature, and bond checks above. | ||
| reject_and_refund( |
There was a problem hiding this comment.
Terminal reject+refund on a missing/not-ACTIVE reserve — the stated justification does not hold.
The comment claims "a complete one-shot candidate has no later wake-up after this authoritative attempt", but the wake-up path is per-candidate, not per-request: a different underwriter's rcrdcommit on a still-PENDING row calls try_select_winner again. Previously this condition left the row PENDING; now it is permanent.
Scenario: a swap is created against a reserve that is provisioned a few seconds later — exactly the dev/smoke shape createuwreq deliberately admits. The first underwriter to complete both legs now permanently rejects and refunds the user instead of leaving the row PENDING for the reserve to come up.
Suggest reverting this one to log + skip (stay PENDING), or gating the terminal path on the request being near expiry.
| dst_r->reserve_chain_amount < req.dst_amount) { | ||
| sysio::print("try_select_winner: insufficient reserve liquidity for " | ||
| "uwreq ", uwreq_id, ", skipping\n"); | ||
| reject_and_refund( |
There was a problem hiding this comment.
Transient reserve-liquidity shortfall is now a terminal reject+refund (same change at lines 1736 and 1750).
A momentary dip in reserve_chain_amount is not a property of the candidate — it is shared, mutable, cross-request state that recovers on its own. Making it terminal both harms honest users and creates a griefing surface.
Scenario 1 (honest): two swaps draw on the same destination reserve. The first settles and drains reserve_chain_amount below the second's dst_amount. The second user's swap is permanently refunded even though the LP is replenished moments later.
Scenario 2 (griefing): any ACTIVE, adequately-bonded underwriter can terminate another user's swap by timing its commit to a moment of low reserve liquidity — every candidate-local gate above this point passes, so the shortfall check is reached and fires.
Liquidity checks should stay non-terminal (log + skip, row stays PENDING); reserve terminal rejects for conditions that are genuinely properties of the request itself.
| /// Raw UIC payloads are cleared because terminal candidates have no remaining | ||
| /// signature-validation reader; timestamps, outpost ids, status, and reason | ||
| /// remain as the compact audit record. | ||
| void disqualify_candidate(uwrit::uwreqs_t& reqs, const uwrit::id_key& pk, |
There was a problem hiding this comment.
Durable DISQUALIFIED makes operator-fixable, candidate-local failures permanent with no recovery path.
disqualify_candidate now clears both UIC blobs, and rcrdcommit returns early on a DISQUALIFIED entry (line 1983), so no later record can re-arm it. The previous code explicitly documented DISQUALIFIED as reclaimable via a later rcrdcommit.
But the disqualification reasons include states the operator can fix:
- a missing destination-chain
sysio.authexlink (try_build_swap_remit→swap_remit_disp::disqualified) !is_active_underwriter
Scenario: an underwriter commits both legs before its destination-chain authex link exists → DISQUALIFIED forever for that uwreq. After the operator adds the link the underwriter still cannot re-enter, and the plugin additionally marks the candidate skip_candidate and never resubmits. In a single-underwriter deployment the swap can then only expire.
Consider splitting the reasons: permanent for genuinely unrecoverable ones (bad signature, malformed UIC), reclaimable for the operator-fixable ones.
| // Complete candidates have already received their one authoritative | ||
| // winner-selection attempt; terminal candidates are also durable. Neither | ||
| // may enter cover selection or replay a paid outpost transaction. | ||
| const auto skipped_candidates = std::erase_if( |
There was a problem hiding this comment.
skip_candidate requests are erased here, before the provisional-capacity reservation — contradicting the invariant the second pass exists to enforce.
The comment at the second erase_if (line 1141) says it reserves winner-time collateral for "candidates whose paid legs are all stored or locally confirmed", but plan_stored_commits sets skip_candidate = true precisely for the depot-stored-complete case (intent_submitted && source_complete && destination_complete). Those requests are removed here and never reach reserve_buckets at line 1157, so only the locally-confirmed-but-not-yet-depot-observed subset is actually budgeted.
Low severity — the state looks close to unreachable today (try_select_winner runs synchronously inside rcrdcommit, so a complete INTENT_SUBMITTED candidate on a PENDING row requires the stored-SwapRequest-decode-failure path). Flagging it as a latent inconsistency between the comment's stated invariant and the code, rather than a live overcommit.
|
|
||
| message(STATUS "CDT_ROOT is set to: ${CDT_ROOT}") | ||
|
|
||
| if(NOT CDT_BUILD AND (BUILD_SYSTEM_CONTRACTS OR (ENABLE_TESTS AND CDT_ROOT))) |
There was a problem hiding this comment.
This find_package(cdt REQUIRED) gate can break local configures that previously worked.
CDT_BUILD changed from a cmake_dependent_option (forced ON when BUILD_SYSTEM_CONTRACTS=OFF) to a plain option(... OFF), and this condition — NOT CDT_BUILD AND (BUILD_SYSTEM_CONTRACTS OR (ENABLE_TESTS AND CDT_ROOT)) — now hard-requires an installed cdt CMake package for any host-test build that merely has CDT_ROOT in the environment.
In this monorepo CDT_ROOT is commonly exported (the standing contract-rebuild sequence uses wire-cdt). A developer whose CDT_ROOT points at a build tree without lib/cmake/cdt/cdt-config.cmake now fails at configure time on a BUILD_SYSTEM_CONTRACTS=OFF build that configured fine before. CI is unaffected because install-wire-cdt-package.sh is what exports CDT_ROOT there.
|
Heads up on an overlap: I split the source-deposit hash fix out of this PR into #556. Not a request to change anything here — the reason for splitting is that this one line is a live defect on I lifted your What you'll hit rebasing onto a
Evidence in #556 if useful: recomputing the preimage both ways against a live ETH→SOL swap gives |
2 participants
Summary
(chain, token, reserve)triple before request creation, while continuing to allow distinct cross-chain or distinct-reserve routes.sysio.msgchandsysio.uwritartifacts and align generated Solana and Solidity UIC runtimes with their published consumer models.Root cause
UIC evidence and candidate state could previously be admitted before all canonicality, caller-binding, identity, capacity, and route-identity constraints were established. That allowed malformed or mismatched evidence to reach mutable state and left several winner-time failures without a deterministic terminal outcome.
The follow-up CI failure had two independent causes:
BUILD_SYSTEM_CONTRACTS=ON, while PR CI intentionally tests the tracked system-contract artifacts withBUILD_SYSTEM_CONTRACTS=OFF;Deployment scope
This is pre-launch work for disposable redeployment. It adds no migration, mixed-version compatibility, deployment automation, or new depot/outpost configuration option.
The final follow-up changes only CI orchestration and test reference inputs. It does not change production sources, build inputs, generated contract artifacts, or packaged binaries, so the accepted local Phase A build/artifacts and 13/13 Phase B flow result remain valid.
Validation
swap-with-underwriting.actionlint1.7.7.git diff --checkpassed.d6bf3c5a19fa1a05b544ebaecb3815b3ba1ed442, using wire-tools-ts PR #59 atc884f8979090323ff5fa5610d0d930b2bfd76d72.Current PR heads:
wire-sysio:d6bf3c5a19fa1a05b544ebaecb3815b3ba1ed442wire-ethereum:256e84582f20bac0797fad53731b0617e3dfcc62wire-solana:e8c08554f79e86d6cd0fa5bb4a95c7806aac409bThe local E2E sysio integration checkout differs from this PR head only by the approved release-only BUILD overlay; final review verified patch equivalence.
Companion PRs
Jira
https://wire-network.atlassian.net/browse/WIRE-291