feat: Add optional payload hash to foreign transaction requests

[netrome]

closes #3997

Supersedes #4008 which was closed during a hack

[Copilot]

Pull request overview

This PR addresses audit issue #3997 by adding an optional expected_payload_hash to foreign transaction verification requests, allowing the contract (and nodes) to reject replayed/stale responses whose signed payload_hash does not match the request’s expectation. It updates the contract-interface DTOs, node request plumbing + signing behavior, SDK request builder behavior, and adds/updates documentation and tests (unit, sandbox, and e2e) to validate the new binding semantics.

Changes:

• Add expected_payload_hash: Option<Hash256> to VerifyForeignTransactionRequestArgs / VerifyForeignTransactionRequest, with JSON omission when None.
• Enforce request/response binding:
  • Contract: respond_verify_foreign_tx rejects mismatching payload hashes.
  • Node: refuses to sign when computed hash differs from request expectation.
• Update SDK builder to compute and populate expected_payload_hash, and extend tests/docs/e2e to cover hash-bound requests.

Reviewed changes

Copilot reviewed 36 out of 37 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
docs/foreign-chain-transactions.md	Documents the new optional expected_payload_hash binding and updated failure behavior.
crates/node/src/types.rs	Threads expected_payload_hash through the node’s internal VerifyForeignTxRequest.
crates/node/src/tests.rs	Updates node test helper to set expected_payload_hash explicitly.
crates/node/src/providers/verify_foreign_tx/sign.rs	Rejects signing when computed payload hash doesn’t match the request’s expected hash; adds unit tests.
crates/node/src/indexer/types.rs	Passes expected_payload_hash through indexer respond-args construction.
crates/node/src/indexer/handler.rs	Plumbs expected_payload_hash out of on-chain args into node request handling.
crates/near-mpc-sdk/tests/starknet.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/tests/polygon.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/tests/hyper_evm.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/tests/bnb.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/tests/bitcoin.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/tests/base.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/tests/arbitrum.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/tests/abstract_chain.rs	Adjusts SDK tests for new fallible builder return type (build().unwrap()).
crates/near-mpc-sdk/src/foreign_chain/starknet.rs	Updates expected request args to include expected_payload_hash computed from expected extracted values.
crates/near-mpc-sdk/src/foreign_chain/polygon.rs	Updates chain-specific tests to unwrap the now-fallible builder.
crates/near-mpc-sdk/src/foreign_chain/hyper_evm.rs	Updates chain-specific tests to unwrap the now-fallible builder.
crates/near-mpc-sdk/src/foreign_chain/evm.rs	Updates expected request args to include expected_payload_hash computed from expected extracted values.
crates/near-mpc-sdk/src/foreign_chain/bnb.rs	Updates chain-specific tests to unwrap the now-fallible builder.
crates/near-mpc-sdk/src/foreign_chain/bitcoin.rs	Updates expected request args to include expected_payload_hash computed from expected extracted values.
crates/near-mpc-sdk/src/foreign_chain/base.rs	Updates chain-specific tests to unwrap the now-fallible builder.
crates/near-mpc-sdk/src/foreign_chain/arbitrum.rs	Updates chain-specific tests to unwrap the now-fallible builder.
crates/near-mpc-sdk/src/foreign_chain/abstract_chain.rs	Updates chain-specific tests to unwrap the now-fallible builder.
crates/near-mpc-sdk/src/foreign_chain.rs	Refactors payload-hash computation into a helper, makes builder return io::Result, and sets expected_payload_hash in request args.
crates/near-mpc-contract-interface/src/types/foreign_chain.rs	Adds expected_payload_hash fields to DTOs and adds a ForeignTxSignPayload::new(...) constructor.
crates/near-mpc-contract-interface/src/snapshots/near_mpc_contract_interface__client__tests__mpc_contract_handle__should_match_the_wire_format_catalog.snap	Updates wire-format snapshot to include the new optional field when present.
crates/near-mpc-contract-interface/src/client.rs	Extends client wire-format catalog test to cover Some/None serialization behavior for expected_payload_hash.
crates/e2e-tests/tests/foreign_chain_tx_validation.rs	Adds an e2e scenario using the SDK builder to submit a hash-bound request and asserts response hash matches expectation.
crates/e2e-tests/Cargo.toml	Adds near-mpc-sdk dependency for e2e tests that build requests via the SDK.
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap	Updates contract ABI snapshot with the new optional field in request DTOs.
crates/contract/tests/sandbox/foreign_chain_request.rs	Adds sandbox tests asserting the contract rejects mismatched payload hashes and accepts matching ones when bound.
crates/contract/src/v3_13_0_state.rs	Migration logic updated to use a new storage key for pending verify-foreign-tx requests and documents abandoned in-flight entries.
crates/contract/src/storage_keys.rs	Deprecates V2 storage key and adds PendingVerifyForeignTxRequestsV3.
crates/contract/src/lib.rs	Enforces expected payload hash check in respond_verify_foreign_tx; updates init storage key.
crates/contract/src/errors.rs	Adds RespondError::UnexpectedPayloadHash.
crates/contract/src/dto_mapping.rs	Maps expected_payload_hash from args into on-chain request struct.
Cargo.lock	Captures dependency graph changes (notably adding near-mpc-sdk to e2e-tests).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Pull request overview

