Fix blockchain state mismatch across RPCs for the same head#950
Conversation
+ whitelist consensus abort
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libs/blockchain/mempool.ts (1)
309-315: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve duplicate-nonce rejection under the future-nonce policy.
The locked re-check now only rejects
txNonce <= accountNonce, so two transactions from the same sender with the same future nonce can both pass after serializing through the advisory lock. Keep accepting nonce gaps, but also reject an existing pending transaction for the same sender and nonce beforesave.🤖 Prompt for 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. In `@src/libs/blockchain/mempool.ts` around lines 309 - 315, The nonce validation in mempool handling now only checks against the on-chain account nonce, so duplicate future nonces can slip through after the advisory lock. In the same flow around the account lookup in mempool.ts, add an additional pending-transaction check for the sender and txNonce before calling save, while still allowing nonce gaps greater than accountNonce. Use the existing senderFrom, txNonce, and GCRMain/repository logic to reject duplicates consistently under the future-nonce policy.
🤖 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.
Outside diff comments:
In `@src/libs/blockchain/mempool.ts`:
- Around line 309-315: The nonce validation in mempool handling now only checks
against the on-chain account nonce, so duplicate future nonces can slip through
after the advisory lock. In the same flow around the account lookup in
mempool.ts, add an additional pending-transaction check for the sender and
txNonce before calling save, while still allowing nonce gaps greater than
accountNonce. Use the existing senderFrom, txNonce, and GCRMain/repository logic
to reject duplicates consistently under the future-nonce policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59ce8656-4575-4af6-be9a-7c9903e48a26
📒 Files selected for processing (12)
data/snapshot/gcr_main.jsonldata/snapshot/gcr_storageprogram.jsonldata/snapshot/identity_commitments.jsonldata/snapshot/manifest.jsondata/snapshot/validators.jsonlsrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/chainDb.tssrc/libs/blockchain/chainTransactions.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/blockchain/genesis/restoreSnapshot.tssrc/libs/blockchain/mempool.tssrc/libs/consensus/v2/PoRBFT.ts
✅ Files skipped from review due to trivial changes (1)
- data/snapshot/validators.jsonl
🚧 Files skipped from review as they are similar to previous changes (4)
- src/libs/blockchain/gcr/handleGCR.ts
- src/libs/blockchain/chainTransactions.ts
- src/libs/blockchain/chainBlocks.ts
- src/libs/consensus/v2/PoRBFT.ts
+ try: shuffle txs list before sort to test deterministic sort
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/libs/blockchain/routines/Sync.ts (2)
333-341: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCompute missing transactions by hash, not by list length.
A peer can return the right count with the wrong hashes or extra transactions, skipping the signer fallback and driving
verifyBlockAttrsinto a fatal mismatch. Build the expected-hash set first, discard extras, then fetch any missing hashes.🐛 Proposed fix
- if (txs.length < block.content.ordered_transactions.length) { + const masterSet = new Set(block.content.ordered_transactions) + const txByHash = new Map( + txs.filter(tx => masterSet.has(tx.hash)).map(tx => [tx.hash, tx]), + ) + const missingTxs = new Set( + [...masterSet].filter(hash => !txByHash.has(hash)), + ) + + if (missingTxs.size > 0) { log.error( "[fastSync] No transactions received for block: " + block.hash, ) log.debug("Trying to get transaction from signers") - - const masterSet = new Set(block.content.ordered_transactions) - const missingTxs = masterSet.difference(new Set(txs.map(tx => tx.hash)))for (const tx of res) { if (missingTxs.has(tx.hash)) { - txs.push(tx) + txByHash.set(tx.hash, tx) missingTxs.delete(tx.hash) } }if (missingTxs.size > 0) { ... process.exit(1) } } + + txs = block.content.ordered_transactions.map(hash => txByHash.get(hash)!)🤖 Prompt for 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. In `@src/libs/blockchain/routines/Sync.ts` around lines 333 - 341, The fast-sync fallback in Sync.ts is currently gated only on tx count, which can miss cases where the peer returns the wrong hashes or extra transactions. Update the transaction reconciliation logic in the block-processing path to build the expected hash set from block.content.ordered_transactions first, compare against the returned tx hashes, discard extras, and compute missingTxs strictly by hash before deciding whether to query signers. Keep the existing logging and signer fallback flow in the same area, but trigger it based on hash mismatches rather than txs.length.
522-538: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftVerify attrs and transactions before inserting the block.
Chain.insertBlock(...)runs beforeaskTxsForBlock(...)andverifyBlockAttrs(...). If attrs verification exits or fails afterward, the node can restart with the bad block already persisted and skip it via the existence check.🐛 Safer flow outline
- await Chain.insertBlock(block, []) - log.debug("Block inserted successfully") + const txs = await askTxsForBlock(block, peer) + const applied = await verifyBlockAttrs(block, txs) + + await Chain.insertBlock(block, []) + log.debug("Block inserted successfully")Apply the same ordering in
batchDownloadBlocks: fetch/resolve block transactions, runverifyBlockAttrs, then insert the block. Ideally, commit block, tx, and GCR updates in one storage transaction or rollback on any later failure.Also applies to: 785-793
🤖 Prompt for 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. In `@src/libs/blockchain/routines/Sync.ts` around lines 522 - 538, Move the block persistence step in Sync's fast sync flow so `Chain.insertBlock(...)` happens only after `askTxsForBlock(...)` and `verifyBlockAttrs(...)` succeed, using the same ordering already expected in `batchDownloadBlocks`. Locate the sequence around the fastSync block handling in `Sync.ts` and reorder it to fetch transactions, validate attributes, then insert the block; keep the peer merge and logging around the validated flow, and prefer a single storage transaction or rollback-safe path if later updates fail.
🤖 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 `@src/libs/blockchain/routines/Sync.ts`:
- Around line 515-520: The duplicate-block recheck in Sync should not be treated
as a failure when another path inserts the same block during verification. In
the Sync routine’s second existence check around Chain.getBlockByNumber and the
“Block already exists, skipping ...” branch, change the return path so the
concurrent duplicate insert is treated as a successful no-op instead of a sync
failure, and apply the same handling in the other matching duplicate-check
block.
- Around line 48-55: Remove the test-only shuffle from the production sync path
by eliminating the use of shuffleArray in Sync and restoring deterministic
ordering in the block verification flow. Update the relevant sync routine in
Sync.ts, especially the code around the block verification sequence that
currently depends on Math.random(), so it processes items in a stable order
instead of shuffling them. Ensure any calls or helpers tied to shuffleArray are
removed or replaced with deterministic logic.
---
Outside diff comments:
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 333-341: The fast-sync fallback in Sync.ts is currently gated only
on tx count, which can miss cases where the peer returns the wrong hashes or
extra transactions. Update the transaction reconciliation logic in the
block-processing path to build the expected hash set from
block.content.ordered_transactions first, compare against the returned tx
hashes, discard extras, and compute missingTxs strictly by hash before deciding
whether to query signers. Keep the existing logging and signer fallback flow in
the same area, but trigger it based on hash mismatches rather than txs.length.
- Around line 522-538: Move the block persistence step in Sync's fast sync flow
so `Chain.insertBlock(...)` happens only after `askTxsForBlock(...)` and
`verifyBlockAttrs(...)` succeed, using the same ordering already expected in
`batchDownloadBlocks`. Locate the sequence around the fastSync block handling in
`Sync.ts` and reorder it to fetch transactions, validate attributes, then insert
the block; keep the peer merge and logging around the validated flow, and prefer
a single storage transaction or rollback-safe path if later updates fail.
🪄 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: 5cf37eb8-965a-40ae-af5e-082de7886b06
📒 Files selected for processing (5)
src/libs/blockchain/chain.tssrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/routines/Sync.tssrc/libs/blockchain/validation/verifyBlock.tssrc/libs/consensus/v2/routines/deterministicOrder.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/libs/blockchain/chain.ts
- src/libs/blockchain/validation/verifyBlock.ts
- src/libs/blockchain/chainBlocks.ts
| // check exists again | ||
| exists = await Chain.getBlockByNumber(block.number) | ||
| if (exists) { | ||
| log.error("Block already exists, skipping ...") | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat a concurrent duplicate insert as success.
The first existence check returns true, but the second check returns false for the same final state. If another guarded path inserts the block during verification, this reports a sync failure even though the chain already has the block.
🐛 Proposed fix
if (exists) {
- log.error("Block already exists, skipping ...")
- return false
+ log.debug("Block already exists, skipping ...")
+ return true
}Also applies to: 778-783
🤖 Prompt for 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.
In `@src/libs/blockchain/routines/Sync.ts` around lines 515 - 520, The
duplicate-block recheck in Sync should not be treated as a failure when another
path inserts the same block during verification. In the Sync routine’s second
existence check around Chain.getBlockByNumber and the “Block already exists,
skipping ...” branch, change the return path so the concurrent duplicate insert
is treated as a successful no-op instead of a sync failure, and apply the same
handling in the other matching duplicate-check block.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
+ set shard size back to 4 + reenable tlsn + monitoring
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
Historical/older-binary blocks arrive with attrs === null (nullable per the AddAttrsToBlocks migration). Dereferencing block.attrs[...] unconditionally crashed the sync loop with an uncaught TypeError. Treat null attrs as 'no metadata' and skip the gcrAppliedTx* checks, per the migration contract.
|
@greptile review |
NotInShardError / ForgingEndedError are caught as graceful aborts and return without finalizing a block, but left exitReason empty. The finally-block finalized-tx assertion (txs.length !== blockTxs.length) then fired and hit process.exit(1) even though no block was expected. Tag these aborts with exitReason = consensusAborted and add it to the assertion's skip allowlist.
|
@greptile review |
confirmTransaction normalized only tx.content.from_ed25519_address; a tx that carries the sender in the legacy content.from field passed undefined into normalizePubkey -> forgeToHex(undefined), throwing and crashing the validator RPC before it could return a signed invalid ValidityData. Fall back to content.from when the ed25519 field is absent.
|
@greptile review |
| const txsToRemove = new Set([ | ||
| ...failedTxsToRemove, | ||
| ...blockTxs.map(tx => tx.hash), | ||
| ]) |
There was a problem hiding this comment.
The high-nonce retry path is defeated here: failedTxsToRemove excludes whitelisted failures, but txsToRemove then unions in every blockTxs hash, including those same whitelisted transactions. A TX_NONCE_INVALID_HIGH transaction is marked failed and intended to stay queued for a later nonce, but this set removes it from the mempool at finalization, so it never gets retried.
Artifacts
Repro: focused TypeScript harness for whitelisted high-nonce finalization removal
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: harness run output showing the whitelisted hash removed
- Keeps the command output available without making the summary code-heavy.
| for (const edit of nonceEdits) { | ||
| const acct = normalizeAccount(edit.account) | ||
| const expected = nonces[acct] + 1 | ||
|
|
||
| if (tx.content.nonce === expected) { | ||
| validTxs.push(tx) | ||
| nonces[acct]++ | ||
| continue txLoop | ||
| } else if (tx.content.nonce < expected) { | ||
| // Update the transaction status as failed, but include in block | ||
| tx.status = TRANSACTION_STATUS.FAILED | ||
| tx.attrs = { | ||
| code: ErrorCode.TX_NONCE_INVALID_LOW, | ||
| message: `Invalid nonce edit. Expected ${expected}, got ${tx.content.nonce}`, | ||
| reference_block: tx.reference_block, | ||
| } | ||
| validTxs.push(tx) | ||
|
|
||
| log.debug( | ||
| `[TX_NONCE_INVALID_LOW] including tx as failed: ${tx.hash}: invalid nonce edit`, | ||
| ) | ||
| log.debug( | ||
| "[TX_NONCE_INVALID_LOW] Invalid nonce edit for account: " + | ||
| acct, | ||
| ) | ||
| log.debug( | ||
| `[TX_NONCE_INVALID_LOW] Expected ${expected}, got ${tx.content.nonce}`, | ||
| ) | ||
| } else { | ||
| failedTxs.push({ | ||
| txhash: tx.hash, | ||
| code: ErrorCode.TX_NONCE_INVALID_HIGH, | ||
| message: `Invalid nonce edit. Expected ${expected}, got ${tx.content.nonce}`, | ||
| }) | ||
|
|
||
| log.debug(`[TX_NONCE_INVALID_HIGH] keeping tx: ${tx.hash}`) | ||
| log.debug( | ||
| "[TX_NONCE_INVALID_HIGH] Invalid nonce edit for account: " + | ||
| acct, | ||
| ) | ||
| log.debug( | ||
| `[TX_NONCE_INVALID_HIGH] Expected ${expected}, got ${tx.content.nonce}`, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
continue txLoop in the low-nonce and high-nonce branches allows multi-nonce-edit transactions to be double-inserted
For a tx that carries nonce edits for more than one account, the inner for (const edit of nonceEdits) loop falls through after the low-nonce or high-nonce branch. Concretely:
- Low-nonce then match (edit A is stale, edit B matches): the tx is marked
FAILEDand pushed tovalidTxson edit A's iteration, then pushed tovalidTxsa second time on edit B's iteration — the same tx object appears twice in the block. - High-nonce then match (edit A is future, edit B matches): the tx is pushed to
failedTxs(kept in mempool) on edit A's iteration, then also pushed tovalidTxson edit B's iteration — the tx lands in both lists, is inserted into the block and simultaneously kept in the mempool for the next round.
Both cases corrupt the blockTxs set. Each mismatched branch needs continue txLoop immediately after pushing to its destination, mirroring the nonce === expected branch which already does this correctly.
This pull request fixes the state mismatch across RPCs for a multinode network. The mismatch occured when the node had more nodes than the ones participating in the consensus, and the non-participants synced the gcr via the sync routine.
What this PR introduces the following behavioral changes to the nodes and the network:
FAILEDand the reason for failure provided as transaction metadata intx.attrs.codeandtx.attrs.message. The user can fetch the transaction by hash and check if the tx was confirmed or it failed..attrsobject on both transactions and blocks. The field is currently used to track the reference block and failure reasons for transactions, and applied transactions order for blocks. The fieldreference_block.Mempool.addTransactioncalls are gated with a mutex to prevent tampering with the mempool during merging.NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE)all over the main sync and consensus source files. The code guards against behavior that causes state drift across RPCs. Please do not remove for now.Summary by CodeRabbit