fix(validation): snapshot tx.content.gcr_edits BEFORE confirmTransaction so applyGasFeeSeparation prepends don't break hash compare#871
Conversation
…ion so applyGasFeeSeparation prepends don't break the hash compare Root cause of the dev.node2 GCREdit mismatch that survived PRs #861/#867/#869: confirmTransaction runs `applyGasFeeSeparation` when the gasFeeSeparation fork is active, which PREPENDS node-computed fee edits onto `tx.content.gcr_edits` in place. The hash comparison ran AFTER that mutation, so: tx.content.gcr_edits (mutated): [fee_a, fee_b, ..., subtract, add, gas, nonce] GCRGeneration.generate(tx): [subtract, add, gas, nonce] The compare was diverging by exactly the prepended fee edits — a structural mismatch the SDK has no way to predict because the fee distribution is the validator's computation, not the signer's. dev.node2 reproduced and the local devnet did NOT because the local devnet boots with `gasFeeSeparation.activationHeight: null` → `applyGasFeeSeparation` is gated off → no prepend → the (already shipped) #861/#867 normalisation was sufficient. Fix: snapshot `tx.content.gcr_edits` via structured clone BEFORE calling `confirmTransaction`, then run the hash compare against the snapshot instead of the mutated array. The invariant we want to verify is "did the SDK ship the same edits GCRGeneration would regenerate?" — that's what the snapshot captures. Fee edits live downstream of this check and become part of the signed validity data via the existing `signValidityData` flow. Verified by tracing call sites: - applyGasFeeSeparation.ts:187 mutates tx.content.gcr_edits in place - validateTransaction.ts:133 calls it inside confirmTransaction - endpointValidation.ts:35 calls confirmTransaction, line ~42 was then reading the already-mutated tx.content.gcr_edits Manual repro from earlier in the session: BROADCAST=1 bun transfer_10pct.mjs against dev.node2 returned `valid: false` / "GCREdit mismatch" on every attempt despite the source on disk being correct (post-#867). Tracing showed regen produced a constant `16f9495100b98…` hash (the SDK-shipped 4-edit set) while tx-side hash varied per run — and the varying part was the SHA-256 of the prepended fee edits, whose contents depend on per-block fee state.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughThe PR modifies transaction validation in ChangesTransaction Validation Edit Snapshot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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 |
Greptile SummaryThis PR fixes a hash comparison false-negative in
Confidence Score: 4/5The core fix is correct and well-reasoned — snapshotting before the mutation point directly eliminates the divergence. The two findings are non-blocking quality items. The snapshot placement is correct (before The single changed file Important Files Changed
Sequence DiagramsequenceDiagram
participant SDK as SDK (client)
participant EV as endpointValidation
participant CT as confirmTransaction
participant AGFS as applyGasFeeSeparation
participant GCRG as GCRGeneration
SDK->>EV: tx (gcr_edits: [sub, add, gas, nonce])
Note over EV: SNAPSHOT txShippedGcrEdits = clone(tx.content.gcr_edits)<br/>[sub, add, gas, nonce]
EV->>CT: confirmTransaction(tx, sender)
CT->>AGFS: applyGasFeeSeparation(tx)
Note over AGFS: tx.content.gcr_edits = [fee_a, fee_b, ..., sub, add, gas, nonce]
AGFS-->>CT: ok
CT-->>EV: validationData
EV->>GCRG: GCRGeneration.generate(tx)
GCRG-->>EV: regenEdits [sub, add, gas, nonce]
Note over EV: Hash(normalise(txShippedGcrEdits)) == Hash(normalise(regenEdits)) ✓
EV-->>SDK: valid: true
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const txShippedGcrEdits: GCREdit[] = JSON.parse( | ||
| JSON.stringify(tx.content.gcr_edits ?? []), | ||
| ) |
There was a problem hiding this comment.
JSON.parse(JSON.stringify(...)) for the deep clone works for pure-JSON payloads, but structuredClone (which the PR title explicitly names) is the semantically correct primitive here — it handles the full JS value space without the implicit strip-undefined / throw-on-BigInt behavior of a JSON round-trip. Given GCREdit.amount is sometimes cast to BigInt downstream (line 205), a future schema change storing it as BigInt directly on the edit would silently throw here.
| const txShippedGcrEdits: GCREdit[] = JSON.parse( | |
| JSON.stringify(tx.content.gcr_edits ?? []), | |
| ) | |
| const txShippedGcrEdits: GCREdit[] = structuredClone( | |
| tx.content.gcr_edits ?? [], | |
| ) |
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? |
1 participant
Root cause
confirmTransactionrunsapplyGasFeeSeparationwhen thegasFeeSeparationfork is active, which PREPENDS node-computed fee edits ontotx.content.gcr_editsin place. The hash comparison ran AFTER that mutation:Divergence by EXACTLY the prepended fee edits — a structural mismatch the SDK has no way to predict because the fee distribution is the validator's computation, not the signer's.
dev.node2 reproduced; local devnet did NOT because the devnet boots with
gasFeeSeparation.activationHeight: null→applyGasFeeSeparationgated off → no prepend → #861+#867 normalisation was sufficient.Fix
Snapshot
tx.content.gcr_editsvia structured clone BEFORE callingconfirmTransaction, then hash against the snapshot instead of the mutated array. The invariant we want to verify is "did the SDK ship the same edits GCRGeneration would regenerate?" — exactly what the snapshot captures.Fee edits live downstream of this check and become part of the signed validity data via the existing
signValidityDataflow — no consensus impact.Trace
applyGasFeeSeparation.ts:187mutatestx.content.gcr_editsin placevalidateTransaction.ts:133calls it insideconfirmTransactionendpointValidation.ts:35callsconfirmTransaction, then was reading the already-mutatedtx.content.gcr_editsTest plan
BROADCAST=1 bun transfer_10pct.mjs— expectvalid: trueCompanion PRs in the saga
Summary by CodeRabbit