Skip to content

fix: serialize Goat transactions per signer and stop client panics and silent failures - #540

Open
KSlashh wants to merge 1 commit into
devfrom
audit/52-client-hardening
Open

KSlashh wants to merge 1 commit into
devfrom
audit/52-client-hardening

Conversation

@KSlashh

@KSlashh KSlashh commented Nov 12, 2026

Copy link
Copy Markdown
Collaborator

Audit items L52, L20, L55, L53, L54, L49, L30, L45, L48. Open against dev.

Goat nonces and receipts (L52, L20). A transaction took its nonce from the latest block, and the node runs several GOATClients with the same key, so concurrent sends could sign with the same nonce and replace each other. A send waited 10 seconds for its receipt and then reported failure while the transaction could still be pending. Now:

  • One lock per sender address, shared by every adaptor in the process, is held from reading the nonce until the receipt arrives or the wait ends. Sends from one key are serialized.

  • The nonce still comes from the latest block, on purpose. With sends serialized it equals the pending nonce, except after a timeout. Then the next send reuses the nonce and replaces the stuck transaction instead of queueing a duplicate behind it. Its caller was already told the send failed.

  • The receipt wait is 30 seconds, and the timeout error says the transaction may still be pending and may be replaced.

SPV overwrite (L55). With ENABLE_UPDATE_SPV_CONTRACT, a block-hash mismatch while checking a peg-in or a graph posted the local esplora's hash to the SPV contract at that height, with no depth or continuity check. Both branches are removed and a mismatch is an error. The relayer task, which posts confirmed blocks only, is the one writer. The variable still enables that task.

Esplora retries (L53). BTC_REQUEST_TIMEOUT_SECS bounds a whole esplora call, retries included. esplora-client retries 429 and 5xx six times with doubling backoff from 256 ms, about 16 seconds of sleep, longer than the default 15-second timeout. The client now retries three times (about 1.8 seconds) and each HTTP request has the same timeout.

Default explorers (L54). Every network other than mainnet defaulted to the testnet3 explorer, while the default network is testnet4. Testnet4 and signet now default to their own mempool.space APIs; testnet3 is unchanged and regtest needs an explicit URL as before. The unused mempool_v1_type module, which built its URLs from the default explorer regardless of the configured one, is deleted.

Subgraph numbers (L49). A non-numeric blockNumber or rewardAmountSats from the subgraph panicked the event watcher on every restart. These are now errors: the watch round fails and its cursor does not advance.

Watch contract writes (L30). upsert_watch_contract and update_watch_contract_status ignored database errors and returned Ok, inside the watcher's transaction. They now return the error.

Swap confirmations (L45). The claim confirmation count used unchecked u32 subtraction and wrapped when the node's height was behind the explorer's. It now saturates at 0.

Swap claim trace (L48). The claim lookup walked every call frame, including reverted ones, so a reverted inner claim could supply the recorded claim data. A reverted frame and everything under it are now skipped.

Tests: two adaptors with the same key share one send lock and another key does not; each network's default explorer; a malformed block number is an error; watch contract writes report a database error; confirmations do not wrap; a claim in a reverted frame is ignored and the same claim in a successful frame is found. Node, client and store tests, clippy with -D warnings and fmt pass.

The test coverage and validation notes above are from the zkm PR notes. Branch scope and diff formatting were checked locally; builds and tests were not rerun during publication.

This branch has not been deployed

No deployments
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.

1 participant