test(devnet): cross-RPC nonce-replay e2e test for nonceEnforcement (batch E)#899
test(devnet): cross-RPC nonce-replay e2e test for nonceEnforcement (batch E)#899tcsenpai wants to merge 1 commit into
Conversation
…atch E) Closes the e2e-test follow-up flagged in PR #886's description. What this verifies Builds + signs ONE transaction via the SDK against node-1, then submits the SAME signed tx to BOTH node-1 AND node-2 in parallel through `DemosTransactions.confirm()` + `DemosTransactions. broadcast()`. Asserts that after the chain advances: 1. Receiver balance increased by EXACTLY one transfer amount (not 2× — no double-spend). 2. Sender account nonce advanced by EXACTLY 1 (not 2 — both did not apply at consensus). Failure modes (any of these → FAIL with diagnostic dump): - `observed_receiver_delta > amount`: cross-RPC double-spend, both broadcasts applied (consensus-side `expectedPrior` reject in `GCRNonceRoutines` failed). - `observed_nonce_delta > 1`: same as above. - `observed_receiver_delta < amount`: neither broadcast landed (test couldn't verify the safety net engaged). Files - `testing/devnet/scripts/double_broadcast_replay.mjs` — the dual-broadcast bun script. Reads NODE1_URL / NODE2_URL / IDENTITY_PATH / RECEIVER_PUBKEY / AMOUNT_OS from env so the bash harness can wire it to devnet without source edits. - `testing/devnet/scripts/test-double-broadcast-e2e.sh` — bash harness. Mirrors `test-transfer-e2e.sh` shape: boots devnet fixture stack, waits for both RPCs, runs the dual broadcast, asserts via the bun script's exit code. How to run bash testing/devnet/scripts/test-double-broadcast-e2e.sh bash testing/devnet/scripts/test-double-broadcast-e2e.sh --keep --no-build Exercises - PR #886's consensus-side `expectedPrior` apply-time reject in `GCRNonceRoutines`. - PR #887's same-node TOCTOU advisory lock in `Mempool. addTransaction` (the second broadcast hitting node-1 may be caught at the mempool boundary before reaching consensus, depending on race ordering). - PR #888's vote-race fix in `broadcastBlockHash` (both txs going through consensus correctly tally and the second one fails BFT once `expectedPrior` rejects the second nonce edit). Out of scope - Performance / load test variants (1000 concurrent replays): tracked separately as the "mempool stress test" item. - Cross-RPC vote-relay scenarios (TODO at end of `broadcastBlockHash.ts`): separate consensus design follow-up.
|
Warning Review limit reached
More reviews will be available in 56 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptile review |
Review Summary by QodoAdd cross-RPC nonce-replay e2e test for nonceEnforcement
WalkthroughsDescription• Add end-to-end test for cross-RPC nonce-replay protection • Verifies nonceEnforcement fork prevents double-spend attacks • Broadcasts same signed transaction through two nodes simultaneously • Asserts receiver balance increases by exactly one transfer amount • Validates sender nonce advances by exactly one (not two) Diagramflowchart LR
A["Devnet Setup<br/>with nonceEnforcement"] --> B["Build + Sign<br/>Single TX"]
B --> C["Submit to<br/>Node-1 & Node-2<br/>in Parallel"]
C --> D["Broadcast<br/>ValidityData<br/>from Both"]
D --> E["Poll Chain State<br/>for 60s"]
E --> F["Assert:<br/>Receiver Delta = 1x<br/>Sender Nonce Delta = 1"]
F --> G{Test Result}
G -->|Pass| H["Exit 0: OK"]
G -->|Fail| I["Exit 1: FAIL<br/>with Diagnostics"]
File Changes1. testing/devnet/scripts/test-double-broadcast-e2e.sh
|
Code Review by Qodo
1. Missing await on getAddress
|
Greptile SummaryThis PR adds an end-to-end test for the
Confidence Score: 1/5The test harness cannot run in its current form; the bash script will error out on the very first docker compose call. Every
Important Files Changed
Sequence DiagramsequenceDiagram
participant SH as test-double-broadcast-e2e.sh
participant N1 as node-1 RPC
participant N2 as node-2 RPC
participant BUN as double_broadcast_replay.mjs
participant C as Consensus
SH->>N1: docker compose up (fixture)
SH->>N2: docker compose up (fixture)
SH->>N1: wait_for_rpc (health check)
SH->>N2: wait_for_rpc (health check)
SH->>BUN: bun double_broadcast_replay.mjs
BUN->>N1: demos1.connect + connectWallet
BUN->>N2: demos2.connect + connectWallet
BUN->>N1: getAddressInfo (pre-state snapshot)
BUN->>N1: demos1.pay() — build + sign ONE tx
par Parallel confirm
BUN->>N1: DemosTransactions.confirm(signedTx, demos1)
BUN->>N2: DemosTransactions.confirm(signedTx, demos2)
end
par Parallel broadcast
BUN->>N1: DemosTransactions.broadcast(confirmed1, demos1)
BUN->>N2: DemosTransactions.broadcast(confirmed2, demos2)
end
N1-->>C: tx proposal (nonce N+1)
N2-->>C: tx proposal (nonce N+1) replay
C-->>C: nonceEnforcement: only first tx applied
loop Poll every 2s up to 60s
BUN->>N1: getAddressInfo(receiver)
BUN->>N1: getAddressInfo(sender)
end
alt "delta == amount AND nonce_delta == 1"
BUN-->>SH: exit 0 OK
else "delta > amount OR nonce_delta > 1"
BUN-->>SH: exit 1 DOUBLE-SPEND
else "delta < amount after timeout"
BUN-->>SH: exit 1 neither tx landed
end
|
Greptile SummaryThis PR adds an end-to-end devnet test for the
Confidence Score: 2/5The test cannot run as written — the devnet fixture compose file it depends on is not present in the repository. The bash harness fails at docker compose up because docker-compose.fixture.yml is absent from the repository, so every invocation exits before any node starts and the test provides zero coverage. The poll loop in the bun script only queries node-1 despite the comment claiming both RPCs are cross-checked, risking silent false negatives. An unguarded property access on the pay() return value would surface as an unhandled TypeError rather than a clean diagnostic exit. Both files need attention: test-double-broadcast-e2e.sh needs the missing fixture compose file resolved, and double_broadcast_replay.mjs needs the poll-loop RPC cross-check and the null guard corrected. Important Files Changed
Sequence DiagramsequenceDiagram
participant SH as test-double-broadcast-e2e.sh
participant DC as Docker Compose
participant BUN as double_broadcast_replay.mjs
participant N1 as node-1 RPC
participant N2 as node-2 RPC
participant Chain as Consensus / Chain
SH->>DC: docker compose up node-1 node-2 postgres tlsnotary
SH->>N1: poll GET / until Hello World
SH->>N2: poll GET / until Hello World
SH->>N1: getAddressInfo(sender)
SH->>BUN: bun double_broadcast_replay.mjs
BUN->>N1: connect + connectWallet(mnemonic)
BUN->>N2: connect + connectWallet(mnemonic)
BUN->>N1: getAddressInfo snapshot pre-state
BUN->>N1: pay() build and sign ONE tx
par Parallel confirm
BUN->>N1: DemosTransactions.confirm(signedTx)
and
BUN->>N2: DemosTransactions.confirm(signedTx)
end
par Parallel broadcast
BUN->>N1: DemosTransactions.broadcast(validityData1)
and
BUN->>N2: DemosTransactions.broadcast(validityData2)
end
Chain->>Chain: GCRNonceRoutines expectedPrior check rejects duplicate
loop Poll up to 60s node-1 only
BUN->>N1: getAddressInfo(receiver) and getAddressInfo(sender)
BUN->>BUN: "check delta == expectedAmount and nonceDelta == 1"
end
BUN-->>SH: exit 0 OK or exit 1 FAIL
Reviews (1): Last reviewed commit: "test(devnet): cross-RPC nonce-replay e2e..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Flags: | ||
| # --keep Leave the devnet running on exit (for manual poking) | ||
| # --no-build Skip --build on `docker compose up` (faster reruns) | ||
|
|
There was a problem hiding this comment.
docker-compose.fixture.yml will immediately break the test run
COMPOSE_FILES references docker-compose.fixture.yml, but only docker-compose.yml exists in testing/devnet/. The first down call is silently masked by || true, so this failure goes unnoticed — but the up call on line 80 is not masked and runs under set -euo pipefail, so Docker Compose will exit non-zero ("no such file or directory") and abort the script before any node starts. The test will never execute.
| // stale node-1 state. | ||
| // ----------------------------------------------------------------------------- | ||
| console.log("[double-broadcast] [4/4] polling post-state for up to 60s") | ||
|
|
||
| let observedDelta = 0n | ||
| let observedNonceDelta = 0 | ||
| let polls = 0 | ||
| const maxPolls = 30 | ||
| const expectedDelta = amountOs | ||
|
|
||
| for (let i = 0; i < maxPolls; i++) { | ||
| polls = i + 1 | ||
| await new Promise(r => setTimeout(r, 2000)) | ||
| const recInfo = await demos1.getAddressInfo(RECEIVER_PUBKEY) |
There was a problem hiding this comment.
The comment directly above the poll loop states: "Re-check via BOTH RPCs so we don't fool ourselves on stale node-1 state." However, both getAddressInfo calls (recInfo and sendInfo) target only demos1 (node-1). If the transaction committed on node-2 first and node-1 hasn't replicated it yet, all 30 polls will see stale state and the test exits with "neither tx landed" — a false negative that doesn't distinguish replay-protection failure from network lag.
| const signedTx = await demos1.pay(RECEIVER_PUBKEY, amountOs) | ||
| const localHash = signedTx?.hash | ||
| console.log(`[double-broadcast] local tx hash: ${localHash}`) | ||
| console.log(`[double-broadcast] tx.content.nonce: ${signedTx.content.nonce}`) |
There was a problem hiding this comment.
pay() returns, signedTx?.hash safely handles a falsy return value, but the very next line signedTx.content.nonce accesses content without any guard. If pay() returns null or undefined, this throws an unhandled TypeError instead of the clean process.exit(1) the rest of the script uses.
| const signedTx = await demos1.pay(RECEIVER_PUBKEY, amountOs) | |
| const localHash = signedTx?.hash | |
| console.log(`[double-broadcast] local tx hash: ${localHash}`) | |
| console.log(`[double-broadcast] tx.content.nonce: ${signedTx.content.nonce}`) | |
| const signedTx = await demos1.pay(RECEIVER_PUBKEY, amountOs) | |
| if (!signedTx) { | |
| console.error("[double-broadcast] FAIL: pay() returned null/undefined; cannot build signed tx") | |
| process.exit(1) | |
| } | |
| const localHash = signedTx?.hash | |
| console.log(`[double-broadcast] local tx hash: ${localHash}`) | |
| console.log(`[double-broadcast] tx.content.nonce: ${signedTx.content.nonce}`) |
| AMOUNT_OS="$((SENDER_BAL_OS / 10))" | ||
| echo "[double-broadcast-e2e] transfer amount: ${AMOUNT_OS} OS (10% of sender balance)" |
There was a problem hiding this comment.
SENDER_BAL_OS is between 1 and 9 inclusive, integer division $((SENDER_BAL_OS / 10)) produces 0. The script passes AMOUNT_OS=0 to the bun script, which then checks senderBalBefore < amountOs → N < 0n → false, so the test proceeds with a zero-value transfer. This doesn't exercise the double-spend path meaningfully.
| AMOUNT_OS="$((SENDER_BAL_OS / 10))" | |
| echo "[double-broadcast-e2e] transfer amount: ${AMOUNT_OS} OS (10% of sender balance)" | |
| AMOUNT_OS="$((SENDER_BAL_OS / 10))" | |
| if [[ "${AMOUNT_OS}" -eq 0 ]]; then | |
| echo "[double-broadcast-e2e] ERROR: computed AMOUNT_OS=0 (sender balance ${SENDER_BAL_OS} OS is too small for 10% slice)." | |
| exit 1 | |
| fi | |
| echo "[double-broadcast-e2e] transfer amount: ${AMOUNT_OS} OS (10% of sender balance)" |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const sender = demos1.getAddress() | ||
| console.log(`[double-broadcast] SENDER=${sender}`) | ||
|
|
||
| // ----------------------------------------------------------------------------- | ||
| // 2. Snapshot pre-state | ||
| // ----------------------------------------------------------------------------- | ||
| const senderInfoBefore = await demos1.getAddressInfo(sender) | ||
| const receiverInfoBefore = await demos1.getAddressInfo(RECEIVER_PUBKEY) |
There was a problem hiding this comment.
1. Missing await on getaddress 🐞 Bug ≡ Correctness
double_broadcast_replay.mjs assigns sender = demos1.getAddress() without awaiting, but getAddress() is used elsewhere in the repo as an async API. This makes sender a Promise and breaks subsequent getAddressInfo(sender) calls, causing the e2e test to fail before it can validate nonce-replay protection.
Agent Prompt
### Issue description
`sender` is assigned from `demos1.getAddress()` without `await`, so it is likely a Promise rather than an address string. The script then passes `sender` into `getAddressInfo()`, which will fail or behave incorrectly.
### Issue Context
Another repo script uses `await demos.getAddress()` when retrieving the wallet address, indicating `getAddress()` is async.
### Fix Focus Areas
- testing/devnet/scripts/double_broadcast_replay.mjs[61-80]
### Suggested fix
- Change to `const sender = await demos1.getAddress()`.
- (Optional hardening) Validate `sender` matches expected hex address format before calling `getAddressInfo(sender)` and exit with a clear error if not.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Plain-English summary
What your code does
The `nonceEnforcement` fork landed across PRs #884-#887 to close cross-RPC replay attacks. The protection is supposed to work like this:
But until this PR, there was no automated test that exercised that scenario end-to-end. PRs #886 + #887's plain-English summaries described the protection, manual traces argued it works, but nothing actually ran the attack against running nodes.
What this PR adds
A devnet e2e test that simulates the attack and asserts the protection works:
Why this matters
Three different layers of protection landed across four PRs (RPC validation, mempool advisory lock, consensus `expectedPrior` reject). Without an integration test exercising all three in concert against real running nodes, a future refactor could silently regress any one of them and the protection would degrade quietly — no unit test would catch it.
This test is the canary. Run it after any change to `assignNonce`, `Mempool.addTransaction`, `GCRNonceRoutines`, `endpointValidation` hash-strip, or `broadcastBlockHash` vote tally.
Technical detail
How to run
```bash
bash testing/devnet/scripts/test-double-broadcast-e2e.sh
or with --keep / --no-build flags (same as test-transfer-e2e.sh)
```
What it exercises
Failure modes the test catches
Out of scope
Test plan