Fix blockchain state mismatch across RPCs for the same head#950
Fix blockchain state mismatch across RPCs for the same head#950cwilvx wants to merge 69 commits into
Conversation
+ add comment: NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE) to find the code in next debug session
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? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds attrs and status schema support for transactions and blocks, changes transaction validation and hashing rules, serializes mempool admission with a mutex, rewires consensus around structured transaction tracking, and strengthens block sync and verification. It also updates snapshot data and runtime defaults. ChangesConsensus Stability and Blockchain Correctness
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Greptile SummaryThis PR changes block and transaction handling to keep synced nodes aligned on the same chain head. The main changes are:
Confidence Score: 4/5The transaction failure-reporting path needs correction before merge because persisted failed transactions are exposed to clients with the wrong lifecycle state. The changed consensus behavior is broad, but the identified issue is focused and was confirmed with a runtime harness exercising the real status helper behavior. src/libs/blockchain/chainTransactions.ts
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| // 2. Resolve the eligible signer set for this block. | ||
| const signatures = block.validation_data?.signatures | ||
| if (!signatures || typeof signatures !== "object") { | ||
| return { valid: false, reason: "block has no validation_data.signatures" } | ||
| // Verify last block hash is the same as this block's previous hash | ||
| const lastBlockHash = await Chain.getLastBlockHash() | ||
| if (lastBlockHash !== block.content.previousHash) { | ||
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | ||
| log.error( | ||
| `last block hash mismatch: last block hash ${lastBlockHash}, block ${block.number}'s previous hash ${block.content.previousHash}`, | ||
| ) | ||
| process.exit(1) | ||
|
|
There was a problem hiding this comment.
process.exit(1)
The return { valid: false, ... } block is unreachable — process.exit(1) terminates the process before it can execute. Any caller of verifyBlock that expects a graceful { valid: false } for a chain-continuity failure will instead receive a crashed node. During any scenario where a block with a non-matching previousHash reaches syncBlock (fork detection, out-of-order delivery, race with another sync path), the entire node process dies rather than the block being cleanly rejected.
| const confirmed = await Chain.getExistingTransactionHashes( | ||
| finalTxs.map(t => t.hash), | ||
| ) | ||
|
|
||
| if (confirmed.size > 0) { | ||
| log.error("Found confirmed txs during tx application") | ||
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | ||
| // fetch confirmed txs from db | ||
| const confirmedTxs = await Chain.getTransactionsFromHashes(Array.from(confirmed)) | ||
| log.error("Confirmed txs: " + JSON.stringify(Array.from(confirmed), null, 2)) | ||
| log.error("Confirmed txs full: " + JSON.stringify(confirmedTxs.map(t => t.hash), null, 2)) | ||
| process.exit(1) |
There was a problem hiding this comment.
process.exit(1)
The finalTxs.filter(...) block that removes already-confirmed transactions from the apply set is unreachable — process.exit(1) terminates the process before it executes. Previously this was a graceful defensive filter; now it is a hard crash. Any legitimate race where a tx lands in both the mempool apply set and the transactions table (e.g., a block was inserted by the broadcast path just before consensus reaches the apply step) will bring the node down permanently instead of skipping the duplicate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/libs/blockchain/block.ts (1)
27-54: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInitialize
attrsin the constructor.
attrsis typed asRecord<string, any> | null, but newBlock()instances leave itundefined, so freshly forged/broadcast blocks may not satisfy the new metadata contract.Proposed fix
constructor() { this.number = null this.hash = null // Calculated on the content this.status = null + this.attrs = null this.next_proposer = ""🤖 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/block.ts` around lines 27 - 54, Initialize the attrs field in the Block constructor so new Block() instances always have a defined metadata object. The current Block class sets number, hash, status, content, proposer, and validation_data but leaves attrs undefined despite the Record<string, any> | null type, so assign an explicit initial value there. Update the constructor in Block to include attrs alongside the other default fields and keep the initialization consistent with the block metadata contract.src/libs/blockchain/validation/verifyBlock.ts (1)
62-122: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftVerify signatures against the expected shard, not every active validator.
This accepts any
thresholdactive validators fromGCR.getGCRValidatorsAtBlock(...); it does not prove those signers were selected for the block’s shard. A non-shard validator quorum could produce a block that sync accepts. Recompute/load the expected shard forblock.numberand derive bothvalidatorIdentitiesandthresholdfrom that shard membership.🤖 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/validation/verifyBlock.ts` around lines 62 - 122, The verification in verifyBlock currently accepts signatures from the full validator set returned by GCR.getGCRValidatorsAtBlock, which can allow a quorum outside the block’s intended shard. Update verifyBlock to load or recompute the expected shard for block.number, derive validatorIdentities from that shard membership, and base the signature threshold on the shard size rather than getSharedState.shardSize so only the correct shard’s signers can satisfy validation.src/libs/blockchain/routines/Sync.ts (2)
474-493: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftMake block, tx, and GCR sync atomic.
Both sync paths persist the block before fetching/verifying txs and applying GCR state. If tx fetch, attrs verification, GCR application, or tx insertion fails afterward, the node can retain an advanced block head with missing transaction/GCR state. Verify/fetch first, then persist block+transactions+GCR effects as one unit or rollback on failure.
Also applies to: 731-741
🤖 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 474 - 493, The fast sync flow in Sync should not persist the block before tx/GCR work succeeds; `askTxsForBlock`, `verifyBlockAttrs`, `syncGCRTables`, and the later tx insertion path must be treated as one atomic unit. Refactor the logic around the block insertion/verification sequence so you fetch and validate first, then commit the block, transactions, and GCR state together, or roll back/abort on any failure. Use the existing `mergePeerlist`, `askTxsForBlock`, `verifyBlockAttrs`, and `syncGCRTables` flow as the anchor points, and apply the same atomicity change to the other sync block referenced by the comment.
1130-1144: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep
fastSyncAbortedasserted until the timed-out sync finishes.Promise.racereturns early here, butsyncLock.runExclusive(...)keeps running in the background, and thefinallyclearsfastSyncAbortedbefore that work exits. That lets the in-flight sync continue mutating state after timeout. Hold the abort flag until the lock settles, or await the locked task after signaling abort.🤖 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 1130 - 1144, The fast sync timeout handling in Sync.ts clears fastSyncAborted too early because Promise.race returns before syncLock.runExclusive(async () => fastSyncRoutine(peers)) has finished. Update the fastSync path so the abort flag stays set until the locked sync task fully settles, either by awaiting the runExclusive task after setting the timeout state or by deferring the flag reset until the background work completes. Keep the fix localized around fastSyncRoutine, syncLock.runExclusive, and the finally cleanup that currently resets getSharedState.fastSyncAborted.src/libs/l2ps/L2PSConsensus.ts (1)
466-526: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winBuild a full transaction shape before insertion.
Chain.insertTransactionnow serializes fields from the transaction object itself. Herestatusis nested undercontent,blockNumberis missing, andcontent.transaction_feeis missing, so the batch tx is either persisted with null metadata or fails inTransaction.toRawTransaction.🐛 Proposed fix
const l1BatchTx = { type: "l2psBatch", hash: `0x${batchHash}`, + status: TRANSACTION_STATUS.CONFIRMED, + blockNumber, signature: { type: "ed25519", data: "", // System-generated, no actual signature needed }, content: { type: "l2psBatch", from: "l2ps:consensus", // System sender for L2PS batch + from_ed25519_address: "l2ps:consensus", to: "l2ps:batch", amount: 0, nonce: blockNumber, timestamp: Date.now(), - status: TRANSACTION_STATUS.CONFIRMED, + transaction_fee: { + network_fee: 0, + rpc_fee: 0, + additional_fee: 0, + rpc_address: null, + }, data: [🤖 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/l2ps/L2PSConsensus.ts` around lines 466 - 526, The batch transaction object passed to Chain.insertTransaction is incomplete because status is nested inside content, while blockNumber and content.transaction_fee are missing, which can lead to null metadata or serialization failures in Transaction.toRawTransaction. Update the l1BatchTx shape in L2PSConsensus so all fields required by the raw transaction serializer are present at the top level and inside content as expected, including status, blockNumber, and transaction_fee. Verify the transaction built in this block matches the full shape used by Chain.insertTransaction before calling it.src/libs/blockchain/routines/validateTransaction.ts (1)
485-485: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove stale
assignNonce/expectedPriorcomments
Update the outdated docstrings insrc/libs/blockchain/mempool.ts,src/forks/forkConfig.ts, andsrc/libs/blockchain/validation/verifyGcrEdits.tsso they match the current nonce flow.🤖 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/validateTransaction.ts` at line 485, Remove the outdated assignNonce/expectedPrior docstrings so they match the current nonce handling flow. Update the stale comments in the relevant logic within mempool.ts, forkConfig.ts, and verifyGcrEdits.ts to describe the current behavior, and ensure any references to nonce assignment or expected prior state are aligned with the actual implementation in the affected functions/classes.
🧹 Nitpick comments (6)
src/model/entities/Mempool.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow
MempoolTx.statustoTransactionStatus
string | TransactionStatusstill collapses tostring, so this field doesn’t enforce the canonical status set. If this entity should only persistpending | confirmed | failed, make the propertyTransactionStatusand keep any legacy/raw coercion at the deserialization boundary instead.🤖 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/model/entities/Mempool.ts` at line 1, MempoolTx.status is still typed too loosely, so it doesn’t enforce the canonical transaction status set; update the Mempool entity to use TransactionStatus directly for the status field. Keep any raw string-to-status normalization at the deserialization boundary in the Mempool model rather than widening the property type, and verify any constructors/parsers that assign status still map legacy values before they reach MempoolTx.Source: Linters/SAST tools
src/libs/blockchain/transaction.ts (1)
51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
statusnarrowed toTransactionStatus | null.string | TransactionStatusis juststring, so the field loses its status constraint; normalize any legacy values before assignment instead.🤖 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/transaction.ts` around lines 51 - 53, The Transaction model’s status field is too broad because `TransactionStatus` is already a string type, so `string | TransactionStatus` collapses the constraint and allows invalid values. Update the `status` declaration in `Transaction` to `TransactionStatus | null` only, and normalize any legacy string inputs before assigning them to `status` in the constructor or related assignment path so only valid transaction statuses are stored.Source: Linters/SAST tools
src/libs/blockchain/routines/Sync.ts (1)
321-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
verifyBlockAttrsto satisfy the complexity gate.Sonar reports this function over the allowed cognitive complexity. Extract the count, block-number, deterministic-order, and attrs checks into small helpers while addressing the sync-integrity checks above.
🤖 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 321 - 443, `verifyBlockAttrs` is too complex and needs to be split into smaller units to pass the cognitive complexity gate. Extract the major checks into dedicated helpers from `Sync.ts`—for example the transaction-count validation, block-number validation, deterministic order comparison, and attribute integrity checks—then have `verifyBlockAttrs` orchestrate them and return the applied txs. Keep the existing `log.error` and `process.exit(1)` behavior inside the helpers so the sync-integrity logic remains unchanged, and use the existing symbols `verifyBlockAttrs`, `orderDeterministically`, `Hashing.sha256`, and `TRANSACTION_STATUS.CONFIRMED` to guide the refactor.Source: Linters/SAST tools
src/libs/blockchain/gcr/gcr.ts (2)
154-171: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getAccountNoncesis O(n·m); build a lookup map once.
accounts.find(...)inside the per-pubkeyreducemakes this quadratic in batch size. A singleMapkeeps it linear.⚡ Proposed fix
- // return a map of pubkey to nonce, 0 for missing accounts - return pubkeys.reduce((acc, pubkey) => { - acc[pubkey] = - accounts.find(account => account.pubkey === pubkey)?.nonce ?? 0 - return acc - }, {}) + // return a map of pubkey to nonce, 0 for missing accounts + const nonceByPubkey = new Map( + accounts.map(account => [account.pubkey, account.nonce]), + ) + return pubkeys.reduce<Record<string, number>>((acc, pubkey) => { + acc[pubkey] = nonceByPubkey.get(pubkey) ?? 0 + return acc + }, {})🤖 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/gcr/gcr.ts` around lines 154 - 171, getAccountNonces currently does a linear accounts.find() for every pubkey inside the reduce, making the method quadratic for large batches. Update the GCR.getAccountNonces flow to build a single lookup map from the queried accounts first, then use that map during the pubkeys.reduce() so each pubkey resolves in constant time while preserving the existing 0 default for missing accounts.
143-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSelect only
nonceto avoid loading the full account row.
getAccountNonceruns on the per-transaction validation path (confirmTransaction) yet fetches the entireGCRMainrow — including the heavyidentities,points, andreferralInfojsonb columns — just to read a single integer.getAccountBalanceabove already usesselectfor the same reason.⚡ Proposed fix
const account = await gcrRepository.findOne({ where: { pubkey }, + select: ["nonce"], })🤖 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/gcr/gcr.ts` around lines 143 - 152, `GCR.getAccountNonce` is fetching the full `GCRMain` entity when only `nonce` is needed in the `confirmTransaction` path. Update the repository query in `getAccountNonce` to select only the `nonce` field, following the same pattern used by `getAccountBalance`, and keep the fallback behavior returning 0 when no row is found.src/libs/blockchain/mempool.ts (1)
309-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNonce recheck logic is fine, but the rationale comment above is now stale.
The new rule (
txNonce <= accountNonce→ reject) no longer queries pending mempool count, andGCRNonceRoutinesno longer carries theexpectedPriorcheck. The large comment block preceding this transaction (re:account.nonce + 1 + pendingCountand "consensus-sideexpectedPriorcheck inGCRNonceRoutines(PR 3)") now misdescribes the safety model on a consensus-critical path. Please update it so future readers don't rely on a guard that was removed.🤖 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 - 328, Update the stale rationale comment near the nonce validation in mempool addTransaction logic so it matches the current behavior. The comment should describe the actual guard used by Mempool.addTransaction and GCRNonceRoutines now that the pending mempool count and expectedPrior check are gone, and should not mention the removed `account.nonce + 1 + pendingCount` rule or the old consensus-side safeguard. Keep the explanation aligned with the `txNonce <= accountNonce` rejection path and the returned invalid nonce error.
🤖 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/chainBlocks.ts`:
- Around line 271-274: The transaction status set in chainBlocks.ts currently
includes FAILED, which is fine for ingestion but not for confirmed-only side
effects. Update the downstream projection logic in the affected handlers
(including persistConfirmedTransactionProjection and the IdentityCommitment
blockNumber update path) so they only operate on transactions whose status is
CONFIRMED, while keeping FAILED rows available elsewhere. Use the existing
transactionEntities/status handling in chainBlocks.ts to filter before applying
these confirmed-only updates.
- Around line 248-292: The block transaction validation in chainBlocks.ts is too
permissive because it only runs when both arrays are non-empty and accepts any
subset of ordered_transactions. Update the transaction check around the
blockTxs/ordered_transactions handling so saving a non-empty block requires an
exact hash match before proceeding, or ensure the sync flow fetches and verifies
transactions before insertBlock is called. Use the existing blockTxs,
block.content.ordered_transactions, and transactionEntities logic to enforce the
full set match before setting blockNumber or continuing.
In `@src/libs/blockchain/gcr/handleGCR.ts`:
- Around line 170-172: The exported normalizer normalizePubkey currently assumes
a non-null string or Uint8Array and can fall through to forgeToHex on undefined
when callers use from_ed25519_address directly. Update normalizePubkey to guard
nullish input explicitly by rejecting or safely handling it before calling
forgeToHex, or adjust the deterministic-order caller to always pass
from_ed25519_address || from so the helper never receives undefined.
- Around line 793-800: The confirmed-transaction guard in handleGCR should not
terminate the process with process.exit(1); instead, make this branch fail the
block-apply flow by returning a typed failure or throwing a domain-specific
error that sync/consensus can catch. Update the logic around confirmed.size,
Chain.getTransactionsFromHashes, and the surrounding tx application path so the
duplicate-confirmed-tx condition aborts application cleanly without killing the
node.
In `@src/libs/blockchain/mempool.ts`:
- Line 39: The shared Mutex on Mempool.lock is never reassigned, so update the
Mempool class to make lock readonly and keep the existing static mutex instance
intact. Use the Mempool.lock declaration as the only place to change, ensuring
runExclusive callers continue to use the same mutual exclusion guard and
accidental reassignment is prevented.
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 375-418: The GCR sync logic in Sync.ts is trusting peer-controlled
block.attrs and tx.status to decide which transactions are applied, so make
local execution authoritative in the fastSync path and in
syncGCRTables/HandleGCR.applyTransactions comparisons. Use the result of
applying transactions locally as the source of truth, then compare the
successful hashes against block.attrs["gcrAppliedTxsHash"] and related metadata,
or otherwise move the relevant attrs into the signed block content; keep the
mismatch checks and logging around the existing applied/sorted transaction flow.
- Around line 378-418: Guard all reads of block.attrs in the fast sync check
inside Sync.ts before indexing into it, because it can be null for pre-attrs or
malformed blocks. Update the mismatch handling around the gcrAppliedTxCount and
gcrAppliedTxsHash comparisons to early-exit or reject when attrs is missing
instead of dereferencing it directly. Use the existing fastSync block-validation
logic in Sync.ts as the place to add the null check, and keep the current error
logging and process.exit paths for the valid attrs case.
In `@src/libs/blockchain/routines/validateTransaction.ts`:
- Around line 89-104: The nonce validation in validateTransaction is using
from_ed25519_address, while Mempool.addTransaction relies on content.from, so
the same transaction can be evaluated against different sender identities.
Update validateTransaction to read the nonce using the same canonical account
key used by the mempool path, and make sure the chosen identifier is normalized
consistently before calling GCR.getAccountNonce. Keep the nonce comparison and
error handling unchanged, but ensure both validation paths reference the same
sender field.
In `@src/libs/blockchain/validation/verifyBlock.ts`:
- Around line 47-59: In verifyBlock, the lastBlockHash mismatch path should
reject the block instead of terminating the process. Remove the process.exit(1)
call in the Chain.getLastBlockHash / block.content.previousHash check, and
return the existing invalid result object with the mismatch reason so
peer-supplied blocks are handled as a normal validation failure.
In `@src/libs/communications/broadcastManager.ts`:
- Line 10: Remove the unused Mutex import from the broadcastManager module since
it is not referenced anywhere in the file and is triggering the lint/Sonar
warning. Update the import list at the top of the file and keep the rest of the
broadcastManager implementation unchanged.
In `@src/libs/consensus/v2/PoRBFT.ts`:
- Around line 713-763: Reject multi-nonce transactions before the nonce-edit
loop in PoRBFT’s transaction validation path mutates validTxs. In the tx
processing logic around the nonceEdits handling, first detect if
tx.content.gcr_edits contains more than one edit of type "nonce" and mark the
transaction failed/rejected immediately instead of iterating per edit. This
ensures each tx is validated once, prevents duplicate pushes on the low-nonce
path, and avoids accepting a tx after only the first matching nonce edit while
skipping the rest.
- Around line 453-491: The final transaction sanity check in PoRBFT still runs
after a handled AbortConsensusError and can incorrectly exit the process during
a rollback. Update the consensus flow around updateValidatorPhase(), the
catch/rollback path, and the final txs/blockTxs verification so that this
post-check is skipped or guarded when exitReason indicates a handled abort like
blockTimestampNotReceived or voteError. Keep the mismatch logging in the same
transaction validation block, but ensure process.exit(1) only happens for truly
unrecoverable cases.
In `@src/libs/network/endpointValidation.ts`:
- Around line 105-111: The mismatch-dump error handling in
handleValidateTransaction is losing useful diagnostics by converting non-Error
values with String(dumpErr), which turns plain objects into [object Object].
Update the catch block in endpointValidation.ts to preserve object details when
logging mismatch dump failures, using the existing log.error call and the
dumpErr symbol so the message includes structured information instead of
collapsing it to a generic string.
In `@src/libs/network/handlers/identityHandlers.ts`:
- Around line 28-30: The RPC error payload in identityHandlers should not
stringify non-Error thrown values, because that turns object errors into
"[object Object]" and hides details. Update the error handling around
response.extra to preserve the original object when error is not an Error, and
only use error.message for real Error instances. Keep the fix localized to the
identityHandlers response building logic so the payload retains the full thrown
value for downstream consumers.
In `@src/libs/network/manageGCRRoutines.ts`:
- Around line 265-268: The fast-path dedupe branch in manageGCRRoutines should
return the same “already processed” response shape as
BroadcastManager.handleNewBlock, not a plain string. Update the block.number <=
getSharedState.lastBlockNumber path to populate the structured payload including
syncData so peers receive the same contract regardless of which dedupe path
handles the block. Make the change in the logic around
getSharedState.lastBlockNumber and keep the response consistent with the
existing already-processed handling.
In `@src/model/entities/Blocks.ts`:
- Around line 33-34: The overwrite path in chainBlocks should also persist
attrs, since the existingBlock && position branch currently only copies
block.attrs into newBlock and leaves the stored row stale. Update the block
rewrite logic in chainBlocks so the existing block entity’s attrs field is
assigned from the incoming block before saving, keeping it consistent with the
Blocks entity’s attrs column.
In `@src/utilities/sharedState.ts`:
- Around line 495-508: Make consensusInfo best-effort in sharedState’s /info
path: the getCommonValidatorSeed() call and secman.secretary.identity access can
throw when consensus or secretary state is unavailable. Update the consensusInfo
block to safely read SecretaryManager data and wrap or guard the common
validator seed lookup so the rest of the /info response still succeeds, leaving
optional fields unset when unavailable.
---
Outside diff comments:
In `@src/libs/blockchain/block.ts`:
- Around line 27-54: Initialize the attrs field in the Block constructor so new
Block() instances always have a defined metadata object. The current Block class
sets number, hash, status, content, proposer, and validation_data but leaves
attrs undefined despite the Record<string, any> | null type, so assign an
explicit initial value there. Update the constructor in Block to include attrs
alongside the other default fields and keep the initialization consistent with
the block metadata contract.
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 474-493: The fast sync flow in Sync should not persist the block
before tx/GCR work succeeds; `askTxsForBlock`, `verifyBlockAttrs`,
`syncGCRTables`, and the later tx insertion path must be treated as one atomic
unit. Refactor the logic around the block insertion/verification sequence so you
fetch and validate first, then commit the block, transactions, and GCR state
together, or roll back/abort on any failure. Use the existing `mergePeerlist`,
`askTxsForBlock`, `verifyBlockAttrs`, and `syncGCRTables` flow as the anchor
points, and apply the same atomicity change to the other sync block referenced
by the comment.
- Around line 1130-1144: The fast sync timeout handling in Sync.ts clears
fastSyncAborted too early because Promise.race returns before
syncLock.runExclusive(async () => fastSyncRoutine(peers)) has finished. Update
the fastSync path so the abort flag stays set until the locked sync task fully
settles, either by awaiting the runExclusive task after setting the timeout
state or by deferring the flag reset until the background work completes. Keep
the fix localized around fastSyncRoutine, syncLock.runExclusive, and the finally
cleanup that currently resets getSharedState.fastSyncAborted.
In `@src/libs/blockchain/routines/validateTransaction.ts`:
- Line 485: Remove the outdated assignNonce/expectedPrior docstrings so they
match the current nonce handling flow. Update the stale comments in the relevant
logic within mempool.ts, forkConfig.ts, and verifyGcrEdits.ts to describe the
current behavior, and ensure any references to nonce assignment or expected
prior state are aligned with the actual implementation in the affected
functions/classes.
In `@src/libs/blockchain/validation/verifyBlock.ts`:
- Around line 62-122: The verification in verifyBlock currently accepts
signatures from the full validator set returned by GCR.getGCRValidatorsAtBlock,
which can allow a quorum outside the block’s intended shard. Update verifyBlock
to load or recompute the expected shard for block.number, derive
validatorIdentities from that shard membership, and base the signature threshold
on the shard size rather than getSharedState.shardSize so only the correct
shard’s signers can satisfy validation.
In `@src/libs/l2ps/L2PSConsensus.ts`:
- Around line 466-526: The batch transaction object passed to
Chain.insertTransaction is incomplete because status is nested inside content,
while blockNumber and content.transaction_fee are missing, which can lead to
null metadata or serialization failures in Transaction.toRawTransaction. Update
the l1BatchTx shape in L2PSConsensus so all fields required by the raw
transaction serializer are present at the top level and inside content as
expected, including status, blockNumber, and transaction_fee. Verify the
transaction built in this block matches the full shape used by
Chain.insertTransaction before calling it.
---
Nitpick comments:
In `@src/libs/blockchain/gcr/gcr.ts`:
- Around line 154-171: getAccountNonces currently does a linear accounts.find()
for every pubkey inside the reduce, making the method quadratic for large
batches. Update the GCR.getAccountNonces flow to build a single lookup map from
the queried accounts first, then use that map during the pubkeys.reduce() so
each pubkey resolves in constant time while preserving the existing 0 default
for missing accounts.
- Around line 143-152: `GCR.getAccountNonce` is fetching the full `GCRMain`
entity when only `nonce` is needed in the `confirmTransaction` path. Update the
repository query in `getAccountNonce` to select only the `nonce` field,
following the same pattern used by `getAccountBalance`, and keep the fallback
behavior returning 0 when no row is found.
In `@src/libs/blockchain/mempool.ts`:
- Around line 309-328: Update the stale rationale comment near the nonce
validation in mempool addTransaction logic so it matches the current behavior.
The comment should describe the actual guard used by Mempool.addTransaction and
GCRNonceRoutines now that the pending mempool count and expectedPrior check are
gone, and should not mention the removed `account.nonce + 1 + pendingCount` rule
or the old consensus-side safeguard. Keep the explanation aligned with the
`txNonce <= accountNonce` rejection path and the returned invalid nonce error.
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 321-443: `verifyBlockAttrs` is too complex and needs to be split
into smaller units to pass the cognitive complexity gate. Extract the major
checks into dedicated helpers from `Sync.ts`—for example the transaction-count
validation, block-number validation, deterministic order comparison, and
attribute integrity checks—then have `verifyBlockAttrs` orchestrate them and
return the applied txs. Keep the existing `log.error` and `process.exit(1)`
behavior inside the helpers so the sync-integrity logic remains unchanged, and
use the existing symbols `verifyBlockAttrs`, `orderDeterministically`,
`Hashing.sha256`, and `TRANSACTION_STATUS.CONFIRMED` to guide the refactor.
In `@src/libs/blockchain/transaction.ts`:
- Around line 51-53: The Transaction model’s status field is too broad because
`TransactionStatus` is already a string type, so `string | TransactionStatus`
collapses the constraint and allows invalid values. Update the `status`
declaration in `Transaction` to `TransactionStatus | null` only, and normalize
any legacy string inputs before assigning them to `status` in the constructor or
related assignment path so only valid transaction statuses are stored.
In `@src/model/entities/Mempool.ts`:
- Line 1: MempoolTx.status is still typed too loosely, so it doesn’t enforce the
canonical transaction status set; update the Mempool entity to use
TransactionStatus directly for the status field. Keep any raw string-to-status
normalization at the deserialization boundary in the Mempool model rather than
widening the property type, and verify any constructors/parsers that assign
status still map legacy values before they reach MempoolTx.
🪄 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: 259cdeca-3d61-4946-8b99-67cae1090f07
📒 Files selected for processing (41)
data/snapshot/manifest.jsondata/snapshot/validators.jsonlscripts/runsrc/config/defaults.tssrc/errors/codes.tssrc/libs/blockchain/block.tssrc/libs/blockchain/chain.tssrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/chainGenesis.tssrc/libs/blockchain/chainTransactions.tssrc/libs/blockchain/gcr/gcr.tssrc/libs/blockchain/gcr/gcr_routines/GCRNonceRoutines.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/blockchain/mempool.tssrc/libs/blockchain/routines/Sync.tssrc/libs/blockchain/routines/validateTransaction.tssrc/libs/blockchain/transaction.tssrc/libs/blockchain/validation/txValidator.tssrc/libs/blockchain/validation/verifyBlock.tssrc/libs/blockchain/validation/verifyGcrEdits.tssrc/libs/communications/broadcastManager.tssrc/libs/consensus/v2/PoRBFT.tssrc/libs/consensus/v2/routines/broadcastBlockHash.tssrc/libs/consensus/v2/routines/deterministicOrder.tssrc/libs/consensus/v2/routines/getCommonValidatorSeed.tssrc/libs/consensus/v2/routines/getShard.tssrc/libs/consensus/v2/routines/manageProposeBlockHash.tssrc/libs/l2ps/L2PSConsensus.tssrc/libs/network/dtr/dtrmanager.tssrc/libs/network/endpointExecution.tssrc/libs/network/endpointValidation.tssrc/libs/network/handlers/identityHandlers.tssrc/libs/network/handlers/miscHandlers.tssrc/libs/network/manageGCRRoutines.tssrc/migrations/1782679232721-AddAttrsToTransactions.tssrc/migrations/1782679300000-AddAttrsToBlocks.tssrc/model/entities/Blocks.tssrc/model/entities/Mempool.tssrc/model/entities/Transactions.tssrc/utilities/constants.tssrc/utilities/sharedState.ts
| if (blockTxs.length > 0 && block.content.ordered_transactions.length > 0) { | ||
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | ||
| // confirm array contains all the txs in the block | ||
| const blockTxsHashes = blockTxs.map(tx => tx.hash) | ||
| const blockOrderedTransactionsHashes = | ||
| block.content.ordered_transactions | ||
|
|
||
| if ( | ||
| blockTxsHashes.every(hash => | ||
| blockOrderedTransactionsHashes.includes(hash), | ||
| ) | ||
| ) { | ||
| transactionEntities = blockTxs | ||
| } | ||
|
|
||
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | ||
| else { | ||
| log.error( | ||
| "Block transactions mismatch with block ordered transactions", | ||
| ) | ||
| process.exit(1) | ||
| } | ||
|
|
||
| const status = new Set([ | ||
| TRANSACTION_STATUS.CONFIRMED, | ||
| TRANSACTION_STATUS.FAILED, | ||
| ]) | ||
|
|
||
| // DEBUG: Confirm all transactions' blockNumber == block.number | ||
| // if (transactionEntities.some(tx => tx.blockNumber !== block.number)) { | ||
| // log.error( | ||
| // `[insertBlock] Transaction blockNumber mismatch: ${transactionEntities.map(tx => tx.blockNumber).join(", ")}`, | ||
| // ) | ||
| // process.exit(1) | ||
| // } | ||
| // confirm all txs have a status set | ||
| const txsWithoutStatus = transactionEntities.filter( | ||
| tx => !status.has(tx.status), | ||
| ) | ||
| if (txsWithoutStatus.length > 0) { | ||
| log.error( | ||
| "Transactions without status: " + | ||
| JSON.stringify(txsWithoutStatus, null, 2), | ||
| ) | ||
| process.exit(1) | ||
| } | ||
|
|
||
| transactionEntities = transactionEntities.map(tx => ({ | ||
| ...tx, | ||
| blockNumber: block.number, | ||
| })) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require an exact transaction set before saving non-empty blocks.
Line 248 skips validation when blockTxs is empty, and Lines 255-260 accept any subset of ordered_transactions. Since sync callers insert blocks with [] before fetching/verifying txs, a later tx/attrs/GCR failure can leave the head advanced while tx state is incomplete. Require an exact hash match before saving, or move sync tx fetching before insertBlock.
Proposed guard
- if (blockTxs.length > 0 && block.content.ordered_transactions.length > 0) {
+ if (block.content.ordered_transactions.length > 0) {
// NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE):
// confirm array contains all the txs in the block
const blockTxsHashes = blockTxs.map(tx => tx.hash)
const blockOrderedTransactionsHashes =
block.content.ordered_transactions
+ const blockTxByHash = new Map(blockTxs.map(tx => [tx.hash, tx]))
+ const orderedHashSet = new Set(blockOrderedTransactionsHashes)
if (
- blockTxsHashes.every(hash =>
- blockOrderedTransactionsHashes.includes(hash),
- )
+ blockTxs.length === blockOrderedTransactionsHashes.length &&
+ blockTxByHash.size === blockTxs.length &&
+ orderedHashSet.size === blockOrderedTransactionsHashes.length &&
+ blockOrderedTransactionsHashes.every(hash => blockTxByHash.has(hash))
) {
- transactionEntities = blockTxs
+ transactionEntities = blockOrderedTransactionsHashes.map(hash =>
+ blockTxByHash.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/chainBlocks.ts` around lines 248 - 292, The block
transaction validation in chainBlocks.ts is too permissive because it only runs
when both arrays are non-empty and accepts any subset of ordered_transactions.
Update the transaction check around the blockTxs/ordered_transactions handling
so saving a non-empty block requires an exact hash match before proceeding, or
ensure the sync flow fetches and verifies transactions before insertBlock is
called. Use the existing blockTxs, block.content.ordered_transactions, and
transactionEntities logic to enforce the full set match before setting
blockNumber or continuing.
| const status = new Set([ | ||
| TRANSACTION_STATUS.CONFIRMED, | ||
| TRANSACTION_STATUS.FAILED, | ||
| ]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep failed transactions out of confirmed-only side effects.
Allowing FAILED rows is correct, but downstream side effects still use all transactionEntities: failed L2PS txs can reach persistConfirmedTransactionProjection, and failed identity tx hashes can update IdentityCommitment.blockNumber. Filter these projections to confirmed txs only.
Proposed filter
+ const confirmedTransactionEntities =
+ transactionEntities.filter(
+ tx => tx.status === TRANSACTION_STATUS.CONFIRMED,
+ )
+
- const l2psTxs = transactionEntities.filter(
+ const l2psTxs = confirmedTransactionEntities.filter(
tx => tx.content?.type === "l2ps_hash_update",
)- const committedTxHashes = transactionEntities.map(tx => tx.hash)
+ const committedTxHashes = confirmedTransactionEntities.map(
+ tx => tx.hash,
+ )Also applies to: 401-419
🤖 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/chainBlocks.ts` around lines 271 - 274, The transaction
status set in chainBlocks.ts currently includes FAILED, which is fine for
ingestion but not for confirmed-only side effects. Update the downstream
projection logic in the affected handlers (including
persistConfirmedTransactionProjection and the IdentityCommitment blockNumber
update path) so they only operate on transactions whose status is CONFIRMED,
while keeping FAILED rows available elsewhere. Use the existing
transactionEntities/status handling in chainBlocks.ts to filter before applying
these confirmed-only updates.
| export function normalizePubkey(account: string | Uint8Array): string { | ||
| return typeof account === "string" ? account : forgeToHex(account) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the exported normalizer’s input contract.
The new deterministic-order consumer can pass from_ed25519_address directly; when that field is absent, this falls through to forgeToHex(undefined) and can crash ordering. Either make the helper reject/handle nullish input explicitly, or update callers to pass from_ed25519_address || from.
Safer caller pattern
-const sa = normalizePubkey(txa.content.from_ed25519_address)
-const sb = normalizePubkey(txb.content.from_ed25519_address)
+const sa = normalizePubkey(txa.content.from_ed25519_address || txa.content.from)
+const sb = normalizePubkey(txb.content.from_ed25519_address || txb.content.from)🤖 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/gcr/handleGCR.ts` around lines 170 - 172, The exported
normalizer normalizePubkey currently assumes a non-null string or Uint8Array and
can fall through to forgeToHex on undefined when callers use
from_ed25519_address directly. Update normalizePubkey to guard nullish input
explicitly by rejecting or safely handling it before calling forgeToHex, or
adjust the deterministic-order caller to always pass from_ed25519_address ||
from so the helper never receives undefined.
| if (confirmed.size > 0) { | ||
| log.error("Found confirmed txs during tx application") | ||
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | ||
| // fetch confirmed txs from db | ||
| const confirmedTxs = await Chain.getTransactionsFromHashes(Array.from(confirmed)) | ||
| log.error("Confirmed txs: " + JSON.stringify(Array.from(confirmed), null, 2)) | ||
| log.error("Confirmed txs full: " + JSON.stringify(confirmedTxs.map(t => t.hash), null, 2)) | ||
| process.exit(1) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t terminate the node from this guard.
A duplicate confirmed tx should reject/abort the block-apply path, not call process.exit(1) from inside transaction application. Return a typed failure or throw a domain error so sync/consensus can handle it without killing the process.
Minimal safer direction
const confirmedTxs = await Chain.getTransactionsFromHashes(Array.from(confirmed))
log.error("Confirmed txs: " + JSON.stringify(Array.from(confirmed), null, 2))
log.error("Confirmed txs full: " + JSON.stringify(confirmedTxs.map(t => t.hash), null, 2))
- process.exit(1)
+ throw new Error(
+ "Confirmed transactions encountered during tx application: " +
+ JSON.stringify(Array.from(confirmed)),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (confirmed.size > 0) { | |
| log.error("Found confirmed txs during tx application") | |
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | |
| // fetch confirmed txs from db | |
| const confirmedTxs = await Chain.getTransactionsFromHashes(Array.from(confirmed)) | |
| log.error("Confirmed txs: " + JSON.stringify(Array.from(confirmed), null, 2)) | |
| log.error("Confirmed txs full: " + JSON.stringify(confirmedTxs.map(t => t.hash), null, 2)) | |
| process.exit(1) | |
| if (confirmed.size > 0) { | |
| log.error("Found confirmed txs during tx application") | |
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | |
| // fetch confirmed txs from db | |
| const confirmedTxs = await Chain.getTransactionsFromHashes(Array.from(confirmed)) | |
| log.error("Confirmed txs: " + JSON.stringify(Array.from(confirmed), null, 2)) | |
| log.error("Confirmed txs full: " + JSON.stringify(confirmedTxs.map(t => t.hash), null, 2)) | |
| throw new Error( | |
| "Confirmed transactions encountered during tx application: " + | |
| JSON.stringify(Array.from(confirmed)), | |
| ) |
🤖 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/gcr/handleGCR.ts` around lines 793 - 800, The
confirmed-transaction guard in handleGCR should not terminate the process with
process.exit(1); instead, make this branch fail the block-apply flow by
returning a typed failure or throwing a domain-specific error that
sync/consensus can catch. Update the logic around confirmed.size,
Chain.getTransactionsFromHashes, and the surrounding tx application path so the
duplicate-confirmed-tx condition aborts application cleanly without killing the
node.
| const SYSTEM_RELAY_TX_TYPES = new Set<string>(["l2psBatch"]) | ||
|
|
||
| export default class Mempool { | ||
| public static lock = new Mutex() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the lock readonly.
lock is never reassigned; flagging it readonly matches the SonarCloud finding and prevents an accidental reassignment from silently dropping mutual exclusion for every runExclusive caller.
Proposed fix
- public static lock = new Mutex()
+ public static readonly lock = new Mutex()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static lock = new Mutex() | |
| public static readonly lock = new Mutex() |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 39-39: Make this public static property readonly.
🤖 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` at line 39, The shared Mutex on Mempool.lock
is never reassigned, so update the Mempool class to make lock readonly and keep
the existing static mutex instance intact. Use the Mempool.lock declaration as
the only place to change, ensuring runExclusive callers continue to use the same
mutual exclusion guard and accidental reassignment is prevented.
Source: Linters/SAST tools
| response.extra = { | ||
| message: error instanceof Error ? error.message : String(error), | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve object errors in the RPC payload.
Line 29 falls back to String(error), so non-Error throws degrade to "[object Object]" and you lose the real failure details.
Suggested fix
response.extra = {
- message: error instanceof Error ? error.message : String(error),
+ message:
+ error instanceof Error
+ ? error.message
+ : typeof error === "object" && error !== null
+ ? JSON.stringify(error)
+ : String(error),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response.extra = { | |
| message: error instanceof Error ? error.message : String(error), | |
| } | |
| response.extra = { | |
| message: | |
| error instanceof Error | |
| ? error.message | |
| : typeof error === "object" && error !== null | |
| ? JSON.stringify(error) | |
| : String(error), | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 29-29: 'error' will use Object's default stringification format ('[object Object]') when stringified.
🤖 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/network/handlers/identityHandlers.ts` around lines 28 - 30, The RPC
error payload in identityHandlers should not stringify non-Error thrown values,
because that turns object errors into "[object Object]" and hides details.
Update the error handling around response.extra to preserve the original object
when error is not an Error, and only use error.message for real Error instances.
Keep the fix localized to the identityHandlers response building logic so the
payload retains the full thrown value for downstream consumers.
Source: Linters/SAST tools
| const isSafeError = safeEthosErrors.some(safe => | ||
| errorMsg.includes(safe), | ||
| ) | ||
| response.extra = { | ||
| error: isSafeError ? errorMsg : "Failed to fetch Ethos score", | ||
| error: isSafeError | ||
| ? errorMsg | ||
| : "Failed to fetch Ethos score", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use exact matches for safe Ethos errors.
includes reflects any message containing a safe phrase, including appended internal details. Keep the sanitizer strict by returning only exact allow-listed messages.
Proposed fix
- const isSafeError = safeEthosErrors.some(safe =>
- errorMsg.includes(safe),
- )
+ const isSafeError = safeEthosErrors.includes(errorMsg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isSafeError = safeEthosErrors.some(safe => | |
| errorMsg.includes(safe), | |
| ) | |
| response.extra = { | |
| error: isSafeError ? errorMsg : "Failed to fetch Ethos score", | |
| error: isSafeError | |
| ? errorMsg | |
| : "Failed to fetch Ethos score", | |
| const isSafeError = safeEthosErrors.includes(errorMsg) | |
| response.extra = { | |
| error: isSafeError | |
| ? errorMsg | |
| : "Failed to fetch Ethos score", |
| if (block.number <= getSharedState.lastBlockNumber) { | ||
| response.result = 200 | ||
| response.response = "Block is already processed" | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the dedupe response contract.
This fast path returns a plain string, while BroadcastManager.handleNewBlock returns the structured “already processed” payload with syncData. Peers hitting only this path can miss sync metadata.
Proposed fix
if (block.number <= getSharedState.lastBlockNumber) {
- response.result = 200
- response.response = "Block is already processed"
+ response.response = await BroadcastManager.handleNewBlock(
+ sender,
+ block,
+ )
break
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (block.number <= getSharedState.lastBlockNumber) { | |
| response.result = 200 | |
| response.response = "Block is already processed" | |
| break | |
| if (block.number <= getSharedState.lastBlockNumber) { | |
| response.response = await BroadcastManager.handleNewBlock( | |
| sender, | |
| block, | |
| ) | |
| break |
🤖 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/network/manageGCRRoutines.ts` around lines 265 - 268, The fast-path
dedupe branch in manageGCRRoutines should return the same “already processed”
response shape as BroadcastManager.handleNewBlock, not a plain string. Update
the block.number <= getSharedState.lastBlockNumber path to populate the
structured payload including syncData so peers receive the same contract
regardless of which dedupe path handles the block. Make the change in the logic
around getSharedState.lastBlockNumber and keep the response consistent with the
existing already-processed handling.
| @Column("jsonb", { name: "attrs", nullable: true }) | ||
| attrs: Record<string, any> | null |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Persist attrs on the overwrite path too.
src/libs/blockchain/chainBlocks.ts:195-491 copies block.attrs into newBlock, but the existingBlock && position branch still never updates existingBlock.attrs. Any rewrite of an existing block row will keep stale metadata even though this entity now exposes the column.
Minimal fix
existingBlock.status = block.status
existingBlock.proposer = block.proposer
existingBlock.validation_data = block.validation_data
+ existingBlock.attrs = block.attrs
log.info("about to save block")
return await blocksRepo.save(existingBlock)🤖 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/model/entities/Blocks.ts` around lines 33 - 34, The overwrite path in
chainBlocks should also persist attrs, since the existingBlock && position
branch currently only copies block.attrs into newBlock and leaves the stored row
stale. Update the block rewrite logic in chainBlocks so the existing block
entity’s attrs field is assigned from the incoming block before saving, keeping
it consistent with the Blocks entity’s attrs column.
| const consensusInfo: Record<string, any> = {} | ||
| const secman = SecretaryManager.getInstance() | ||
|
|
||
| if (secman && secman.shard && (secman.shard.members || []).length > 0) { | ||
| const cvsa = await getCommonValidatorSeed() | ||
| consensusInfo.blockRef = secman.shard.blockRef | ||
| consensusInfo.cvsa = cvsa.commonValidatorSeed | ||
| consensusInfo.cvsaBlockRef = cvsa.lastBlockNumber | ||
| consensusInfo.secretary = secman.secretary.identity | ||
| consensusInfo.shard = secman.shard.members.map(m => ({ | ||
| identity: m.identity, | ||
| url: m.connection.string + "/info", | ||
| })) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make consensusInfo best-effort.
getCommonValidatorSeed() can throw when the chain head/genesis is unavailable, and secman.secretary.identity assumes secretary initialization. Don’t let optional consensus metadata fail /info.
Proposed guard
- if (secman && secman.shard && (secman.shard.members || []).length > 0) {
- const cvsa = await getCommonValidatorSeed()
- consensusInfo.blockRef = secman.shard.blockRef
- consensusInfo.cvsa = cvsa.commonValidatorSeed
- consensusInfo.cvsaBlockRef = cvsa.lastBlockNumber
- consensusInfo.secretary = secman.secretary.identity
- consensusInfo.shard = secman.shard.members.map(m => ({
- identity: m.identity,
- url: m.connection.string + "/info",
- }))
+ try {
+ if (secman?.shard?.members?.length) {
+ const cvsa = await getCommonValidatorSeed()
+ consensusInfo.blockRef = secman.shard.blockRef
+ consensusInfo.cvsa = cvsa.commonValidatorSeed
+ consensusInfo.cvsaBlockRef = cvsa.lastBlockNumber
+ consensusInfo.secretary = secman.secretary?.identity ?? null
+ consensusInfo.shard = secman.shard.members.map(m => ({
+ identity: m.identity,
+ url: m.connection.string + "/info",
+ }))
+ }
+ } catch (error) {
+ log.debug(
+ `[SharedState.getInfo] consensusInfo unavailable: ${
+ error instanceof Error ? error.message : String(error)
+ }`,
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const consensusInfo: Record<string, any> = {} | |
| const secman = SecretaryManager.getInstance() | |
| if (secman && secman.shard && (secman.shard.members || []).length > 0) { | |
| const cvsa = await getCommonValidatorSeed() | |
| consensusInfo.blockRef = secman.shard.blockRef | |
| consensusInfo.cvsa = cvsa.commonValidatorSeed | |
| consensusInfo.cvsaBlockRef = cvsa.lastBlockNumber | |
| consensusInfo.secretary = secman.secretary.identity | |
| consensusInfo.shard = secman.shard.members.map(m => ({ | |
| identity: m.identity, | |
| url: m.connection.string + "/info", | |
| })) | |
| } | |
| const consensusInfo: Record<string, any> = {} | |
| const secman = SecretaryManager.getInstance() | |
| try { | |
| if (secman?.shard?.members?.length) { | |
| const cvsa = await getCommonValidatorSeed() | |
| consensusInfo.blockRef = secman.shard.blockRef | |
| consensusInfo.cvsa = cvsa.commonValidatorSeed | |
| consensusInfo.cvsaBlockRef = cvsa.lastBlockNumber | |
| consensusInfo.secretary = secman.secretary?.identity ?? null | |
| consensusInfo.shard = secman.shard.members.map(m => ({ | |
| identity: m.identity, | |
| url: m.connection.string + "/info", | |
| })) | |
| } | |
| } catch (error) { | |
| log.debug( | |
| `[SharedState.getInfo] consensusInfo unavailable: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| ) | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 498-498: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🤖 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/utilities/sharedState.ts` around lines 495 - 508, Make consensusInfo
best-effort in sharedState’s /info path: the getCommonValidatorSeed() call and
secman.secretary.identity access can throw when consensus or secretary state is
unavailable. Update the consensusInfo block to safely read SecretaryManager data
and wrap or guard the common validator seed lookup so the rest of the /info
response still succeeds, leaving optional fields unset when unavailable.
Source: Linters/SAST tools
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/peer/PeerManager.ts (1)
398-415: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate recursive discovery before creating promises.
With
waitForDiscovery=false, callers still start recursive hello RPCs and return before those promises settle. Also, thetruepath waits only one level because child calls omitwaitForDiscovery.Proposed fix
- if (newPeersUnfiltered.length === 0) { + if (newPeersUnfiltered.length === 0 || !waitForDiscovery) { return } // INFO: Recursively say hello to the new peers const peerManager = PeerManager.getInstance() const newPeers = newPeersUnfiltered.filter( ({ publicKey }) => !peerManager.getPeer(publicKey), ) - const promise = newPeers.map(peer => - PeerManager.sayHelloToPeer(new Peer(peer.url, peer.publicKey)), - ) - - if (waitForDiscovery) { - await Promise.all(promise) - } + await Promise.all( + newPeers.map(peer => + PeerManager.sayHelloToPeer( + new Peer(peer.url, peer.publicKey), + true, + ), + ), + )🤖 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/peer/PeerManager.ts` around lines 398 - 415, Gate the recursive discovery flow in PeerManager’s peer-hello logic so recursion only happens when waitForDiscovery is true; currently the Promise list is created in all cases, which still fires nested hello RPCs even when callers opted out. Update the code around the newPeers filtering and PeerManager.sayHelloToPeer calls to build/await those recursive promises only inside the waitForDiscovery branch, and ensure the recursive invocation propagates waitForDiscovery so the true path waits through child discoveries instead of stopping after one level.
🤖 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/peer/PeerManager.ts`:
- Around line 398-415: Gate the recursive discovery flow in PeerManager’s
peer-hello logic so recursion only happens when waitForDiscovery is true;
currently the Promise list is created in all cases, which still fires nested
hello RPCs even when callers opted out. Update the code around the newPeers
filtering and PeerManager.sayHelloToPeer calls to build/await those recursive
promises only inside the waitForDiscovery branch, and ensure the recursive
invocation propagates waitForDiscovery so the true path waits through child
discoveries instead of stopping after one level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8f8a5c6-3359-453c-a357-1023e4316d33
📒 Files selected for processing (3)
src/libs/consensus/v2/routines/getShard.tssrc/libs/network/manageHelloPeer.tssrc/libs/peer/PeerManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libs/consensus/v2/routines/getShard.ts
| for (const hash of txHashes) { | ||
| const tx = consensusTxMap.get(hash) | ||
|
|
||
| if (tx) { | ||
| mempoolTxs.push(tx) | ||
|
|
||
| // NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE): | ||
| if (!status.has(tx.status)) { | ||
| log.error( | ||
| "Transaction status mismatch: " + | ||
| hash + | ||
| " " + | ||
| tx.status, | ||
| ) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
process.exit(1) fires on any RPC call during the consensus voting window
The getBlockTransactions function checks !status.has(tx.status) and calls process.exit(1) if a transaction's status is not CONFIRMED or FAILED. However, txMap is populated (in consensusRoutine) before forgeBlock sets candidateBlock, and tx statuses are only assigned later — after the block receives enough votes and applyGCREditsFromMergedMempool runs. During the entire voting window between forgeBlock and GCR application, any valid client that calls getBlockTransactions(candidateBlock.hash) will find entries in txMap with null status and immediately crash the node via process.exit(1). This is labeled "NODE_CRITICAL_DEBUG (DO NOT REMOVE COMMENTED OUT CODE)" but the check is live, not commented out.
| const sa = normalizePubkey(txa.content.from_ed25519_address) | ||
| const sb = normalizePubkey(txb.content.from_ed25519_address) |
There was a problem hiding this comment.
normalizePubkey receives undefined whenever from_ed25519_address is not set. The type guard inside normalizePubkey treats anything that is not a string as a Uint8Array and calls forgeToHex on it, which throws on null/undefined. The old implementation used tx.content?.from with a full defensive coercion that handled every possible buffer shape. handleGCR.ts still uses the fallback from_ed25519_address || from, so the pattern for safe access is already established — apply the same guard here to avoid a crash in consensus or sync for any transaction where the ed25519 address field is absent.
| const sa = normalizePubkey(txa.content.from_ed25519_address) | |
| const sb = normalizePubkey(txb.content.from_ed25519_address) | |
| const sa = normalizePubkey(txa.content.from_ed25519_address || txa.content.from) | |
| const sb = normalizePubkey(txb.content.from_ed25519_address || txb.content.from) |
+ whitelist consensus abort
| const threshold = Math.floor((getSharedState.shardSize * 2) / 3) + 1 | ||
| if (verifiedSigners.size < threshold) { | ||
| return { | ||
| valid: false, | ||
| reason: `insufficient verified signatures: ${verifiedSigners.size}/${getSharedState.shardSize} (need ${threshold})`, | ||
| } | ||
| } |
There was a problem hiding this comment.
shardSize instead of actual validator count
The threshold is computed against getSharedState.shardSize (a static runtime config value) rather than validatorIdentities.size (the height-stable resolved eligible-validator count). This was explicitly disabled in the previous revision with the comment "BECAUSE WE CAN'T VERIFY THE VALIDATORS WERE ONLINE TO FORGE THIS BLOCK — THAT CAUSES REJECTION FOR ALL BLOCKS FORGED IN THE EARLY PHASES OF THE NETWORK". Re-enabling the check without adjusting for historical blocks means any node syncing from genesis that encounters a block forged during an era when fewer validators than Math.floor(shardSize * 2 / 3) + 1 were registered will hit { valid: false } at line 117-122, which upstream in syncBlock returns false (sync failure), permanently stalling that node at that height. The correct base for the threshold is validatorIdentities.size — the same set already resolved height-stably above — which is what the old commented-out threshold code used.
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
| function shuffleArray<T>(array: T[]): T[] { | ||
| const shuffled = [...array] | ||
| for (let i = shuffled.length - 1; i > 0; i--) { | ||
| const j = Math.floor(Math.random() * (i + 1)) | ||
| ;[shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]] | ||
| } | ||
| return shuffled | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the test shuffle from the production sync path.
Line 388 explicitly says this must be removed before merging. Keeping Math.random() in block verification can make valid blocks fail nondeterministically if deterministic ordering has any tie case.
🧹 Proposed cleanup
-function shuffleArray<T>(array: T[]): T[] {
- const shuffled = [...array]
- for (let i = shuffled.length - 1; i > 0; i--) {
- const j = Math.floor(Math.random() * (i + 1))
- ;[shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]]
- }
- return shuffled
-}- // for testing, shuffle list
- // TODO: Remove this before merging
- txs = shuffleArray(txs)
const sorted = orderDeterministically(txs)Also applies to: 387-389
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 51-51: Make sure that using this pseudorandom number generator is safe here.
🤖 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 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.
Source: Linters/SAST tools
| // 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.
2 participants
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