Fix state drift, expired txs cleanup, etc. - #976
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| src/libs/blockchain/routines/Sync.ts | Adds fork recovery and drift latches, but restart detection is not atomic with the GCR writes it is meant to detect. |
| src/libs/blockchain/gcr/handleGCR.ts | Applies broader nonce and gas edits while retaining separate persistence steps for GCR entities and assignment markers. |
| src/libs/consensus/v2/routines/mergeMempools.ts | Defers peerlist contributions until transaction admission and now fails closed when admission cannot be determined. |
| src/libs/consensus/v2/routines/peerlistMerge.ts | Merges deterministic validator peerlists but still defaults to an unfiltered pool when no validators are found. |
| src/libs/consensus/v2/routines/networkAheadVeto.ts | Adds validator-filtered network-ahead vetoes while retaining an unfiltered default fallback for an empty validator set. |
| src/libs/network/endpointHandlers.ts | Avoids contributions after explicit receive failures, but still treats a success response as proof that envelope transactions were admitted. |
Sequence Diagram
sequenceDiagram
participant Sync
participant GCR
participant Marker as GCRAssignedTx
participant Chain
Sync->>GCR: Apply synced transaction edits
GCR-->>Sync: Persist nonce/balance/validator state
GCR->>Marker: Persist assignment markers
Sync->>Chain: Insert block and transactions
alt Process stops before marker write
Note over GCR,Marker: State exists without durable drift marker
else Block insertion fails
Sync->>Sync: Set in-process drift latch
end
Note over Sync,Chain: Restart clears the in-process latch
Sync->>Marker: Query MAX(block_number)
Marker-->>Sync: No orphan marker
Sync->>GCR: Retry and reapply edits
Prompt To Fix All With AI
### Issue 1
src/libs/blockchain/routines/Sync.ts:934-940
**Drift marker misses persisted state**
When the process terminates after `saveGCREditChanges` persists a synced block's GCR edits but before `bulkUpdateAssignedTxs` writes the corresponding markers, the restart guard sees no assignment above the chain tip. The node then reapplies the same block's edits, compounding nonce, balance, or validator-state drift.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (14): Last reviewed commit: "put back testnet validators" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- mergeMempools: do not record peerlist contributions from non-200 responses. peerlist feeds hash-sensitive block.content.peerlist, so recording it from failed exchanges let validators that saw different failure bodies derive divergent candidate blocks. - peerlistMerge/getShard: gate the empty-validator unfiltered fallbacks behind DEMOS_REQUIRE_VALIDATORS, matching the existing guard in getEligiblePool. Contributions are only shape-validated, so an unfiltered commit let any online identity reach shard selection without an active stake. - Sync.applySyncedBlock: GCR mutations are applied before insertBlock and neither takes a shared transaction manager, so an insert failure left state ahead of the chain, invisible to retry via the block-exists guard. Surface it as fatal instead of continuing on drifted state. Adds a test covering the DEMOS_REQUIRE_VALIDATORS refusal path.
- mempool.receive: staleCutoff was computed but never read, so the expired-tx cleanup this PR advertises had no effect and the extra getLastBlockNumber() call was pure overhead. Actually filter incoming txs on the reference-block window, matching the bound isReferenceBlockAllowed applies on the endpoint admission path. Inlined rather than imported: endpointExecution already imports mempool, so importing back would close a cycle. - computeMergedPeerlist: filter against validators at blockRef - 1 (the round's own parent height) instead of the live getSharedState.lastBlockNumber. The tip can advance mid-round via sync, letting two nodes in the same round filter against different heights and commit divergent block.content.peerlist values. Qodo's third finding (unstaked signers verifying via the unfiltered eligible pool) shares a root cause with the Greptile P1 already fixed by the DEMOS_REQUIRE_VALIDATORS guard in getEligiblePool.
Receiver-side mirror of the mergeMempools guard. handleMempool recorded the sender's peerlist contribution even when Mempool.receive threw or returned unsuccessfully, in which case the handler replies 400 and the caller discards its reciprocal contribution. The two ends then hold different peerlists and derive different hash-sensitive block.content.peerlist values for the same round.
![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 (2)
src/libs/blockchain/chainBlocks.ts (1)
278-307: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse an unconditionally fatal error for post-GCR
BlockInvalidError.
batchDownloadBlockswrapsinsertBlockwithtry/catchand rethrows, butsyncBlockdoes not, so aBlockInvalidErrorfrom transaction/tx-status integrity checks aftersyncGCRTables(...)can still leave the batch-synced node with mutated GCR state and a failed block insertion in the normal sync path. Make this error fatal from that caller path too, or keep this check unconditionally exiting.🤖 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 278 - 307, Make the post-syncGCRTables transaction integrity checks in insertBlock unconditionally fatal by removing the debugAssertionsEnabled guard around process.exit(1), including both transaction mismatch and missing-status paths. Ensure BlockInvalidError cannot propagate into the normal syncBlock path after GCR state mutation.src/libs/consensus/v2/routines/createBlock.ts (1)
6-7: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the
Block.peerlistsource type so the assignment no longer needsunknown.
createBlock.tsassignspeerlist as unknown as typeof block.content.peerlist, butsrc/libs/blockchain/block.tsstill initializespeerlist: []without thestring[]contract. Addpeerlist: [] as string[](or update the Block model/exported type) and remove the force-cast at lines 18 and 33-34.🤖 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/consensus/v2/routines/createBlock.ts` around lines 6 - 7, Update the Block model’s peerlist initialization or exported type to declare peerlist as string[], then revise createBlock’s assignments to use typeof block.content.peerlist directly without the intermediate unknown cast. Preserve the existing peerlist values and behavior.
🧹 Nitpick comments (9)
src/libs/blockchain/routines/Sync.ts (2)
731-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
resolveForkedBlockto clear the Sonar complexity gate.SonarCloud fails the build here (cognitive complexity 22 vs 15 allowed). The function does three separable things: candidate selection, polling/dedup into variants, and verify-then-fetch-txs. Extracting the poll-and-group step and the variant-selection loop into helpers resolves it without behavior change.
Minor related note:
.slice(0, FORK_RECOVERY_MAX_PEERS)truncates an unsortedpeerManager.getAll(), so recovery may keep polling the same five (possibly stale) peers. Sorting bysync.blockdescending before slicing would make the choice deterministic and pick the most advanced peers.🤖 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 731 - 745, The resolveForkedBlock function exceeds the Sonar cognitive-complexity threshold; split its candidate polling/deduplication and variant-selection logic into focused helpers while preserving behavior. Keep candidate selection in resolveForkedBlock, and sort peers by sync.block descending before applying FORK_RECOVERY_MAX_PEERS so the most advanced eligible peers are selected deterministically.Source: Linters/SAST tools
1218-1232: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGate the optional
blockbefore asserting the nonce trace.At runtime, a non-null
nonceTracerequiresblockto be present, but the actual control flow only checksnonceTrace. Making theblockpresence explicit avoids relying on an inference that the function contract does not make explicit.🛡️ Proposed fix
- const nonceTrace = debugAssertionsEnabled() - ? readNonceTrace(block?.attrs) - : null - + const nonceTrace = block && debugAssertionsEnabled() ? readNonceTrace(block.attrs) : null + if (!nonceTrace) { - await HandleGCR.applyTransactions(sortedTxs, false) - return } + if (!block) { + await HandleGCR.applyTransactions(sortedTxs, false) + return + } const traceAccounts = Object.keys(nonceTrace) const localBefore = await readNonces(traceAccounts) await HandleGCR.applyTransactions(sortedTxs, false) const localAfter = await readNonces(traceAccounts) assertSyncedNonceTrace(block.number, nonceTrace, localBefore, localAfter)🤖 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 1218 - 1232, Update the nonce-trace guard in the Sync flow to require both debug tracing data and a present block before reading trace accounts or calling assertSyncedNonceTrace. Preserve the existing transaction application and early return behavior when either nonceTrace or block is absent.src/libs/debug/nonceTrace.ts (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
debugAssertionsEnabled()lives in an unrelated module. It's a genericConfig.getInstance().debug.assertionsEnabledgetter, but it's defined innonceTrace.ts(a module scoped to nonce-trace invariants) and imported from there by four files for entirely unrelated checks (block hash mismatches, tx status, GCR confirmed-tx uniqueness). Moving it to a shared debug/config utility would make the dependency direction clearer.
src/libs/debug/nonceTrace.ts#L20-L22: movedebugAssertionsEnabledout to a general-purpose debug/config module (e.g. alongside otherConfig-derived getters) and re-export or update call sites.src/libs/blockchain/chainBlocks.ts#L29-L30: update the import path once the function is relocated.src/libs/blockchain/chainTransactions.ts#L14-L14: update the import path once the function is relocated.src/libs/blockchain/gcr/handleGCR.ts#L44-L44: update the import path once the function is relocated.src/libs/blockchain/validation/verifyBlock.ts#L20-L22: update the import path once the function is relocated.🤖 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/debug/nonceTrace.ts` around lines 20 - 22, Move the generic debugAssertionsEnabled getter out of nonceTrace.ts into a shared debug/config utility alongside other Config-derived getters, preserving its behavior. Update imports in src/libs/blockchain/chainBlocks.ts (lines 29-30), src/libs/blockchain/chainTransactions.ts (line 14), src/libs/blockchain/gcr/handleGCR.ts (line 44), and src/libs/blockchain/validation/verifyBlock.ts (lines 20-22) to use the new module; remove or re-export the nonceTrace definition as appropriate.src/libs/blockchain/gcr/handleGCR.ts (1)
752-773: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduced-tx clone works but is easy to misread.
Object.assign(Object.create(Object.getPrototypeOf(tx)), tx, {...})preservestx's prototype (unlike a plain{...tx}spread used elsewhere, e.g.chainBlocks.ts'stransactionEntities.map(tx => ({...tx, blockNumber}))), which is presumably needed if anything downstream relies oninstanceof Transaction. A small named helper (e.g.cloneTransactionWithEdits(tx, edits)) would make that intent explicit instead of leaving readers to infer why this call site diverges from the simpler spread pattern used nearby.🤖 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 752 - 773, Extract the reduced transaction construction in the universalEdits branch into a clearly named helper such as cloneTransactionWithEdits, preserving the Transaction prototype and replacing only content.gcr_edits with the supplied edits. Use the helper from the existing finalTxs path so the prototype-preserving intent is explicit without changing behavior.src/libs/blockchain/gcr/gcr.ts (1)
225-228: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSibling functions have the same falsy-
blockNumberbug this line just fixed.Good fix here —
blockNumber === null || blockNumber === undefinedcorrectly treats an explicit0(genesis) as a real height instead of "not provided." However,getGCRHashedStakes(if (!n), line 182) andgetGCRValidatorStatus(if (!blockNumber), line 258) still use the old falsy check and would silently default to the last block when explicitly queried at block0.🐛 Suggested fix for the two sibling functions
- if (!n) { + if (n === null || n === undefined) { n = await Chain.getLastBlockNumber() }- if (!blockNumber) { + if (blockNumber === null || blockNumber === undefined) { blockNumber = await Chain.getLastBlockNumber() }🤖 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 225 - 228, Update the block-number presence checks in getGCRHashedStakes and getGCRValidatorStatus to test only for null or undefined, matching the corrected check in the surrounding GCR logic. Preserve an explicit block number of 0 and continue fetching the latest block only when no block number was provided.src/libs/blockchain/genesis/verifySnapshot.ts (1)
174-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHigh cognitive complexity on a now-public function.
SonarCloud flags
readFileSinglePass's cognitive complexity at 69 (allowed: 15). The function's logic itself wasn't changed here — only its export visibility — but now that it's part of the public surface used byscripts/state-snapshot/rehash.ts, splitting the chunk/leftover accumulation and the per-line/trailing-row JSON parsing into smaller named helpers would make it easier to reason about and modify safely going forward.Also applies to: 201-305
🤖 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/genesis/verifySnapshot.ts` at line 174, Reduce the cognitive complexity of the public readFileSinglePass function by extracting named helpers for chunk/leftover accumulation and for per-line plus trailing-row JSON parsing. Keep readFileSinglePass focused on orchestration, preserve the existing parsing and accumulation behavior, and ensure the extracted helpers retain the current error and result handling.Source: Linters/SAST tools
src/libs/consensus/v2/routines/getShard.ts (1)
139-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer
??=per Sonar hint.- if (lastBlockNumber === undefined || lastBlockNumber === null) { - lastBlockNumber = getSharedState.lastBlockNumber - } + lastBlockNumber ??= getSharedState.lastBlockNumber🤖 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/consensus/v2/routines/getShard.ts` around lines 139 - 141, Update the lastBlockNumber fallback in the getShard routine to use the nullish-assignment operator (??=), preserving the existing behavior of assigning getSharedState.lastBlockNumber only when lastBlockNumber is null or undefined.Source: Linters/SAST tools
src/libs/consensus/v2/types/secretaryManager.ts (1)
879-916: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFragile cross-file string-sentinel coupling for secretary-unreachable detection.
secretaryUnreachableis derived fromres.response === "Max retries reached", which is a hardcoded literal defined inPeer.longCall(src/libs/peer/Peer.ts,response: "Max retries reached"). Both a legitimate 400 from the secretary ("block reference too low") and a transport-level retry exhaustion currently shareresult === 400, disambiguated only by matching that exact string. If that literal ever changes inPeer.ts, this check silently breaks and a transport failure would be misclassified as a hard rejection (aborting the round) instead of triggering secretary-failover.Consider a dedicated signal (e.g. a distinct result code, or a boolean field on the response) instead of string matching across files.
🤖 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/consensus/v2/types/secretaryManager.ts` around lines 879 - 916, Replace the cross-file `"Max retries reached"` string check in the `handleSendStatusRes` secretary response flow with a dedicated transport-failure signal produced by `Peer.longCall`, such as a distinct result code or explicit boolean field. Update both the response producer and the `secretaryUnreachable` classification so retry exhaustion triggers secretary failover while a legitimate 400 block-reference rejection still aborts consensus.src/libs/consensus/v2/routines/peerlistMerge.ts (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not apply the SonarCloud
localeComparesuggestion here — it would break consensus determinism.The default
Array.prototype.sort()on strings compares by UTF-16 code unit, which is locale-independent and identical across all nodes.localeComparedepends on the runtime's ICU/locale configuration and can order strings differently across environments. Since this array feedsblock.content.peerlist(hash-sensitive, voted on by the shard), switching tolocaleComparerisks nodes computing different block hashes from the same logical peerlist. Consider adding a short comment at each site so a future contributor doesn't "fix" the lint warning and introduce a determinism bug.Also applies to: 125-125, 134-134
🤖 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/consensus/v2/routines/peerlistMerge.ts` at line 39, Keep the default UTF-16 code-unit sort in the peerlist merge logic at each affected site, including the returns around the existing view-sorting code, and do not replace it with localeCompare. Add a brief comment at each site documenting that this locale-independent ordering is required for deterministic consensus and hash-stable peerlist output.Source: Linters/SAST tools
🤖 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 `@scripts/run`:
- Around line 10-11: Update the TLSNOTARY_DISABLED and MONITORING_DISABLED
defaults in the run script so both services remain enabled by default: set them
to false or preserve caller-provided environment values while only disabling
them when explicitly requested.
In `@scripts/state-snapshot/rehash.ts`:
- Line 58: Reduce the cognitive complexity of main by extracting manifest
loading, per-file rehashing, and final write/check handling into focused helper
functions. Keep main responsible only for orchestrating these helpers,
preserving the existing behavior and error handling while bringing its
complexity below the configured threshold.
- Line 152: Update the manifest write in the rehash flow to use an atomic
replacement: write the serialized manifest to a temporary file in the same
directory as manifestPath, then rename that temporary file to manifestPath only
after the write succeeds. Ensure failures do not truncate or replace the
existing manifest, and clean up the temporary file when appropriate.
In `@src/config/loader.ts`:
- Around line 107-123: Move block timestamp policy and watchdog timeout
configuration out of the node-local environment loading in the core
configuration path and source them from genesis/network parameters so
verifyBlock uses chain-defined values. Add startup validation enforcing
blockTimestampMinDelta >= 1, blockTimestampTolerance >= 0, and
blockWatchdogTimeoutSeconds > 0, rejecting invalid configuration before the node
starts.
In `@src/index.ts`:
- Line 1288: Refactor gracefulShutdown into focused helper functions for each
subsystem shutdown stage, reducing its cognitive complexity below the configured
threshold. Preserve the existing shutdown order and behavior, including timeout
handling, signal processing, and exit-code propagation.
In `@src/libs/blockchain/mempool.ts`:
- Around line 412-413: Update receive() to use SecretaryManager.lastBlockRef as
the basis for staleCutoff, then apply that cutoff when filtering incoming
transactions and unseenTransactions by excluding entries with reference_block
below staleCutoff; otherwise remove the unused lastBlock/staleCutoff calculation
and database call.
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 855-876: Make the block insertion failure in the helper containing
the Chain.insertBlock call truly fatal under the repository’s established fatal
policy, such as marking the node unsynced or terminating the process, rather
than only rethrowing the database error to recoverable callers. Preserve the
detailed error log and update its prefix from [batchDownloadBlocks] to
[applySyncedBlock] so it accurately identifies both sync and fork-recovery
paths.
- Around line 980-985: The `resolveForkedBlock` failure path in `requestBlocks`
must distinguish a self-fork from an invalid peer variant: when it returns null,
surface a terminal unsynced condition and trigger the operator alert instead of
throwing `ForkedPeerError` and returning false. Ensure `fastSyncRoutine` does
not retry this condition indefinitely, including during the first sync before
`FAST_SYNC_TIMEOUT_MS` applies.
- Around line 1442-1449: Update the fast-sync timeout handling in the routine
containing the timeout branch and its finally cleanup so the detached
fastSyncRoutine can observe a per-run abort signal until it settles, rather than
having finally immediately clear the shared fastSyncAborted state. Preserve
retry-loop cancellation behavior while preventing stale abort state from
affecting later runs, and build the timeout warning from FAST_SYNC_TIMEOUT_MS
instead of hardcoding “30s”.
- Around line 765-778: The candidate polling inside resolveForkedBlock must
enforce a per-peer timeout so one unresponsive candidate cannot stall
Promise.allSettled. Update candidate.call(request, false) to use the existing
CallOptions timeout mechanism or an equivalent Promise.race fallback, treating
timeout expiry as a failed candidate while preserving successful block
validation and settled-result handling.
- Around line 578-585: Update syncBlock to delegate the post-transaction-fetch
processing to applySyncedBlock, including merge, verification, GCR
synchronization, insertion, and fatal-drift handling. Remove the duplicated
mergePeerlist, exists check, and related processing from syncBlock, while
preserving the transaction-fetch flow before invoking the shared helper.
In `@src/libs/consensus/v2/routines/getShard.ts`:
- Line 114: Update both sort calls in the getShard routine—on [...localView] and
[...new Set(pool)]—to pass an explicit string comparator using localeCompare,
preserving the existing ascending ordering and returned values.
- Around line 45-125: Reduce the cognitive complexity of getEligiblePool below
the CI limit by extracting its committed-peerlist filtering, validator
bootstrap, and local-view fallback branches into small named helpers. Keep the
existing validator requirement checks, warnings, sorting, deduplication,
caching, and returned pool behavior unchanged.
In `@src/libs/consensus/v2/routines/networkAheadVeto.ts`:
- Around line 11-24: Update the validator-loading flow around
getValidatorAddresses and its isNetworkAhead/getAheadValidatorPeers callers to
detect an empty validator set before unfiltered peer fallback occurs. Mirror
peerlistMerge.ts’s SECURITY warning and opt-in DEMOS_REQUIRE_VALIDATORS refusal,
preserving normal behavior when validators are present.
In `@src/libs/consensus/v2/routines/peerlistMerge.ts`:
- Around line 52-65: The contributePeerlist validation in
src/libs/consensus/v2/routines/peerlistMerge.ts lines 52-65 must reject blockRef
values outside a bounded window around the node’s expected round, such as
getSharedState.lastBlockNumber + 1 ± N, before resetting contributions or
contributionsBlockRef. The mempool RPC call in
src/libs/network/endpointHandlers.ts lines 146-159 requires no direct change
once this upstream validation is enforced; verify whether existing sender or
shard-membership authorization provides additional protection.
- Around line 67-83: Cap the attacker-supplied peerlist before the
filter/map/regex chain in the peerlist merge flow, using
MERGE_PEERLIST_MAX_ENTRIES_PER_PEER as the processing bound. Preserve the
existing validation, normalization, warning behavior, and final entries result
while ensuring no more than the allowed number of input entries are
synchronously processed.
In `@src/libs/utils/calibrateTime.ts`:
- Around line 29-61: Update the recalibrationTimer callback to prevent
concurrent getMeasuredTimeDelta requests when a prior measurement is still
running. Use an in-flight guard or completion-based scheduling, ensuring skipped
runs do not overwrite corrections and the existing rejection, update, and
error-handling behavior remains unchanged.
In `@src/utilities/sharedState.ts`:
- Line 299: Initialize lastBlockInsertedAt immediately after the bootstrap
Chain.getLastBlock() call, using the current timestamp even when no higher block
is inserted. Preserve the restored block number and hash behavior while ensuring
the watchdog is armed for nodes starting on a stalled chain.
---
Outside diff comments:
In `@src/libs/blockchain/chainBlocks.ts`:
- Around line 278-307: Make the post-syncGCRTables transaction integrity checks
in insertBlock unconditionally fatal by removing the debugAssertionsEnabled
guard around process.exit(1), including both transaction mismatch and
missing-status paths. Ensure BlockInvalidError cannot propagate into the normal
syncBlock path after GCR state mutation.
In `@src/libs/consensus/v2/routines/createBlock.ts`:
- Around line 6-7: Update the Block model’s peerlist initialization or exported
type to declare peerlist as string[], then revise createBlock’s assignments to
use typeof block.content.peerlist directly without the intermediate unknown
cast. Preserve the existing peerlist values and behavior.
---
Nitpick comments:
In `@src/libs/blockchain/gcr/gcr.ts`:
- Around line 225-228: Update the block-number presence checks in
getGCRHashedStakes and getGCRValidatorStatus to test only for null or undefined,
matching the corrected check in the surrounding GCR logic. Preserve an explicit
block number of 0 and continue fetching the latest block only when no block
number was provided.
In `@src/libs/blockchain/gcr/handleGCR.ts`:
- Around line 752-773: Extract the reduced transaction construction in the
universalEdits branch into a clearly named helper such as
cloneTransactionWithEdits, preserving the Transaction prototype and replacing
only content.gcr_edits with the supplied edits. Use the helper from the existing
finalTxs path so the prototype-preserving intent is explicit without changing
behavior.
In `@src/libs/blockchain/genesis/verifySnapshot.ts`:
- Line 174: Reduce the cognitive complexity of the public readFileSinglePass
function by extracting named helpers for chunk/leftover accumulation and for
per-line plus trailing-row JSON parsing. Keep readFileSinglePass focused on
orchestration, preserve the existing parsing and accumulation behavior, and
ensure the extracted helpers retain the current error and result handling.
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 731-745: The resolveForkedBlock function exceeds the Sonar
cognitive-complexity threshold; split its candidate polling/deduplication and
variant-selection logic into focused helpers while preserving behavior. Keep
candidate selection in resolveForkedBlock, and sort peers by sync.block
descending before applying FORK_RECOVERY_MAX_PEERS so the most advanced eligible
peers are selected deterministically.
- Around line 1218-1232: Update the nonce-trace guard in the Sync flow to
require both debug tracing data and a present block before reading trace
accounts or calling assertSyncedNonceTrace. Preserve the existing transaction
application and early return behavior when either nonceTrace or block is absent.
In `@src/libs/consensus/v2/routines/getShard.ts`:
- Around line 139-141: Update the lastBlockNumber fallback in the getShard
routine to use the nullish-assignment operator (??=), preserving the existing
behavior of assigning getSharedState.lastBlockNumber only when lastBlockNumber
is null or undefined.
In `@src/libs/consensus/v2/routines/peerlistMerge.ts`:
- Line 39: Keep the default UTF-16 code-unit sort in the peerlist merge logic at
each affected site, including the returns around the existing view-sorting code,
and do not replace it with localeCompare. Add a brief comment at each site
documenting that this locale-independent ordering is required for deterministic
consensus and hash-stable peerlist output.
In `@src/libs/consensus/v2/types/secretaryManager.ts`:
- Around line 879-916: Replace the cross-file `"Max retries reached"` string
check in the `handleSendStatusRes` secretary response flow with a dedicated
transport-failure signal produced by `Peer.longCall`, such as a distinct result
code or explicit boolean field. Update both the response producer and the
`secretaryUnreachable` classification so retry exhaustion triggers secretary
failover while a legitimate 400 block-reference rejection still aborts
consensus.
In `@src/libs/debug/nonceTrace.ts`:
- Around line 20-22: Move the generic debugAssertionsEnabled getter out of
nonceTrace.ts into a shared debug/config utility alongside other Config-derived
getters, preserving its behavior. Update imports in
src/libs/blockchain/chainBlocks.ts (lines 29-30),
src/libs/blockchain/chainTransactions.ts (line 14),
src/libs/blockchain/gcr/handleGCR.ts (line 44), and
src/libs/blockchain/validation/verifyBlock.ts (lines 20-22) to use the new
module; remove or re-export the nonceTrace definition as appropriate.
🪄 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 Plus
Run ID: b621b177-63ac-462b-b91b-a5fbf1ec9178
📒 Files selected for processing (43)
.env.exampledata/snapshot/gcr_main.jsonldata/snapshot/gcr_storageprogram.jsonldata/snapshot/manifest.jsondata/snapshot/validators.jsonlpackage.jsonscripts/runscripts/state-snapshot/rehash.tssrc/config/defaults.tssrc/config/envKeys.tssrc/config/index.tssrc/config/loader.tssrc/config/types.tssrc/index.tssrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/chainTransactions.tssrc/libs/blockchain/gcr/gcr.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/blockchain/genesis/verifySnapshot.tssrc/libs/blockchain/mempool.tssrc/libs/blockchain/routines/Sync.tssrc/libs/blockchain/validation/verifyBlock.test.tssrc/libs/blockchain/validation/verifyBlock.tssrc/libs/consensus/v2/PoRBFT.tssrc/libs/consensus/v2/routines/createBlock.tssrc/libs/consensus/v2/routines/getShard.test.tssrc/libs/consensus/v2/routines/getShard.tssrc/libs/consensus/v2/routines/manageProposeBlockHash.tssrc/libs/consensus/v2/routines/mergeMempools.tssrc/libs/consensus/v2/routines/networkAheadVeto.tssrc/libs/consensus/v2/routines/peerlistMerge.test.tssrc/libs/consensus/v2/routines/peerlistMerge.tssrc/libs/consensus/v2/types/secretaryManager.tssrc/libs/debug/nonceTrace.tssrc/libs/network/endpointHandlers.tssrc/libs/network/manageConsensusRoutines.tssrc/libs/network/rpcDispatch.tssrc/libs/omniprotocol/protocol/handlers/control.tssrc/libs/peer/Peer.tssrc/libs/peer/PeerManager.tssrc/libs/utils/calibrateTime.tssrc/utilities/mainLoop.tssrc/utilities/sharedState.ts
A 200 response carrying a missing or non-array txs field is still a failed exchange. Recording its peerlist let peers that parsed the malformed payload differently derive divergent block.content.peerlist values. Move contributePeerlist below the tx-array validation so only fully usable responses influence block content.
- Sync: the previous 'fatal' insert-failure handler only rethrew, and callers up the fastSync chain swallow errors, so the node kept running on drifted state — the exact outcome the comment warned about. Extract insertBlockOrHalt, which marks the node unsynced before rethrowing so it stops participating until an operator resyncs. - Sync: route syncBlock through the same guard. It duplicated the un-guarded syncGCRTables-then-insertBlock sequence that applySyncedBlock was added to fix. - peerlistMerge: apply the per-contributor cap BEFORE validating entries. Lowercasing and regex-testing every element of an attacker-supplied array first let a hostile contributor push unbounded work onto the consensus tick regardless of the cap. - peerlistMerge/getShard: give .sort() an explicit comparator (compareIdentities). Behaviourally identical for strings, but the bare form is a SonarCloud CI gate failure.
Addresses the two remaining Greptile blockers: - Sync: an insertBlock failure leaves GCR edits persisted for a block that was never inserted, so the block-exists guard does not stop a retry from applying the SAME edits again, double-advancing nonces and balances. Latch the drift and refuse all subsequent block application via assertNoUnreconciledGcrState, on both the batch and syncBlock paths. Marking the node unsynced alone did not prevent this, since the sync loop is what retries. - mergeMempools: peerlist contributions were recorded before the batched Mempool.receive that admits the transactions they arrived with. A throw there left us holding peerlists whose txs we never took, while peers dropped theirs — divergent block.content.peerlist again. Stage contributions and commit them only once admission succeeds.
|
Greptile confidence went 3/5 → 2/5 → 3/5 across this loop; all review threads from Greptile, Qodo and CodeRabbit are resolved. The remaining 3/5 rests on two known, deliberate items:
Fixed in this loop: peerlist no longer recorded from non-200 / malformed / locally-unadmitted mempool exchanges (both caller and receiver side), peerlist filtered at the round's own parent height instead of the moving tip, the dead |
- scripts/run: restore TLSNOTARY_DISABLED/MONITORING_DISABLED to false. This PR flipped both to true, silently disabling TLSNotary and monitoring by default and contradicting the default-enabled lifecycle. - index.ts: arm the block watchdog at boot. lastBlockInsertedAt was only set when a HIGHER block was inserted, so a node starting on an already-stalled chain never armed the watchdog at all. - calibrateTime: guard against overlapping NTP recalibrations. A round walks 4 servers with no per-request timeout and can outlast the 10min interval, letting an older reading clobber a newer correction. - Sync: bound fork-recovery peer polling with a per-peer timeout. One unresponsive candidate could hold Promise.allSettled — and the sync round — open indefinitely. - Sync: fix the fastSync abort flag being a no-op. The finally block reset fastSyncAborted before the detached routine could observe it; ownership now passes to a queued reset behind syncLock. Also interpolate FAST_SYNC_TIMEOUT_MS instead of hardcoding "30s". - networkAheadVeto: mirror the peerlistMerge/getShard guard. With no validator set, any peer claiming a higher block could veto forging through an unfiltered, silent fallback. - config/loader: reject out-of-range block timing at startup. These feed verifyBlock acceptance, so bad values fork a node silently rather than failing loudly. (Sourcing them from genesis remains out of scope.) - rehash: write manifest.json atomically via temp file + rename, and split main() into loadManifest/computeEntry/applyEntry. - getShard/index: split getEligiblePool and gracefulShutdown to clear the SonarCloud cognitive-complexity gate (30/32 vs 15 allowed).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/libs/blockchain/routines/Sync.ts (1)
1504-1521: 🩺 Stability & Availability | 🟡 Minor | 🏗️ Heavy liftResidual race: a later
fastSyncentry can clear the abort out from under the detached routine.The queued
runExclusivereset correctly waits for the lock, butfastSyncAbortedis still a shared boolean: onceinSyncLoopis cleared infinally, the nextfastSynccall resets it tofalseat Line 1471 before acquiringsyncLock, so a still-stalled routine can again readfalseand keep going — and the queued reset can later clear the new run's abort. A per-run token (e.g. a monotonically increasing run id the routine compares against) would make the abort unambiguous.🤖 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 1504 - 1521, Replace the shared boolean abort handling in fastSync with a per-run identifier or token, incremented for each new run and captured by the detached routine. Update the abort checks and reset logic around fastSync, inSyncLoop, and the timeout branch to compare against the captured run token, so later fastSync entries cannot clear or reuse an earlier run’s abort state.
🧹 Nitpick comments (5)
src/libs/blockchain/routines/Sync.ts (1)
875-877: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale log prefix.
This helper also serves
syncBlockand fork recovery, so[batchDownloadBlocks]is misleading — the error path was already renamed to[applySyncedBlock].🤖 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 875 - 877, Update the success log in the block application helper to use the existing “[applySyncedBlock]” prefix instead of “[batchDownloadBlocks]”, matching the renamed error path while preserving the rest of the message.src/index.ts (3)
891-893: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass a
reasonlabel togetNextAvailablePort.Every other call site labels itself (
"metrics","mcp"); without it the port-drift warning logsunspecified, making L2PS-IM drift hard to attribute.♻️ Proposed tweak
indexState.L2PS_MESSAGING_PORT = await getNextAvailablePort( indexState.L2PS_MESSAGING_PORT, + "l2ps-messaging", )🤖 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/index.ts` around lines 891 - 893, Update the getNextAvailablePort call that assigns indexState.L2PS_MESSAGING_PORT to pass a descriptive reason label identifying the L2PS-IM/L2PS messaging port, matching the labeled arguments used by the other call sites so drift warnings identify this port correctly.
1449-1459: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalize the L2PS messaging shutdown error before logging.
Interpolating
unknowndirectly yields[object Object]for non-Error throws (Sonar flags this); every other step routes throughhandleError.♻️ Proposed tweak
- error => - log.error(`[SHUTDOWN] Error stopping L2PS Messaging: ${error}`), + error => + log.error( + "[SHUTDOWN] Error stopping L2PS Messaging: " + + (error instanceof Error ? error.message : String(error)), + ),🤖 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/index.ts` around lines 1449 - 1459, Update the L2PS messaging shutdown error callback in shutdownStep to pass the received error through the existing handleError normalization before logging, matching the other shutdown steps and preserving useful messages for non-Error throws.Source: Linters/SAST tools
1295-1300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the meaningless
unknown | Promise<unknown>union.
unknownabsorbs the union, so the declared type is simplyunknown(Sonar flags this).awaitalready handles both sync and async returns.♻️ Proposed tweak
async function shutdownStep( active: boolean, label: string, - stop: () => unknown | Promise<unknown>, + stop: () => unknown, onError: (error: unknown) => void, ): Promise<void> {🤖 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/index.ts` around lines 1295 - 1300, Update the stop callback type in shutdownStep to use unknown instead of the redundant unknown | Promise<unknown> union; keep the existing await behavior so both synchronous and asynchronous return values remain supported.Source: Linters/SAST tools
src/libs/utils/calibrateTime.ts (1)
42-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the measurement itself.
As the guard comment notes,
getMeasuredTimeDeltahas no per-request timeout; a hung NTP round leavesrecalibrationInFlightlatched and silently suspends all recalibration (only repeated warnings) untilstopTimestampRecalibrationruns. APromise.racewith a timeout shorter thanRECALIBRATION_INTERVAL_MSwould keep the loop self-healing.🤖 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/utils/calibrateTime.ts` around lines 42 - 45, Bound the await of getMeasuredTimeDelta in the recalibration loop with a Promise.race timeout shorter than RECALIBRATION_INTERVAL_MS. Ensure a timed-out measurement follows the existing error/cleanup path so recalibrationInFlight is released and the loop can retry, while preserving successful measurement handling.
🤖 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/graphify-out/cache/stat-index.json`:
- Line 1: Remove src/graphify-out/cache/stat-index.json from the committed
changes unless a repository consumer requires it; otherwise regenerate it with
repository-relative paths and deterministic metadata rather than
machine-specific absolute paths and mutable mtime_ns values.
In `@src/libs/consensus/v2/routines/mergeMempools.ts`:
- Around line 173-177: Update mergeMempools around Mempool.receive and
commitPendingContributions to preserve each transaction’s originating peer,
obtain per-hash admission results, and treat persistence failures as
unsuccessful. Commit a staged contributor only when all transactions in that
contributor’s exchange satisfy the admission policy, and handle contributors
with empty exchanges explicitly instead of committing them through the merged
batch result.
---
Duplicate comments:
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 1504-1521: Replace the shared boolean abort handling in fastSync
with a per-run identifier or token, incremented for each new run and captured by
the detached routine. Update the abort checks and reset logic around fastSync,
inSyncLoop, and the timeout branch to compare against the captured run token, so
later fastSync entries cannot clear or reuse an earlier run’s abort state.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 891-893: Update the getNextAvailablePort call that assigns
indexState.L2PS_MESSAGING_PORT to pass a descriptive reason label identifying
the L2PS-IM/L2PS messaging port, matching the labeled arguments used by the
other call sites so drift warnings identify this port correctly.
- Around line 1449-1459: Update the L2PS messaging shutdown error callback in
shutdownStep to pass the received error through the existing handleError
normalization before logging, matching the other shutdown steps and preserving
useful messages for non-Error throws.
- Around line 1295-1300: Update the stop callback type in shutdownStep to use
unknown instead of the redundant unknown | Promise<unknown> union; keep the
existing await behavior so both synchronous and asynchronous return values
remain supported.
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 875-877: Update the success log in the block application helper to
use the existing “[applySyncedBlock]” prefix instead of “[batchDownloadBlocks]”,
matching the renamed error path while preserving the rest of the message.
In `@src/libs/utils/calibrateTime.ts`:
- Around line 42-45: Bound the await of getMeasuredTimeDelta in the
recalibration loop with a Promise.race timeout shorter than
RECALIBRATION_INTERVAL_MS. Ensure a timed-out measurement follows the existing
error/cleanup path so recalibrationInFlight is released and the loop can retry,
while preserving successful measurement handling.
🪄 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 Plus
Run ID: f44ae13f-9e2b-49ea-b173-118a6e1de937
📒 Files selected for processing (17)
scripts/state-snapshot/rehash.tssrc/config/loader.tssrc/graphify-out/cache/ast/v0.9.25/671145763822c10890d71250e86ea6849467fd7c0882b06bd9943b9f3d61a15c.jsonsrc/graphify-out/cache/ast/v0.9.25/ec8ae7cfaad3cd085799fd8cabb7e0d45ccb536a518b59572259be7be331140e.jsonsrc/graphify-out/cache/semantic/pd5fd89c46bb5/31c81c59dc6ebae41078a5fa28207be8d076b2ccad439842f6024c45250045d4.jsonsrc/graphify-out/cache/semantic/pd5fd89c46bb5/963ff91af607f7d819783f5e2d8e8acec8f2791f590b7cd9355dcca023fda7ce.jsonsrc/graphify-out/cache/semantic/pd5fd89c46bb5/d4874874cb4059ac192d609b82ecc9aacaa2adbbc94d939d81ffba9283ef57d2.jsonsrc/graphify-out/cache/stat-index.jsonsrc/index.tssrc/libs/blockchain/mempool.tssrc/libs/blockchain/routines/Sync.tssrc/libs/consensus/v2/routines/getShard.tssrc/libs/consensus/v2/routines/mergeMempools.tssrc/libs/consensus/v2/routines/networkAheadVeto.tssrc/libs/consensus/v2/routines/peerlistMerge.tssrc/libs/network/endpointHandlers.tssrc/libs/utils/calibrateTime.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/libs/consensus/v2/routines/peerlistMerge.ts
- scripts/state-snapshot/rehash.ts
- src/libs/consensus/v2/routines/getShard.ts
- src/libs/consensus/v2/routines/networkAheadVeto.ts
- src/libs/network/endpointHandlers.ts
- Sync: the drift latch was process-local, so a restart cleared it while the orphaned GCR edits stayed in the DB and fast sync re-applied them. Detect the condition durably instead: gcr_assigned_txs stamps each edit with its block, so an assignment above the chain tip can only mean that block never landed. Probed on every apply, failing closed. (Greptile P1) - mergeMempools: Mempool.receive returns success even when it drops invalid txs or swallows an insert failure, so committing every staged contributor on its resolution was too permissive. Track the hashes each peer carried and commit its peerlist only if its own txs actually landed (mempool or already on chain). Exchanges that carried no txs are committed as before. (CodeRabbit) - Untrack graphify-out artifacts committed by mistake and gitignore local tool caches.
The lookup-failure fallback marked every merged transaction as admitted and committed the associated peerlist contributions, even though Mempool.receive may have rejected them. Peers whose lookup succeeded would commit a different set, diverging the hash-sensitive block.content.peerlist and therefore candidate block hashes. Drop the contributions instead: it only costs this round's peerlist additions, and is deterministic with respect to the local failure.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/libs/blockchain/routines/Sync.ts (1)
903-954: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-block aggregate query on the hot sync path.
assertNoUnreconciledGcrState/findOrphanedGcrBlockrun aMAX(block_number)query againstGCRAssignedTxon every call tosyncBlock/applySyncedBlock, i.e. once per block during batch/fast sync. For a long historical replay this adds a DB round trip per block purely for a safety probe whose answer rarely changes. OnceunreconciledGcrBlockis non-null the function short-circuits, but the common (healthy) path still pays this cost on every block.Consider throttling the durable probe (e.g. only check every N blocks, or once per sync round) while keeping the in-process latch check on every call.
🤖 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 903 - 954, Throttle the durable orphaned-state probe in assertNoUnreconciledGcrState so findOrphanedGcrBlock is not queried on every syncBlock/applySyncedBlock call during healthy batch sync. Keep the unreconciledGcrBlock latch check on every invocation, and retain the existing error/latch behavior whenever the throttled probe detects an orphaned block.
🤖 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 929-954: Update the catch in findOrphanedGcrBlock to return null
only when the database error indicates a missing GCRAssignedTx table, matching
the TypeORM/Postgres error shape and code 42P01. Rethrow or otherwise escalate
all other errors while preserving the existing debug log for the tolerated
missing-table case.
In `@src/libs/consensus/v2/routines/mergeMempools.ts`:
- Around line 150-171: Track whether the raw txs payload contained any entries
separately from carried in the merge exchange loop. Update the downstream
contribution acceptance logic to auto-accept only genuinely empty txs arrays,
while rejecting or withholding contributions whose non-empty array produced no
valid carried hashes; preserve normal admission handling for valid transactions.
---
Nitpick comments:
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 903-954: Throttle the durable orphaned-state probe in
assertNoUnreconciledGcrState so findOrphanedGcrBlock is not queried on every
syncBlock/applySyncedBlock call during healthy batch sync. Keep the
unreconciledGcrBlock latch check on every invocation, and retain the existing
error/latch behavior whenever the throttled probe detects an orphaned block.
🪄 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 Plus
Run ID: 4a4b5706-ddfc-4def-894d-dd8895ef1507
📒 Files selected for processing (5)
.gitignoredata/snapshot/manifest.jsondata/snapshot/validators.jsonlsrc/libs/blockchain/routines/Sync.tssrc/libs/consensus/v2/routines/mergeMempools.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- data/snapshot/manifest.json
| async function findOrphanedGcrBlock(): Promise<number | null> { | ||
| try { | ||
| const tip = await Chain.getLastBlockNumber() | ||
| const db = await Datasource.getInstance() | ||
| const repo = db.getDataSource().getRepository(GCRAssignedTx) | ||
| const highest = await repo | ||
| .createQueryBuilder("a") | ||
| .select("MAX(a.block_number)", "max") | ||
| .getRawOne<{ max: number | string | null }>() | ||
|
|
||
| const highestAssigned = Number(highest?.max ?? 0) | ||
| if (!Number.isFinite(highestAssigned) || highestAssigned <= tip) { | ||
| return null | ||
| } | ||
| return highestAssigned | ||
| } catch (e) { | ||
| // Never let the drift probe itself block syncing — on a fresh node the | ||
| // table may not exist yet. | ||
| log.debug( | ||
| `[applySyncedBlock] could not check for orphaned GCR state: ${ | ||
| e instanceof Error ? e.message : String(e) | ||
| }`, | ||
| ) | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What Postgres error code indicates a table/relation does not exist (pg / node-postgres / TypeORM)?
💡 Result:
The PostgreSQL error code for a relation (table, view, sequence, index, or foreign table) that does not exist is 42P01 [1][2]. This is defined by PostgreSQL as the undefined_table condition [2]. When using libraries like node-postgres or TypeORM, this underlying PostgreSQL error code is passed through as part of the error object. 1. node-postgres: The error typically manifests as a database error with code '42P01' [3][4]. 2. TypeORM: The error is often wrapped in a QueryFailedError, which includes the PostgreSQL error code '42P01' [5]. Common causes for this error include: - Missing migrations: The table has not been created yet in the database [2][6]. - Schema search path: The table exists in a schema not included in the session's current search_path [2][7]. - Case sensitivity: The table was created with mixed-case or uppercase letters (requiring double-quotes) but is being queried without them (resulting in PostgreSQL folding the name to lowercase) [8][4]. - Incorrect database: The connection is pointed to a different database than the one containing the table [2][7].
Citations:
- 1: https://dba.stackexchange.com/questions/101570/error-42p01-relation-does-not-exist
- 2: https://pulse.support/kb/postgresql-relation-does-not-exist
- 3: error: relation "users" does not exist brianc/node-postgres#3259
- 4: https://stackoverflow.com/questions/68371539/node-postgresql-42p01-table-relation-does-not-exist-in-independent-file
- 5: https://stackoverflow.com/questions/66909895/typeorm-queryfailederror-relation-does-not-exist
- 6: https://bugstack.ai/errors/nestjs-typeorm-migration-error
- 7: https://stackoverflow.com/questions/19941922/error-relation-does-not-exist
- 8: https://stackoverflow.com/questions/695289/cannot-simply-use-postgresql-table-name-relation-does-not-exist
Narrow the fail-open path to missing-table errors.
The catch currently swallows every DB failure. Since this probe is meant only to tolerate a fresh node where the GCRAssignedTx table is absent, return null only for the missing-table condition and rethrow/escalate other errors so transient DB failures or query bugs don’t silently disable drift detection.
For Postgres, the relation-does-not-exist code is 42P01; use the matcher supported by your DB client/TypeORM error shape.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 949-949: 'e' 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/blockchain/routines/Sync.ts` around lines 929 - 954, Update the
catch in findOrphanedGcrBlock to return null only when the database error
indicates a missing GCRAssignedTx table, matching the TypeORM/Postgres error
shape and code 42P01. Rethrow or otherwise escalate all other errors while
preserving the existing debug log for the tolerated missing-table case.
| // Track which hashes this peer carried so its contribution can be | ||
| // judged on its OWN admission outcome, not the round's aggregate. | ||
| const carried: string[] = [] | ||
| for (const tx of txs) { | ||
| if (tx && typeof tx.hash === "string" && !merged.has(tx.hash)) { | ||
| merged.set(tx.hash, tx) | ||
| if (tx && typeof tx.hash === "string") { | ||
| carried.push(tx.hash) | ||
| if (!merged.has(tx.hash)) { | ||
| merged.set(tx.hash, tx) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Stage the contribution rather than recording it now: the exchange | ||
| // is not complete until the txs it carried are admitted locally | ||
| // below. Recording here would let a failed Mempool.receive leave us | ||
| // holding peerlists whose transactions we never took. | ||
| pendingContributions.push({ | ||
| identity: peer.identity, | ||
| peerlist: payload?.peerlist, | ||
| carried, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Malformed-but-non-empty exchanges are treated the same as legitimately-empty ones.
carried only records hashes where tx.hash is a string; entries that fail that check are silently dropped (line 154) without being counted anywhere. Downstream, a contribution with carried.length === 0 is auto-accepted (line 196-198) regardless of why it's empty. A peer that sends a non-empty txs array made entirely of malformed entries (e.g. hash missing/non-string) ends up with an empty carried list and gets its peerlist committed unconditionally — exactly the "malformed... mempool exchange" the PR objective says should be rejected. This differs from a peer that legitimately sent txs: [], which should be (and is) auto-accepted.
Track whether the peer's raw payload actually contained entries, and only auto-accept when it was genuinely empty:
🛡️ Distinguish empty exchange from malformed exchange
const pendingContributions: Array<{
identity: string
peerlist: unknown
carried: string[]
+ hadEntries: boolean
}> = []
@@
const carried: string[] = []
for (const tx of txs) {
if (tx && typeof tx.hash === "string") {
carried.push(tx.hash)
if (!merged.has(tx.hash)) {
merged.set(tx.hash, tx)
}
}
}
@@
pendingContributions.push({
identity: peer.identity,
peerlist: payload?.peerlist,
carried,
+ hadEntries: txs.length > 0,
}) pendingContributions.filter(contribution => {
if (contribution.carried.length === 0) {
- return true
+ return !contribution.hadEntries
}📝 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.
| // Track which hashes this peer carried so its contribution can be | |
| // judged on its OWN admission outcome, not the round's aggregate. | |
| const carried: string[] = [] | |
| for (const tx of txs) { | |
| if (tx && typeof tx.hash === "string" && !merged.has(tx.hash)) { | |
| merged.set(tx.hash, tx) | |
| if (tx && typeof tx.hash === "string") { | |
| carried.push(tx.hash) | |
| if (!merged.has(tx.hash)) { | |
| merged.set(tx.hash, tx) | |
| } | |
| } | |
| } | |
| // Stage the contribution rather than recording it now: the exchange | |
| // is not complete until the txs it carried are admitted locally | |
| // below. Recording here would let a failed Mempool.receive leave us | |
| // holding peerlists whose transactions we never took. | |
| pendingContributions.push({ | |
| identity: peer.identity, | |
| peerlist: payload?.peerlist, | |
| carried, | |
| }) | |
| } | |
| // Track which hashes this peer carried so its contribution can be | |
| // judged on its OWN admission outcome, not the round's aggregate. | |
| const carried: string[] = [] | |
| for (const tx of txs) { | |
| if (tx && typeof tx.hash === "string") { | |
| carried.push(tx.hash) | |
| if (!merged.has(tx.hash)) { | |
| merged.set(tx.hash, tx) | |
| } | |
| } | |
| } | |
| // Stage the contribution rather than recording it now: the exchange | |
| // is not complete until the txs it carried are admitted locally | |
| // below. Recording here would let a failed Mempool.receive leave us | |
| // holding peerlists whose transactions we never took. | |
| pendingContributions.push({ | |
| identity: peer.identity, | |
| peerlist: payload?.peerlist, | |
| carried, | |
| hadEntries: txs.length > 0, | |
| }) |
🤖 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/consensus/v2/routines/mergeMempools.ts` around lines 150 - 171,
Track whether the raw txs payload contained any entries separately from carried
in the merge exchange loop. Update the downstream contribution acceptance logic
to auto-accept only genuinely empty txs arrays, while rejecting or withholding
contributions whose non-empty array produced no valid carried hashes; preserve
normal admission handling for valid transactions.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
237-247: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftTimeouts leave stale peers and uncancelled work.
Promise.allSettleddiscards timeout failures, so a timed-out peer is neither marked offline nor removed beforegetPeers()is returned. The underlyingsayHelloToPeercall also continues after the race and can recursively launch additional hello calls, allowing late callbacks to mutate the peer list after this method completes. Handle timed-out peers explicitly and make the underlying/recursive requests abortable or bounded.🤖 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 237 - 247, Update the peer refresh flow around Promise.allSettled and sayHelloToPeer so HELLO_TIMEOUT_MS failures are detected explicitly, mark the timed-out peer offline, and remove it before getPeers() returns. Propagate cancellation or enforce bounded recursion through sayHelloToPeer and any recursive hello calls so timed-out work cannot continue mutating peerList after completion.
🤖 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 `@data/snapshot/validators.jsonl`:
- Line 8: Restore valid JSON in the validator snapshot record by assigning valid
timestamp values to first_seen and valid_at instead of leaving them empty.
Preserve the existing null representation for optional timestamp fields such as
unstake_requested_at and unstake_available_at, ensuring the rehash command can
parse the record and update manifest.json.
---
Outside diff comments:
In `@src/libs/peer/PeerManager.ts`:
- Around line 237-247: Update the peer refresh flow around Promise.allSettled
and sayHelloToPeer so HELLO_TIMEOUT_MS failures are detected explicitly, mark
the timed-out peer offline, and remove it before getPeers() returns. Propagate
cancellation or enforce bounded recursion through sayHelloToPeer and any
recursive hello calls so timed-out work cannot continue mutating peerList after
completion.
🪄 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 Plus
Run ID: 8519252a-178b-4166-9c18-6caa01fc8bd4
📒 Files selected for processing (4)
data/snapshot/manifest.jsondata/snapshot/validators.jsonlsrc/libs/consensus/v2/PoRBFT.tssrc/libs/peer/PeerManager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- data/snapshot/manifest.json
- src/libs/consensus/v2/PoRBFT.ts
| {"address":"0x08560420330282fa86dae369a34d9ab49e1e0979fba26abb6326ea6c747e5356","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":3000,"valid_at":3000,"unstake_requested_at":null,"unstake_available_at":null} | ||
| {"address":"0x33061b33179360cc206ca530945a1cfd796f55a8fc52d1c9450a93bcbcd569c6","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":3000,"valid_at":3000,"unstake_requested_at":null,"unstake_available_at":null} | ||
| {"address":"0x35fe178cdcedc66ae1031e63fc851058d50210b7cc566046f3d3ac2cb72aee0a","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":3000,"valid_at":3000,"unstake_requested_at":null,"unstake_available_at":null} | ||
| {"address":"0x765ccff781f21af0d2d6fc86b600dff9eac519358ff0846cb527efddbb0f238c","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":,"valid_at":,"unstake_requested_at":null,"unstake_available_at":null} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore valid JSON for the timestamp fields.
Line 8 cannot be parsed because first_seen and valid_at have no values. This makes the rehash command abort before it can update manifest.json, leaving the snapshot unloadable.
Proposed fix
-{"address":"0x765ccff781f21af0d2d6fc86b600dff9eac519358ff0846cb527efddbb0f238c","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":,"valid_at":,"unstake_requested_at":null,"unstake_available_at":null}
+{"address":"0x765ccff781f21af0d2d6fc86b600dff9eac519358ff0846cb527efddbb0f238c","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":3000,"valid_at":3000,"unstake_requested_at":null,"unstake_available_at":null}📝 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.
| {"address":"0x765ccff781f21af0d2d6fc86b600dff9eac519358ff0846cb527efddbb0f238c","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":,"valid_at":,"unstake_requested_at":null,"unstake_available_at":null} | |
| {"address":"0x765ccff781f21af0d2d6fc86b600dff9eac519358ff0846cb527efddbb0f238c","status":"2","connection_url":"","staked_amount":"1000000000000","first_seen":3000,"valid_at":3000,"unstake_requested_at":null,"unstake_available_at":null} |
🤖 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 `@data/snapshot/validators.jsonl` at line 8, Restore valid JSON in the
validator snapshot record by assigning valid timestamp values to first_seen and
valid_at instead of leaving them empty. Preserve the existing null
representation for optional timestamp fields such as unstake_requested_at and
unstake_available_at, ensuring the rehash command can parse the record and
update manifest.json.
| const highest = await repo | ||
| .createQueryBuilder("a") | ||
| .select("MAX(a.block_number)", "max") | ||
| .getRawOne<{ max: number | string | null }>() | ||
|
|
||
| const highestAssigned = Number(highest?.max ?? 0) | ||
| if (!Number.isFinite(highestAssigned) || highestAssigned <= tip) { |
There was a problem hiding this comment.
When the process terminates after saveGCREditChanges persists a synced block's GCR edits but before bulkUpdateAssignedTxs writes the corresponding markers, the restart guard sees no assignment above the chain tip. The node then reapplies the same block's edits, compounding nonce, balance, or validator-state drift.
Knowledge Base Used: Blockchain Core: Chain, Blocks, Transactions, and Mempool
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/libs/blockchain/routines/Sync.ts
Line: 934-940
Comment:
**Drift marker misses persisted state**
When the process terminates after `saveGCREditChanges` persists a synced block's GCR edits but before `bulkUpdateAssignedTxs` writes the corresponding markers, the restart guard sees no assignment above the chain tip. The node then reapplies the same block's edits, compounding nonce, balance, or validator-state drift.
**Knowledge Base Used:** [Blockchain Core: Chain, Blocks, Transactions, and Mempool](https://app.greptile.com/kynesyslabs/-/custom-context/knowledge-base/kynesyslabs/node/-/docs/blockchain-core.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
2 participants
Changelog
block.content.peerlistDEBUG_ASSERTIONS_ENABLED.env file flaggcr_main=14022 balance_sum=15024999999998830681000000000 storage=2540 size_bytes_sum=3668809 identity=2 validators=12).Not in this PR
Summary by CodeRabbit