fix(validation): symmetric gcr_edits normalisation on both sides of the hash compare#867
Conversation
…he hash compare dev.node2 production logs after PR #861 still showed: gcrEditsHash: 16f9495100b98... (constant across runs) txGcrEditsHash: 6dabe23e82ad... (varies per run) → GCREdit mismatch The regen side normalises (txhash blanked + serializer-walk) but the tx-side input was hashed straight from `tx.content.gcr_edits` without the same treatment. Whatever field varies in the wire payload — most likely a populated `txhash` on inbound edits some SDK flow ships — ended up in the hash and diverged from the blanked-regen value. The right thing is symmetry: run both sides through the IDENTICAL normalisation. After this patch: - txhash: forced empty on both regen and tx-side edits before hash - amount shape: both sides walk through `normaliseGcrEditsForHash` so the serializer's per-edit-type rewrite applies uniformly Any future SDK or peer that ships gcr_edits with a different edit-internal shape (re-keyed JSON object order, populated txhash, post-fork OS string vs pre-fork DEM number on the amount field) now hashes identically on both sides of the comparison. The compare is truly checking "did the SDK generate the same logical edit set as GCRGeneration.generate?" instead of "does the SDK happen to produce byte-identical JSON to GCRGeneration.generate?". Verified by manual repro on dev.node2 after a rebuild that picks this up: - pre-fix: confirm() returns valid=false, "GCREdit mismatch" - post-fix: confirm() returns valid=true, broadcast lands (Pre-existing PR #861 normalisation was load-bearing on the regen side; this PR makes the symmetric move on the tx-side input.)
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? |
|
Warning Review limit reached
More reviews will be available in 10 minutes and 44 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 (1)
✨ 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 SummaryThis PR fixes a persistent
Confidence Score: 4/5The change is narrowly scoped to the hash-comparison path, touching no DB writes, no error-handling paths, and no other validation logic. The fix is logically correct and symmetric with the pre-existing regen side. The only gap is the absence of an automated test covering the normalisation invariant — if the SDK serializer behaviour changes, the divergence would only be caught by a live integration run. No files require special attention; src/libs/network/endpointValidation.ts is the single changed file and the logic is self-contained. Important Files Changed
Reviews (1): Last reviewed commit: "fix(validation): symmetric gcr_edits nor..." | Re-trigger Greptile |
…ion canary (#870) Two diagnostic-only log lines so the dev.node2 mystery ("source has fix #867+#869, container reports the right SHA, mismatch still happens") becomes traceable from the log stream alone: 1) SYMMETRIC-NORMALISE-V2 canary — distinctive marker proves the running binary actually executed the post-PR-#867 branch. Grep for the literal string on the host; missing = runtime is using stale bytes despite the SHA. 2) Mismatch dump — when the hash compare fails, emit three JSON blobs back-to-back: - normalisedTxEdits (what tx-side hash sees) - normalisedRegen (what regen-side hash sees) - tx.content.gcr_edits raw (pre-normalisation) so we can eyeball-diff the exact field that diverges instead of guessing from the truncated hashes. Pure logging — no consensus / validation behaviour change. Strip after the root cause is found. Co-authored-by: tcsenpai <tcsenpai@discus.sh>
…ion so applyGasFeeSeparation prepends don't break the hash compare (#871) 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. Co-authored-by: tcsenpai <tcsenpai@discus.sh>
Production repro
dev.node2 (rebuilt from
0f8ead5d, which already includes PR #861) keeps logging:The regen path was normalised by #861 (txhash blanked + serializer-walk via the envelope) but the tx-side input was hashed straight from
tx.content.gcr_editswithout the same treatment. Whatever field varies in the wire payload — most likely a populatedtxhashon inbound edits some SDK flow ships, plus the post-fork OS-string vs pre-fork DEM-number ambiguity onamount— ended up in the hash and diverged from the blanked-regen value.Fix
Symmetric normalisation on both sides:
txhashforced empty on regen and tx-side edits before hashnormaliseGcrEditsForHashenvelope so the serializer's per-edit-type rewrite applies uniformlyThe compare is now truly asking "did the SDK generate the same logical edit set as
GCRGeneration.generate?" instead of "does the SDK happen to produce byte-identical JSON toGCRGeneration.generate?".Test plan
bun run type-check-ts— no new errorsBROADCAST=1 bun transfer_10pct.mjsreturnsvalid: trueinstead ofGCREdit mismatch