Adds an optional expected_payload_hash to foreign-transaction verification requests so callers can bind a response to the exact payload they expect. This closes the ToB finding (#3997): respond_verify_foreign_tx previously accepted any root-key-signed (payload_hash, signature) pair, so a stale response could resolve — and thereby DoS — a pending request. The contract now rejects a response whose payload_hash differs from the request's expectation, nodes refuse to sign a payload whose hash differs, and the near-mpc-sdk builder populates the field automatically. Because the field lands inside the pending-request LookupMap key, the map moves to a fresh storage prefix and requests in flight at upgrade time are abandoned.

Changes:

• Contract: new RespondError::UnexpectedPayloadHash, checked in respond_verify_foreign_tx before signature verification.
• Contract state: PendingVerifyForeignTxRequestsV2 renamed to _Deprecated… and PendingVerifyForeignTxRequestsV3 appended (append-only rule respected); the 3.13.0 migration starts an empty V3 map.
• Interface DTOs: expected_payload_hash: Option<Hash256> on VerifyForeignTransactionRequest{,Args} with #[serde(default, skip_serializing_if = "Option::is_none")] so the wire format is byte-identical when unset; new ForeignTxSignPayload::new.
• Node: field threaded through indexer args, VerifyForeignTxRequest (JSON-persisted in RocksDB — missing field reads back as None, so no DB migration needed), and respond args; build_signature_request bails on mismatch for both leader and follower.
• SDK: ForeignChainRequestBuilder::build() now returns io::Result<…> and binds the request to the hash derived from the caller's expected values.
• Tests: contract unit + sandbox accept/reject cases, node unit tests, an e2e case submitting an SDK-built hash-bound request, plus wire-format and ABI snapshots.
• Docs: new expected_payload_hash section in docs/foreign-chain-transactions.md (also drops the stale derivation_path/tweak fields from the DTO snippets).

Reviewed changes

Per-file summary

File	Description
crates/contract/src/lib.rs	Hash check in respond_verify_foreign_tx; V3 storage prefix in both constructors; two new unit tests + sign_foreign_tx_payload helper
crates/contract/src/errors.rs	New RespondError::UnexpectedPayloadHash
crates/contract/src/dto_mapping.rs	Maps the new field from args to request
crates/contract/src/storage_keys.rs	V2 key renamed to _Deprecated…, V3 appended
crates/contract/src/v3_13_0_state.rs	Migration abandons the V2 pending map, starts an empty V3 map
crates/contract/tests/sandbox/foreign_chain_request.rs	Sandbox accept/reject tests for the bound hash; existing cases pass None
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap	New optional ABI property (not in required)
crates/near-mpc-contract-interface/src/types/foreign_chain.rs	New DTO field + ForeignTxSignPayload::new
crates/near-mpc-contract-interface/src/client.rs, snapshots/…wire_format_catalog.snap	Wire-format catalog covers both Some and omitted-None cases
crates/near-mpc-sdk/src/foreign_chain.rs	expected_payload_hash() helper; build() returns io::Result and sets the expectation
crates/near-mpc-sdk/src/foreign_chain/{bitcoin,evm,starknet}.rs	Tests assert the built request carries the expected hash
crates/near-mpc-sdk/src/foreign_chain/{abstract_chain,arbitrum,base,bnb,hyper_evm,polygon}.rs, crates/near-mpc-sdk/tests/*.rs	Mechanical .build().unwrap() updates
crates/node/src/types.rs, indexer/types.rs, indexer/handler.rs, tests.rs	Field threaded through node request types and respond args
crates/node/src/providers/verify_foreign_tx/sign.rs	build_signature_request rejects a payload whose hash differs; three new unit tests; helper functions moved below the tests
crates/e2e-tests/{Cargo.toml,tests/foreign_chain_tx_validation.rs}	Adds near-mpc-sdk dep and an SDK-built hash-bound Bitcoin case
docs/foreign-chain-transactions.md	Documents the field, the threat it mitigates, and the new node-side failure mode
Cargo.lock	near-mpc-sdk added to e2e-tests

The security property holds: the expectation is part of the borsh LookupMap key, so a responder cannot strip or alter expected_payload_hash to dodge the check — a mismatching key just misses the lookup and yields RequestNotFound. Wire compatibility is preserved for existing callers (skip_serializing_if + optional ABI property), and the RocksDB-persisted node request reads old rows back as None.

Findings

Non-blocking (nits, follow-ups, suggestions):

• crates/contract/src/v3_13_0_state.rs:166 — The migration abandons every in-flight verify_foreign_tx request (they time out) and permanently strands their V2 storage staking. The comment documents this well, but per CLAUDE.md a consequence this durable warrants a TODO(#NNNN) with an issue: the abandoned entries are addressable later (the keys are recoverable from indexer history under the _Deprecated…V2 prefix), so "never reclaimed" is really "not reclaimed by this version". Also worth stating in the PR description, since it is a deliberate on-chain behavior change at upgrade time. If the ForeignTx flow is already carrying real traffic, consider whether a transitional read of the V2 map (shadowing the old key type) is worth ~30 lines to avoid failing in-flight requests.
• crates/contract/src/v3_13_0_state.rs:96 — The shadow struct still types the old map's key as the new VerifyForeignTransactionRequest, which no longer describes the deployed layout that this module's doc comment (line 88) says to shadow. It is harmless today only because store::LookupMap's borsh payload is just the prefix; a future reader could reuse old.pending_verify_foreign_tx_requests believing the keys are compatible. Either shadow the old key type or note in the comment that the type parameter is irrelevant to deserialization.
• crates/contract/tests/sandbox/upgrade_to_current_contract.rs — The upgrade harness asserts pending signature requests survive a 3.13.0 upgrade; there is no analogous assertion for foreign-tx requests, so the new abandonment behavior is untested and would not fail if reverted (engineering-standards §Add tests). A negative case (pending foreign-tx request before upgrade → get_pending_verify_foreign_tx_request returns None / respond returns RequestNotFound afterwards) would lock in the intent.
• crates/node/src/providers/verify_foreign_tx/sign.rs:36 — The mismatch is detected only after the leader has consumed a presignature (take_owned() at line 71) and after the RPC fan-out, so a bogus expected_payload_hash burns one presignature plus RPC quota per request. Issue [Audit] respond_verify_foreign_tx doesn't check that payload_hash matches the response #3997 explicitly calls out "users can't drain needless resources from the network by submitting bogus payload hashes", so it deserves an explicit answer here. It is not a regression (a request for a non-existent tx already drains the same way), but moving take_owned() to after execute_foreign_chain_request + build_signature_request would remove the presignature waste for both cases.
• crates/near-mpc-sdk/src/foreign_chain.rs:86 — ForeignChainSignatureVerifier is borsh-persistable but does not record the payload version; expected_payload_hash() re-reads the crate-global DEFAULT_PAYLOAD_VERSION at verify time while build() pins payload_version into the on-chain request. Once a V2 exists and the default moves, a verifier persisted across an SDK upgrade will recompute a V2 hash for a V1 request and reject a valid response. The new ForeignTxSignPayload::new(version, …) already takes a version — storing it in the verifier at build time closes this cheaply.
• crates/e2e-tests/tests/foreign_chain_tx_validation.rs:338 — The comment claims the test "proves the SDK and the node agree on the payload encoding", but MOCK_BLOCK_HASH is 0xaa…aa, which is byte-order symmetric, so a hash byte-order disagreement between the SDK's expected value and the inspector's extracted value would pass unnoticed. Either soften the claim or use an asymmetric mock hash. Separately, _verifier is discarded at line 347 — running verifier.verify_signature(&response, &pk) would exercise the SDK's verification path end-to-end and match what the comment promises.
• crates/contract/src/lib.rs:3985 — The new sign_foreign_tx_payload helper duplicates the inline signing block in the adjacent respond_verify_foreign_tx__should_succeed_when_response_is_valid_and_request_exists test (lines 3849-3870); reusing it there removes the copy. It also panics on failure, so engineering-standards §must_ prefix would name it must_sign_foreign_tx_payload (the surrounding module predates that convention, so this is only a nit).
• Deployment ordering: an upgraded contract accepts expected_payload_hash immediately, but a not-yet-upgraded node drops the unknown field and responds with a key lacking it, so the response misses the lookup and the request times out (one failed respond tx per request, no retry loop). Worth calling out in the release notes so node rollout lands before callers start setting the field.

✅ Approved

[netrome]

crates/near-mpc-sdk/src/foreign_chain.rs:86 — ForeignChainSignatureVerifier is borsh-persistable but does not record the payload version; expected_payload_hash() re-reads the crate-global DEFAULT_PAYLOAD_VERSION at verify time while build() pins payload_version into the on-chain request. Once a V2 exists and the default moves, a verifier persisted across an SDK upgrade will recompute a V2 hash for a V1 request and reject a valid response. The new ForeignTxSignPayload::new(version, …) already takes a version — storing it in the verifier at build time closes this cheaply.

This sounds like a potential good point, but orthogonal to this PR and I don't have the capacity to distinguish this statement from slop/hallucinations atm so I will leave this for now.

[netrome]

crates/near-mpc-sdk/src/foreign_chain.rs:86 — ForeignChainSignatureVerifier is borsh-persistable but does not record the payload version; expected_payload_hash() re-reads the crate-global DEFAULT_PAYLOAD_VERSION at verify time while build() pins payload_version into the on-chain request. Once a V2 exists and the default moves, a verifier persisted across an SDK upgrade will recompute a V2 hash for a V1 request and reject a valid response. The new ForeignTxSignPayload::new(version, …) already takes a version — storing it in the verifier at build time closes this cheaply.

This sounds like a potential good point, but orthogonal to this PR and I don't have the capacity to distinguish this statement from slop/hallucinations atm so I will leave this for now.

Looked deeper into this and created a follow-up as it may be a good thing to look into #4079

[haiyuechen-nearone]

🚢

[anodar]

Thank you!