feat(consensus): nonceEnforcement consensus rule + caller wire-up (batch C PR 3) [DRAFT - review needed]#886
Merged
tcsenpai merged 3 commits intoMay 31, 2026
Conversation
…tch C PR 3 / Path A)
Final PR of the 3-PR batch C sequence specified in
`docs/specs/audit-sweep-batch-c-nonce.md`. Closes the cross-RPC
replay window end-to-end and activates the `nonceEnforcement` fork
on devnet (already active at block 0 since PR 1's genesis fixture).
Path A — SDK ships type-only, node populates `expectedPrior` at
apply time. Chosen over Path B (SDK-side emit) and Path C
(SDK fork introspection) for zero breaking-change risk: old SDK
clients continue working post-fork without re-publishing. See spec
doc §"Emission-site rationale" for the trade-off matrix.
Changes:
src/libs/blockchain/gcr/gcr_routines/GCRNonceRoutines.ts
- When `expectedPrior` is set on a non-rollback nonce edit,
reject the apply iff `accountGCR.nonce !== expectedPrior`.
- Rollback path skips the check (unwinding, not validating
forward progress).
- Cross-RPC double-spend safety net: two competing same-nonce
txs bundled into the same block from different RPCs both pass
per-node validation but only one survives consensus apply.
src/libs/blockchain/gcr/handleGCR.ts
- In `applyTransaction`'s per-edit loop, populate `expectedPrior`
on every `nonce` edit when `nonceEnforcement` is active at the
apply block. Read live from `entities.accounts` so a
hypothetical second nonce edit in the same tx sees the
post-first-apply value.
- Block height: `tx.blockNumber ?? lastBlockNumber ?? 0`.
- Skip on rollback. Skip if `expectedPrior` is already set (defence
against future SDK-side population).
src/libs/network/endpointValidation.ts
- Symmetric strip of `expectedPrior` on both
`gcrEdits` (regen) and `txEditsBlanked` (SDK-shipped) before
hash compare. Mirrors the existing `txhash` blanking pattern.
- SDK never writes the field today; the strip is defence in depth
against future SDK-side emission and any non-conformant clients.
src/libs/blockchain/routines/validateTransaction.ts
- Uncomment `assignNonce` caller (lines 80-89). Pre-fork it
short-circuits to true (PR 1). Post-fork enforces strict
sequential semantics with mempool lookahead (PR 2) + the
matching consensus-side rejection above.
package.json
- Bump `@kynesyslabs/demosdk` from 4.0.3 to 4.0.5 — the
published SDK that adds the `expectedPrior?: number` field to
`GCREditNonce` (type-only, runtime unchanged).
docs/specs/audit-sweep-batch-c-nonce.md
- Update PRs list (#885 merged, #884 merged, #TBD = this PR).
- Add `sdk_publish: 4.0.5` to frontmatter.
- Rewrite §Fork activation Post-fork bullet to describe the
Path A flow (node populates, not SDK emits).
- Add §Emission-site rationale documenting why Path A was
chosen over B / C.
- Update PR 3 row in the breakdown to list actual files +
summarise the SDK pin bump.
Verification:
- `tsc --noEmit` clean on all six changed files.
- SDK 4.0.5 verified locally: `expectedPrior?: number` present in
`node_modules/@kynesyslabs/demosdk/build/types/blockchain/GCREdit.d.ts`.
- No new e2e test in this commit; will follow up in a separate
PR with the devnet two-RPC double-broadcast scenario once the
base wires are merged.
Cross-RPC double-spend scenario (post-fork):
1. Client signs tx with `content.nonce = N+1`.
2. Broadcasts to RPC-A and RPC-B in parallel.
3. Both RPCs pass per-node validation (mempool lookahead is
single-node).
4. Block-formation gathers both → applies in deterministic order.
5. First apply: account.nonce goes N → N+1. `expectedPrior` matched
(N).
6. Second apply: `expectedPrior = N` (populated at apply time from
pre-first-apply snapshot), but `accountGCR.nonce = N+1`. Mismatch
→ `GCRNonceRoutines` rejects → whole tx rolls back at consensus.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR implements sequential nonce enforcement through an optional ChangesSequential nonce enforcement via expectedPrior
Sequence DiagramsequenceDiagram
participant Client
participant confirmTransaction
participant assignNonce
participant txContentGCREdits
participant GCRNonceRoutines
participant endpointValidation
Client->>confirmTransaction: submit tx
confirmTransaction->>assignNonce: assignNonce(tx)
assignNonce->>assignNonce: compute expectedPrior = account.nonce + pendingCount
assignNonce->>txContentGCREdits: populate expectedPrior on nonce edits
assignNonce-->>confirmTransaction: return success or nonce error
confirmTransaction->>GCRNonceRoutines: apply GCR edits
GCRNonceRoutines->>GCRNonceRoutines: if expectedPrior present && not rollback && fork active -> compare to accountGCR.nonce
GCRNonceRoutines-->>confirmTransaction: apply success or mismatch error
confirmTransaction->>endpointValidation: handleValidateTransaction (hash check)
endpointValidation->>endpointValidation: strip expectedPrior from nonce edits before hashing
endpointValidation-->>confirmTransaction: hash comparison result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
#886 self-review) Caught during draft-PR review: the apply-time populate site I shipped in the initial commit was incorrect. Bug: Populating `expectedPrior` inside `HandleGCR.applyTransaction`'s per-edit loop reads `entities.accounts` which already reflects prior in-block applies. For two competing same-nonce txs in the same block: 1. Tx A applies. Edit populates `expectedPrior = N` from live state. Apply check passes (N === N). Account → N+1. 2. Tx B (replay) applies next. Edit populates `expectedPrior = N+1` (live state already advanced). Apply check passes (N+1 === N+1). Account → N+2. Both succeed. This defeats the entire cross-RPC safety net. Fix: Move populate into `assignNonce` itself (validation time), using the already-loaded account row + pending-mempool-count: `expectedPrior = account.nonce + pendingCount` This locks the value to the pre-block snapshot the sender's nonce was supposed to be at, NOT what it becomes at apply time. For the same scenario: 1. Tx A's edit carries `expectedPrior = 0`. Apply: account=0, expectedPrior=0 → match. Account → 1. 2. Tx B's edit also carries `expectedPrior = 0` (validated on a node that didn't yet see Tx A). Apply: account=1, expectedPrior=0 → MISMATCH → reject. Whole tx rolls back. For back-to-back same-sender submissions (k-th tx): `expectedPrior = account.nonce + (k-1)` matching the apply order because PR 2's mempool lookahead ensures each pending tx applies before this one. Hash compare safety: `endpointValidation` snapshots `tx.content.gcr_edits` at line 48 BEFORE `confirmTransaction` runs (which invokes `assignNonce`). My mutation hits the real array but the snapshot is the pre-mutation shape. Both sides of the compare are still stripped of `expectedPrior`, so signature integrity is preserved. The mutated array flows downstream into the apply pipeline carrying the validation-time-locked `expectedPrior`. Files: - src/libs/blockchain/routines/validateTransaction.ts: walk `tx.content.gcr_edits`, set `expectedPrior` on the matching `nonce` edit (sender address compared case-insensitive). - src/libs/blockchain/gcr/handleGCR.ts: revert the apply-time populate code added in the previous commit. Also drop the now- unused `isForkActive` and `getSharedState` imports. - docs/specs/audit-sweep-batch-c-nonce.md: rewrite the fork activation Post-fork bullet to describe the validation-time populate, plus explicit rationale for why apply-time is wrong. Update PR 3 file list to reflect the moved populate site. Verification: - tsc --noEmit clean. - Manual trace of the cross-RPC scenario now correctly rejects the second replay.
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/audit-sweep-batch-c-nonce.md`:
- Around line 150-167: The emission-site rationale text incorrectly says “node
populates at apply time”; update this section to state that the node populates
expectedPrior at validation time (not apply time) to match the implementation in
validateTransaction.ts and the endpointValidation flow; adjust the Path A
description and any bullet points referencing apply-time population so they
instead describe validation-time population (keeping notes about signature
safety, zero breaking change, and SDK type-only shipping intact) and ensure the
symbol expectedPrior is referenced as being stripped before signed-hash checks
in endpointValidation/validateTransaction.
In `@src/libs/blockchain/gcr/gcr_routines/GCRNonceRoutines.ts`:
- Around line 60-77: The current expectedPrior enforcement in GCRNonceRoutines
unconditionally checks editOperation.expectedPrior (inside the block using
editOperation.isRollback and actualNonce) which causes a pre-fork consensus
change; modify the conditional to also require the runtime/apply-time
nonceEnforcement flag (e.g., check nonceEnforcement === true) before performing
the expectedPrior equality check so that nodes without nonceEnforcement set will
ignore expectedPrior; update the branch that returns the mismatch error to only
run when nonceEnforcement is enabled, leaving behavior unchanged for isRollback
and absent expectedPrior cases.
In `@src/libs/blockchain/routines/validateTransaction.ts`:
- Around line 556-565: The nonce-edit branch in validateTransaction.ts fails to
normalize non-string edit.account so binary account values never match
senderAddress; update the loop that handles tx.content.gcr_edits (the block
checking edit.type === "nonce") to normalize edit.account the same way as
tx.content.from is normalized (convert string accounts to lowercase, otherwise
keep as-is) before comparing to senderAddress, and then set edit.expectedPrior =
expectedPrior when they match (using account.nonce and pendingCount as already
computed); mirror the normalization logic used in GCRNonceRoutines.ts to ensure
binary-form accounts are handled correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbae1b88-b82f-4adf-9497-895eec4844ac
📒 Files selected for processing (5)
docs/specs/audit-sweep-batch-c-nonce.mdpackage.jsonsrc/libs/blockchain/gcr/gcr_routines/GCRNonceRoutines.tssrc/libs/blockchain/routines/validateTransaction.tssrc/libs/network/endpointValidation.ts
Greptile SummaryThis PR wires the
Confidence Score: 4/5The core populate-and-reject pipeline is logically sound on the diff as written, but the PR is correctly held as draft: the cross-RPC double-spend e2e test is absent and mempool serialisation preservation of expectedPrior is unverified. The validation-time populate in assignNonce, the symmetric strip in endpointValidation, and the fork-gated expectedPrior check in GCRNonceRoutines are individually correct. The remaining open question — whether expectedPrior survives mempool storage/retrieval to reach GCRNonceRoutines.apply — is the key gap the draft status acknowledges. The interaction between validateTransaction.ts (populate site) and the mempool storage path deserves a second look: if the mempool serialises the tx object before persisting and deserialises it for block formation, expectedPrior must survive that round-trip or the reject gate in GCRNonceRoutines.ts will silently not engage. Important Files Changed
Sequence DiagramsequenceDiagram
participant SDK as SDK Client
participant EP as endpointValidation
participant CT as confirmTransaction / assignNonce
participant MP as Mempool
participant BF as Block Formation
participant GCR as GCRNonceRoutines.apply
SDK->>EP: POST /tx (gcr_edits without expectedPrior)
EP->>EP: snapshot txShippedGcrEdits (pre-mutation)
EP->>CT: confirmTransaction(tx)
CT->>CT: assignNonce - read account.nonce + pendingCount
CT->>CT: mutate tx.content.gcr_edits in-place
CT-->>EP: "hasNonce = true"
EP->>EP: GCRGeneration.generate(tx) regen edits
EP->>EP: strip expectedPrior from both sides
EP->>EP: "hash(regen) == hash(snapshot)"
EP->>MP: accept tx (expectedPrior on in-memory gcr_edits)
MP->>BF: provide tx for block
BF->>GCR: applyTransaction (nonce edit with expectedPrior)
GCR->>GCR: isForkActive nonceEnforcement
alt expectedPrior matches accountGCR.nonce
GCR-->>BF: success - nonce incremented
else mismatch cross-RPC replay
GCR-->>BF: failure - tx rejected
end
Reviews (3): Last reviewed commit: "fix(audit-sweep): address Greptile + Cod..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
@greptile review |
…eploop iter 1)
Three real findings + three doc/comment fixes.
Critical (CodeRabbit) — fork-gate the consensus rule
GCRNonceRoutines.apply enforced the `expectedPrior` mismatch check
unconditionally whenever the field was present. The spec says
pre-fork nodes must ignore it, and the field is stripped from the
hash compare in endpointValidation — so a client could attach
`expectedPrior` on a pre-fork chain without breaking signature
validation, and upgraded nodes would start rejecting edits that
older nodes still accepted. Validator-split risk before the fork
activates.
Fix: wrap the mismatch check in `isForkActive(nonceEnforcement,
blockHeight)` using `getSharedState.lastBlockNumber` (same gate
source `assignNonce` uses). Pre-fork ignores the field entirely;
post-fork enforces. Imports added: `isForkActive`,
`getSharedState`.
Major (CodeRabbit) / P1 (Greptile) — forge-account comparison bug
In `assignNonce`'s populate loop, `edit.account` can be a
forge-key object (non-string), but the comparison was
`editAccount === senderAddress` where `senderAddress` is a
lowercase hex string. Strict equality between an object and a
string is always false, so the populate would silently skip for
forge-format accounts and the cross-RPC safety net would never
engage for those transactions.
Fix: mirror the normalization already used in `GCRNonceRoutines`:
`typeof edit.account === "string" ? edit.account :
forgeToHex(edit.account)`, then `.toLowerCase()`. Now matches the
sender regardless of how the SDK ships the account field.
Doc / comment cleanup
Three locations still carried "node fills it in at apply time"
wording, contradicting the validation-time populate site. Updated
in:
- `GCRNonceRoutines.ts`: comment now describes the fork gate
+ validation-time source.
- `endpointValidation.ts`: comment now points at `assignNonce`
as the populate site.
- `docs/specs/audit-sweep-batch-c-nonce.md`: Path A header
rewritten as "node populates at validation time"; rationale
paragraph now explains the apply-time draft was rejected,
with the cross-RPC reasoning inline.
Doc — PR 3 row scope
The scope-table row for PR 3 listed "e2e test" as a deliverable,
but the diff ships no test. Removed the "e2e test" item and
added an explicit note that cross-RPC e2e is tracked as a
follow-up PR. Also reworded "apply-time rejection" to "consensus-
side rejection" to match what `GCRNonceRoutines` actually does
(rejection at apply, populate at validation).
Out of scope for this iter
TOCTOU advisory-lock wrapper documented in `assignNonce`'s
PR 2 docstring is still not landed; flagged in this review round
but deferred to a follow-up PR since wrapping the validate +
`Mempool.addTransaction` sequence in `pg_advisory_xact_lock` is
its own structural change that affects every native-tx ingress
path. Filed as PR 4 in the follow-up tracker.
Verification:
- `tsc --noEmit` clean.
- Pre-fork trace: `expectedPrior` present on edit, fork inactive,
`isForkActive` returns false → mismatch check skipped → legacy
apply behaviour preserved bit-identically for re-sync.
- Post-fork trace: forge-format `edit.account` now hex-coerces
and matches the lowercased sender → `expectedPrior` populates
correctly → `GCRNonceRoutines` enforces the mismatch reject.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DRAFT — design review needed before merge.
PR 3 of the 3-PR batch C nonce-replay-protection sequence specified in
docs/specs/audit-sweep-batch-c-nonce.md.Concern flagged during review
The current implementation populates
expectedPriorinhandleGCR.applyTransactionat apply time, reading the live account nonce fromentities.accounts. This is not correct for the cross-RPC double-spend scenario:expectedPrior = N. Applies → N+1. ✓entities.accounts. Edit populatesexpectedPrior = N+1. Reject check:accountGCR.nonce(N+1) === expectedPrior(N+1)→ passes (BUG).The populate must happen at validation time (during
confirmTransaction), capturing the pre-block account state, so each tx'sexpectedPriorreflects what the sender's nonce should be. Re-reading at apply time defeats the safety net.Filed as draft so we don't merge the broken populate logic. Need to:
handleGCR.applyTransactionto a per-tx pass during validation OR at block-formation time beforeapplyTransactionruns.endpointValidationis still correct given the new populate site.The other changes are sound:
GCRNonceRoutinesrejection logic, validateTransaction caller uncomment, SDK pin bump, doc updates.Files
src/libs/blockchain/gcr/gcr_routines/GCRNonceRoutines.ts— apply-time rejection onexpectedPriormismatch (✓ correct)src/libs/blockchain/gcr/handleGCR.ts— apply-timeexpectedPriorpopulate (⚠ needs to move)src/libs/network/endpointValidation.ts— symmetric strip both sides (✓ correct under either populate site)src/libs/blockchain/routines/validateTransaction.ts—assignNoncecaller uncommented (✓ correct)package.json— demosdk 4.0.3 → 4.0.5 (✓)docs/specs/audit-sweep-batch-c-nonce.md— Path A rationale + PR list updates (✓)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores