Skip to content

WIRE-352: sweep linked rewards into dclaim - #594

Open
huangminghuang wants to merge 2 commits into
masterfrom
fix/wire-352-authx-dclaim-sweep
Open

WIRE-352: sweep linked rewards into dclaim#594
huangminghuang wants to merge 2 commits into
masterfrom
fix/wire-352-authx-dclaim-sweep

Conversation

@huangminghuang

@huangminghuang huangminghuang commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • make successful AuthEx link creation inline sysio.dclaim::linkswept with the linked Wire account, chain kind, and native address
  • derive the canonical EVM address for signed links and carry the attested Ethereum address through sysio.msgch and sysio.roa for trusted node-owner links
  • preserve idempotent identical-link retries while rejecting conflicting or malformed chain/key/address shapes
  • create, deploy, and privilege sysio.dclaim in the clean integration bootstrap required by the new inline dependency
  • regenerate the affected sysio.authex, sysio.msgch, and sysio.roa ABI/WASM artifacts
  • update focused C++ and Python integration coverage, including EVM/SVM pre-link reward sweeps and identical-link retry sweeping

Coordinated delivery

Merge order: wire-sysio → wire-libraries-ts → wire-tools-ts.

Exact head: 6a57bba2cf7d9f00840ac9db9b15dbb4031f833a.

Backward-compatible rollout

  • the existing three-field recordlink payload remains accepted and does not invoke the new sweep
  • the existing four-field nodeownreg payload remains accepted and emits the legacy inline payload
  • the safe contract deployment order is sysio.dclaim → sysio.authex → sysio.roa → sysio.msgch
  • the safe repository merge order is wire-sysio → wire-libraries-ts → wire-tools-ts

Follow-up cleanup

  • WIRE-386 tracks removal of the temporary compatibility scaffolding after all three coordinated changes have merged and rolled out.

Validation

  • final-content hygiene and CDT SDK freshness passed
  • source and artifact-copy configure/build paths passed
  • source and artifact-copy contract suites passed, including both 701-test suites
  • plugin integration passed with clean bootstrap deploying sysio.dclaim; this directly covers the prior all-matrix nodeownreg_test failure
  • tracked ABI/WASM artifacts match the selected source build byte-for-byte
  • before the libraries generated-type review follow-up, all 14 workflow-selected canonical flows passed in the local platform campaign at libraries head 355a66e7c5d848fd73d20fdff6d385962adcd0e0, with isolated clusters and paired heartbeat monitors
  • exact-head Linux CI passed: https://github.com/Wire-Network/wire-sysio/actions/runs/33575457628
  • exact-head macOS CI passed: https://github.com/Wire-Network/wire-sysio/actions/runs/33575457081
  • before the libraries generated-type review follow-up, coordinated remote E2E passed all 14 workflow-selected flows in Release mode with four-way flow concurrency, using sysio 6a57bba2cf7d9f00840ac9db9b15dbb4031f833a, libraries 355a66e7c5d848fd73d20fdff6d385962adcd0e0, and tools ded16e6824727a9d2635cb974eada12add9034ee: https://github.com/Wire-Network/wire-platform-build-system/actions/runs/33577019573
  • backward-compatibility remote E2E passed all 14 workflow-selected flows in Release mode with four-way flow concurrency, using feature sysio 6a57bba2cf7d9f00840ac9db9b15dbb4031f833a with unchanged libraries origin/master at 133cc7428be513db091f6562f90c88244b3eae71 and unchanged tools origin/master at f36f7175018c75fcad88dc0cc2967aebf35dd645: https://github.com/Wire-Network/wire-platform-build-system/actions/runs/33577023707
  • fresh coordinated platform E2E and independent review receipts are pending for libraries head f43490afe55bdd33348c644d6dbd4fb0778a0f73; this PR does not yet claim delivery readiness

Reviewer notes

The platform campaigns used a detached validation-only Wire Solana origin/next checkout at 9d49cbe92419606667f6a4684714aa742ac5c40b because current sysio master requires the SOL-396 custody token-program schema. That checkout is not part of the WIRE-352 delivery scope.

The compatibility campaign demonstrates that sysio can be deployed first without requiring consumers to adopt the libraries and tools changes simultaneously.

No merge, Jira transition, or cleanup is included in this PR.

Change-Id: Ie64d696fbeb7ffcff325121e463986183f0d4635
@huangminghuang
huangminghuang force-pushed the fix/wire-352-authx-dclaim-sweep branch from f9beb0b to 6a57bba Compare September 2, 2026 00:28
@heifner

heifner commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review — WIRE-352 / WNS-27

The approach is right and matches CertiK's recommendation exactly: sweep inline from both link paths rather than leaving linkswept reachable only by privileged remediation. The split — derive the address on-chain in createlink (where k1_recover_uncompressed gives you the 65-byte key), thread it in for recordlink (where a compressed key can't be decompressed on-chain) — is the correct read of the constraint. Test coverage for the new behaviour is good: EVM, SVM, identical-retry re-sweep, and the createlink path all covered. Removing EmissionsSoakScenarioSteps.planLinkswept in wire-tools-ts#93 is a real win — that harness crank was hiding the missing production behaviour.

Two blocking items, one merge condition, then a set of should-fixes.


Blocking

1. recordlink must stay non-throwing on the OPP inbound path

recordlink's contract is stated in its own doc comment — "idempotent and non-throwing so the trust-OPP depot dispatch is never aborted" — and the PR keeps that wording while adding an unconditional inline send to another contract. That guarantee is now conditional on external bootstrap state. Two concrete throw vectors:

(a) sysio.dclaim account missing. apply_context::execute_inline hard-asserts in the sender's context, before the callee ever runs:

auto* code = control.db().find<account_object, by_name>(a.account);
SYS_ASSERT( code != nullptr, action_validate_exception,
            "inline action's code account {} does not exist", a.account );

This is what the PR body calls "the prior all-matrix nodeownreg_test failure", and it's why four Boost fixtures plus tests/TestHarness/Cluster.py had to grow a sysio.dclaim account. Cheap to close:

void sweep_linked_rewards(...) {
   if (!is_account(dclaim_account)) return;   // never-throw: no dclaim on this cluster
   action(...).send();
}

(sysio::is_account — CDT libraries/sysiolib/contracts/sysio/action.hpp:312.)

(b) sysio.dclaim present but not privileged, and the sweep actually credits a row. dclaim::credit_wire emplaces/modifies with ram_payer = "sysio"_n, and apply_context::validate_account_ram_deltas requires privilege for a non-receiver sysio payer:

if( !privileged && itr->delta > 0 && itr->account != receiver ) {
   SYS_ASSERT( has_authorization( itr->account ), unauthorized_ram_usage_increase, ... );
}

The inline action carries {sysio.authex, active}, not sysio, so the later payer-search fallback doesn't rescue it either. is_account() does not close this one. In practice dclaim is deployed via setsyscode (privileged) and Cluster.py now setPrivs it — but that makes the never-throw guarantee an environmental property rather than a structural one, on the one path where a throw stalls OPP dispatch chain-wide.

Please close (a) with the guard and make a call on (b) — either eliminate the dependency, or make the precondition explicit and enforced (a bootstrap check that fails loudly, plus a corrected doc comment on recordlink naming the new preconditions). "It happens to be privileged everywhere today" isn't a guarantee.

For reference, the third case is benign: an account that exists with no code deployed is a silent no-op for an inline send — exec_one's No contract for action assert only fires when get_sender().empty().

2. A malformed native_address discards the whole link, not just the sweep

sysio.authex.cpp, recordlink:

if (native_address.has_value()) {
   const size_t expected_size = valid_evm ? evm_address_size : svm_address_size;
   if (native_address->size() != expected_size) return;   // returns BEFORE creating the link row
}

A wrong-sized optional auxiliary field silently drops the node-owner link. The size check should guard only sweep_linked_rewards, leaving the link recorded. Unreachable from the live path today (both msgch and roa pre-validate 20 bytes), which is exactly why it would go unnoticed if it ever became reachable.


Merge condition

3. binary_extension is accepted as scaffolding — file its removal

To be clear on the rationale, since the PR body frames this as a rollout property: we have not launched and there is no in-place contract-upgrade path to support, so deployment atomicity is not the justification. The justification that does hold is coordination — needing all three PRs approved and merged simultaneously is genuinely limiting, and the extension buys a green master through that window. On those grounds it can stay.

But it is scaffolding, not design, and it is scaffolding that the standing pre-release rule (.claude/rules/no-back-compat-before-release.md, cutoff 2026-09-09) otherwise forbids outright. Please open a follow-on PR or a Jira issue, linked from this one, to strip it once wire-sysio / wire-libraries-ts / wire-tools-ts are all merged. Scope for that cleanup:

  • make native_address / eth_address required, dropping binary_extension
  • delete the if (eth_address.has_value()) / else fork in roa::nodeownreg that emits two distinct recordlink payloads
  • delete the two has_value() guards in authex::recordlink
  • delete recordlink_accepts_legacy_three_field_payload + recordlink_legacy, and nodeownreg_accepts_legacy_four_field_payload + nodeownreg_legacy
  • delete push_nodeownreg_legacy + Test 1b in nodeownreg_test.py
  • delete the "trailing ABI binary extension … permits the contract release to precede" paragraph in production-bootstrap.md
  • restore the ABI-generated type annotations in wire-tools-ts (below)

That last one is the item I'd most want tracked. wire-tools-ts#93 removes the explicit generated-type annotation in at least four places so the payload satisfies both SDK versions:

-    const data: SysioContracts.SysioRoaNodeownregAction = {
+    // Inferred so this rollout test compiles against both the published
+    // pre-extension SDK type and the upgraded optional-extension type.
+    const data = {

(same pattern in ClusterBuildDefaults.ts, EthereumNodeOwnerNftTool.ts, NodeOwnerNftScenario.ts). Temporarily necessary given the above, but it's a typing hole that will silently outlive the merge window unless it's on a list.

The $ handling added to generate-sysio-contract-types.py should stay either way — that's useful codegen infra independent of this rollout.


Should fix

4. The sysio.authex@sysio.code grant looks redundant

The PR adds this grant to the authex fixture, and to the roa fixture (which then needed a new std::sort for the second co-signer); wire-tools-ts#93 adds "sysio.authex" to Constants.OPP_SYSTEM_ACCOUNTS. But sysio.authex is deployed via setsyscode, i.e. privileged, and execute_inline skips authorization entirely for a privileged sender:

// No need to check authorization if replaying irreversible blocks or contract is privileged
if( !control.skip_auth_check() && !privileged && !trx_context.is_read_only() ) {

Same reasoning that made CertiK WNS-04 a non-issue. Worth dropping the grant and re-running both fixtures to confirm — it removes the sort and a bootstrap step.

5. New silent chain/key gate widens scope beyond the ticket

if (!valid_evm && !valid_svm) return;

recordlink previously accepted any (chain_kind, key) pair; it now silently drops everything else. Probably fine given only EVM/EM is in production use, but it's an undocumented behaviour change riding a sweep PR — please call it out in the doc comment. Also pub_key.index() == 4 is a magic literal sitting directly beside the symbolic fc::crypto::key_type_em; use the named ED constant.

6. Prefer deriving the address in msgch over trusting reg.actor.address

actor_pub_key is the uncompressed 65-byte key (BAR.sol enforces UNCOMPRESSED_PUBKEY_LENGTH / 0x04, and em_pubkey_from_eth_bytes already handles that branch), so dispatch_node_owner_reg can compute the 20-byte address from the key it already validates rather than consuming a second trusted payload field and guarding it. sysio.msgch.cpp already hand-rolls the same SEC1 parity logic a few lines up. One source of truth, one less field an outpost can disagree with itself about.


Nits

  • evm_address_from_uncompressed_key would be better in the CDT next to k1_recover_uncompressed / eip191_hash in chain_conversions.hpp, where msgch can reuse it. Follow-up, since it's cross-repo.
  • sweep_linked_rewards hardcodes permission_level{"sysio.authex"_n, "active"_n}; the rest of the contract uses get_self().

Verified, no action

  • createlink's switch from recover_key to k1_recover_uncompressed + manual SEC1 compression is equivalent (0x02 | (Y[31] & 1)); the stored verified_pub_key is unchanged in meaning.
  • SVM native_address = pubkey_to_bytes case 4 → raw 32 ED bytes, matching what importseed / onreward park.
  • The new msgch guard is satisfied by the real emitter: BAR.sol populates actor with ChainAddress(EVM, msg.sender) and derives that same address from depositorPubKey, so key and address are the same identity.
  • createlink is EVM/SVM-only, so native_address is never empty there.
  • All three .wasm changes ride real source changes in the same commit.
  • sysio.dclaim itself is untouched — no artifact churn.

Change-Id: I5e5dd088379fb8f58d20b4d2fa5023a186200840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants