Merged
Conversation
+ more logging
+ update test scenario targets
+ update gcr method names + clean up log.onlys
Contributor
WalkthroughRefactors GCR processing from per-edit repository operations to an in-memory, batch-apply model with preloaded Changes
Sequence Diagram(s)sequenceDiagram
participant Consensus
participant HandleGCR
participant AccountLoader as AccountLoader
participant InMemory as In-Memory<br/>GCRMain
participant Persister
Consensus->>HandleGCR: applyTransactions(txs, simulate=false)
HandleGCR->>AccountLoader: prepareAccounts(txs)
AccountLoader-->>HandleGCR: Map<pubkey,GCRMain>
loop per tx
HandleGCR->>InMemory: applyTransaction(accounts, tx, isRollback?, simulate?)
InMemory-->>HandleGCR: GCRApplyResult (accounts, sideEffects, status)
end
HandleGCR->>Persister: saveGCREditChanges(accounts, sideEffects)
Persister-->>HandleGCR: persisted / side-effects executed
HandleGCR-->>Consensus: result
sequenceDiagram
participant IdentityRoutine
participant GCRMainEntity as GCRMain Entity
participant IncentiveManager
Note over IdentityRoutine: applyWeb2IdentityAdd(editOp, accountGCR)
IdentityRoutine->>GCRMainEntity: mutate identities.web2[...] in-memory
IdentityRoutine-->>IdentityRoutine: return { success, entity, sideEffect: awardPoints }
Note over IdentityRoutine,IncentiveManager: later execution
IdentityRoutine->>IncentiveManager: awardPoints() -> IncentiveManager.*Linked()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Review Summary by QodoBatch GCR edit processing with in-memory entity handling and infrastructure updates
WalkthroughsDescription• Refactored HandleGCR to support batch application of GCR edits with in-memory entity processing, eliminating per-transaction database operations • Added new batch processing methods: applyTransactions(), applyTransaction(), prepareAccounts(), and saveGCREditChanges() for efficient bulk operations • Converted all identity routine modules (TLSN, Ethos, Human Passport, Nomis, XM, Web2, PQC, UD) to accept in-memory GCRMain entities instead of database repositories • Refactored balance and nonce routines with new applyToEntity static methods for in-memory batch processing • Updated consensus, sync, and endpoint execution layers to use new batch GCR processing API • Moved incentive point operations and database saves into deferred async sideEffect callbacks for post-batch execution • Improved mempool transaction filtering with better error handling for duplicate key constraints • Updated testing infrastructure port configurations (53552/53554 → 53555/53557) and reduced RPC wait timeouts from 120s to 3s • Fixed docker compose command syntax in devnet run script Diagramflowchart LR
A["Transaction Input"] --> B["prepareAccounts"]
B --> C["In-Memory GCRMain Entity"]
C --> D["applyTransaction"]
D --> E["Identity Routines<br/>Balance/Nonce/Rewards"]
E --> F["Entity + SideEffect"]
F --> G["saveGCREditChanges"]
G --> H["Bulk Database Update"]
I["Consensus/Sync/Endpoint"] --> A
File Changes1. src/libs/blockchain/gcr/handleGCR.ts
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
src/libs/blockchain/mempool_v2.ts (1)
154-208:⚠️ Potential issue | 🟠 MajorOnly hide hashes after the transaction is actually stored.
Every incoming hash is inserted into
noSendBackTxsbefore the save attempt. Ifthis.repo.save(tx)fails for a non-duplicate reason, the catch only logs and the final response still omits that hash, so the peer getssuccess: trueand never retries a transaction we did not persist.Possible fix
const noSendBackTxs = new Map<string, string>() for (const tx of incoming) { const isCoherent = TxUtils.isCoherent(tx) if (!isCoherent) { log.error( @@ return { success: false, mempool: [], } } - noSendBackTxs.set(tx.hash, tx.hash) } const blockNumber = SecretaryManager.lastBlockRef const existingHashes = await this.getMempoolHashMap(blockNumber) @@ for (const tx of incoming) { if (existingHashes[tx.hash]) { + noSendBackTxs.set(tx.hash, tx.hash) log.info(`tx already exists: ${tx.hash}`) continue } try { await this.repo.save(tx) + noSendBackTxs.set(tx.hash, tx.hash) } catch (error) { if ( error instanceof QueryFailedError && error.message.includes( "duplicate key value violates unique constraint", @@ ) { noSendBackTxs.set(tx.hash, tx.hash) continue } - // INFO: Log other errors - console.error(error) + throw error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/mempool_v2.ts` around lines 154 - 208, The code currently adds every incoming tx.hash into noSendBackTxs before attempting to persist, causing peers to think those txs were accepted even if repo.save(tx) fails; change the flow in receive (the loop using TxUtils.isCoherent, TxUtils.validateSignature and the later save via this.repo.save) so that noSendBackTxs.set(tx.hash, tx.hash) is only executed after a successful save OR when you detect a duplicate in the catch (the QueryFailedError branch); remove the initial noSendBackTxs.set in the first validation loop and ensure existingHashes (from getMempoolHashMap) still short-circuits duplicates before save.src/libs/blockchain/gcr/gcr_routines/routines/rewardsRoutines.ts (1)
11-27:⚠️ Potential issue | 🔴 CriticalMutate
accountGCR, not a second lookup.This routine still calls
ensureGCRForUser(address)and updatesaccount, but it returnsentity: accountGCR. In the new batch flow the caller will persistaccountGCR, so this points edit is effectively dropped.Possible fix
export async function applyAwardPoints( editOperation: any, accountGCR: GCRMain, ): Promise<GCRResult> { - const { account: address, amount, date } = editOperation - const account = await ensureGCRForUser(address) + const { amount, date } = editOperation const challengeEntry = { date, points: amount, } - if (!account.points.breakdown.weeklyChallenge) { - account.points.breakdown.weeklyChallenge = [] + if (!accountGCR.points.breakdown.weeklyChallenge) { + accountGCR.points.breakdown.weeklyChallenge = [] } - account.points.breakdown.weeklyChallenge.push(challengeEntry) - account.points.totalPoints = (account.points.totalPoints || 0) + amount - account.points.lastUpdated = new Date() + accountGCR.points.breakdown.weeklyChallenge.push(challengeEntry) + accountGCR.points.totalPoints = + (accountGCR.points.totalPoints || 0) + amount + accountGCR.points.lastUpdated = new Date() return { success: true, message: "Points awarded", entity: accountGCR } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/routines/rewardsRoutines.ts` around lines 11 - 27, The function is mutating the locally named variable account (result of ensureGCRForUser(address)) but returns entity: accountGCR, so changes are never persisted; update the routine to operate on the same object you return—either rename account to accountGCR or assign ensureGCRForUser(address) to accountGCR, then perform the breakdown/weeklyChallenge push, totalPoints and lastUpdated updates on that accountGCR object (ensuring points.breakdown and weeklyChallenge are initialized if missing) and return entity: accountGCR so the caller's batch persistence picks up the edits.src/libs/consensus/v2/PoRBFT.ts (1)
129-152:⚠️ Potential issue | 🟠 MajorReintroduce the phase-4 secretary barrier.
After batched GCR application and
L2PSConsensus.applyPendingProofs(), this flow goes straight to forging. The only remaining phase-4 update in the file is inside the old commented helper below, so validators no longer synchronize after step 4.🛠️ Possible fix
if (l2psResult.proofsFailed > 0) { log.warning( `[consensusRoutine] ${l2psResult.proofsFailed} L2PS proofs failed verification`, ) } + + await updateValidatorPhase(4, blockRef) // REVIEW Re-merge the mempools anyway to get the correct mempool from the whole shard🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/consensus/v2/PoRBFT.ts` around lines 129 - 152, The flow skips the phase-4 synchronization so validators can start forging prematurely; after finishing GCR application (applyGCREditsFromMergedMempool) and L2PSConsensus.applyPendingProofs(blockRef, false) you must reintroduce the phase-4 secretary barrier so all validators wait for phase 4 to complete before forging. Insert a call to the project’s phase-4 synchronization method (e.g., the secretary/PhaseSecretary barrier function used elsewhere in this file or repo) immediately after L2PSConsensus.applyPendingProofs returns and before any forging/next-phase logic in consensusRoutine, ensuring it awaits completion.
🧹 Nitpick comments (2)
src/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.ts (1)
10-15: Collapseapply()ontoapplyToEntity().The same balance rules now live in two places and they already diverge:
apply()mutateseditOperation.operationon rollback, whileapplyToEntity()doesn't. Reusing one implementation will keep the batched and non-batched paths consistent and lets you delete the commented repository code.♻️ Possible consolidation
static async apply( editOperation: GCREdit, accountGCR: GCRMain, - // gcrMainRepository: Repository<GCRMain>, - // simulate: boolean, ): Promise<GCRResult> { - if (editOperation.type !== "balance") { - return { success: false, message: "Invalid GCREdit type" } - } - - const editOperationAccount = - typeof editOperation.account !== "string" - ? forgeToHex(editOperation.account) - : editOperation.account - - // Safeguarding the operation by checking if the amount is positive - if (editOperation.amount <= 0) { - return { success: false, message: "Invalid amount" } - } - - log.debug( - "Applying GCREdit balance: " + - editOperation.operation + - " " + - editOperation.amount + - " " + - editOperationAccount + - " " + - (editOperation.isRollback ? "ROLLBACK" : "NORMAL"), - ) - // Reversing the operation if it is a rollback - if (editOperation.isRollback) { - editOperation.operation = - editOperation.operation === "add" ? "remove" : "add" - } - - // Getting the actual balance to apply the operation - const actualBalance = accountGCR.balance - - if (editOperation.operation === "add") { - accountGCR.balance = - BigInt(accountGCR.balance) + BigInt(editOperation.amount) - } else if (editOperation.operation === "remove") { - // Safeguarding the operation - // NOTE PROD flag is used to enable testing when not in production - if ( - (actualBalance < editOperation.amount || - actualBalance === 0n) && - getSharedState.PROD - ) { - return { success: false, message: "Insufficient balance" } - } - accountGCR.balance = - BigInt(accountGCR.balance) - BigInt(editOperation.amount) - - // Safeguarding the operation by checking if the balance is negative - // NOTE This applies just to the non-production environment - if (accountGCR.balance < 0n && !getSharedState.PROD) { - accountGCR.balance = 0n - } - } - - return { success: true, message: "Balance applied", entity: accountGCR } + const result = this.applyToEntity(editOperation, accountGCR) + return { ...result, entity: accountGCR } }Also applies to: 93-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.ts` around lines 10 - 15, The apply() implementation duplicates balance-rule logic from applyToEntity() and they diverge (apply() mutates editOperation.operation on rollback while applyToEntity() does not); refactor by collapsing one into the other (e.g., have apply() delegate to applyToEntity() or vice versa) so there is a single source of truth in GCRBalanceRoutines, remove the commented repository parameters, and ensure rollback handling is unified — stop mutating editOperation.operation in one path or move that mutation into the shared implementation so both batched and non-batched flows behave identically.src/libs/l2ps/L2PSConsensus.ts (1)
253-264: Remove the archived per-edit code path.These commented blocks make the new batch flow much harder to audit, and Sonar is already flagging them. The old implementation is safer to keep in git history than inline in the file.
Also applies to: 274-297, 565-590
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/l2ps/L2PSConsensus.ts` around lines 253 - 264, Remove the archived per-edit commented code blocks (e.g., the commented-out rollbackEdits function and the other commented sections referenced around the older per-edit flow) so the file only contains the active batch flow; find occurrences of the commented rollbackEdits code and any other commented per-edit logic (search for "rollbackEdits", "rollbackEdit", and long commented blocks in L2PSConsensus) and delete those commented lines entirely — leave no commented legacy per-edit implementation in the source (it will remain in git history).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libs/blockchain/gcr/gcr_routines/routines/ethosRoutines.ts`:
- Around line 18-25: The function applyEthosIdentityUpsert currently
destructures editOperation.data without guarding that data exists; change it to
first assign a local variable (e.g., const data = editOperation.data) and then
validate using optional chaining or an explicit check before destructuring (or
access properties via data?.chain, data?.subchain, data?.address) so the
function returns a structured GCRResult instead of throwing when
editOperation.data is undefined; mirror the safe pattern used in
applyEthosIdentityRemove to locate the correct spot and update checks
accordingly.
In `@src/libs/blockchain/gcr/gcr_routines/routines/utils.ts`:
- Line 4: The import statement 'import { dataSource } from
"@/model/datasource";' contains a trailing semicolon that violates the project's
`semi` ESLint rule; remove the stray semicolon so the line reads without the
final ';' (locate the import of dataSource in utils.ts) to satisfy formatting
rules and avoid the lint failure.
In `@src/libs/blockchain/gcr/handleGCR.ts`:
- Around line 368-377: prepareAccounts() currently creates unsaved GCRMain
objects for every missing pubkey (via HandleGCR.createAccount and
gcrMainCache.set) causing blank rows to be persisted later by
saveGCREditChanges() even for failed/no-op transactions; modify the flow to
track a dirty set (e.g., touchedPubkeys or dirtyPubkeys) and only add pubkeys to
gcrMainCache and persist them in saveGCREditChanges() when the transaction
successfully mutates state: 1) in places where you now call
HandleGCR.createAccount (including the loop over affectedPubkeys and the similar
block at lines ~597-607), create in-memory placeholders but do not set them into
gcrMainCache until the edit is confirmed successful; 2) when an operation
actually mutates an account, add its pubkey to the dirty set and ensure
saveGCREditChanges() iterates only over dirtyPubkeys (or checks a isDirty flag
on GCRMain) before writing to the DB; and 3) ensure rollback/error paths clear
any temporary placeholders so failed/no-op txs do not create rows.
- Around line 449-475: The code currently only handles applyGCREdit failures via
result.success, but applyGCREdit can throw and bypass snapshot restoration and
rollback; wrap the ApplyGCREdit call in a try/catch, and in the catch perform
the same snapshot restoration loop and non-batchable rollback as in the
result.success === false branch; additionally, maintain a separate list (e.g.,
appliedNonBatchableEdits) that you push to only when an edit is non-batchable
(use this list instead of appliedEdits when calling this.rollback) so you don't
double-rollback edits already restored from snapshots; ensure both the
false-result branch and the catch share the same unwind logic to avoid leaving
partial work.
- Around line 597-613: saveGCREditChanges currently saves the final batch state
then runs queued sideEffects (nomis/xm/tlsn) which call isFirstConnection(),
causing rewards to be computed against the post-batch DB state and running them
with Promise.all makes order nondeterministic and failure handling partial; fix
by moving incentive evaluation into applyTransaction() when each edit is applied
(or flush sideEffects immediately per transaction), i.e., evaluate and
enqueue/execute nomis/xm/tlsn callbacks inside applyTransaction for each
transaction (or run sideEffects sequentially per transaction instead of
collecting them for the whole batch), run them before persisting the next
transaction state (or at least before the final gcrMainRepo.save in
saveGCREditChanges), and replace Promise.all(sideEffects.map(...)) with
sequential execution that catches and surfaces individual errors so one
rejection doesn’t silently produce a partial commit.
- Around line 401-404: The spread [...tx.content.gcr_edits] runs before the
empty-edit check and will throw when tx.content.gcr_edits is undefined/null; to
fix, first check that tx.content && Array.isArray(tx.content.gcr_edits) (or use
a guarded expression like tx.content?.gcr_edits) and only spread when that check
passes, e.g. compute gcrEdits after the guard (or initialize gcrEdits = [] when
not an array) so the subsequent fast-path if (!gcrEdits ||
!Array.isArray(gcrEdits) || gcrEdits.length === 0) can run without an exception;
update the code in handleGCR around the gcrEdits declaration and any use of
tx.content.gcr_edits accordingly.
- Around line 560-567: The sender value used as the key in assignedTxsUpdates
must be normalized the same way prepareAccounts() does to avoid storing a
Uint8Array (which later causes pubkey.replace is not a function in
bulkUpdateAssignedTxs); update the block in handleGCR.ts where sender is derived
(around assignedTxsUpdates usage) to convert/normalize
tx.content?.from_ed25519_address or tx.content.from into the canonical string
form used by prepareAccounts() before using it as a Map key and before calling
assignedTxsUpdates.get/ set/ push so lookups and downstream updates use the same
string pubkey format.
In `@src/libs/blockchain/mempool_v2.ts`:
- Around line 147-152: The early return in Mempool.receive() (checking
getSharedState.inConsensusLoop in src/libs/blockchain/mempool_v2.ts) currently
returns { success: false, mempool: [] } which causes RPC callers to treat it as
an error; change this to return a successful no-op (e.g., { success: true,
mempool: [] }) when writes are intentionally disabled so callers (like the
endpoint handler that calls Mempool.receive()) treat it as a skipped/preserved
merge rather than a failure. Ensure the change is made in the function that
contains the getSharedState.inConsensusLoop check (Mempool.receive / the
early-exit block) and keep the returned mempool as an empty array to indicate
nothing was persisted.
In `@src/libs/blockchain/routines/Sync.ts`:
- Around line 725-726: The current syncGCRTables function swallows failed GCR
batch results from HandleGCR.applyTransactions; change it to propagate failures
by checking the result of HandleGCR.applyTransactions(txs, false) and throwing
an Error (including the returned failure details) when result.success === false
so callers like syncBlock and batchDownloadBlocks cannot silently continue;
update or leave callers to await and let the thrown error bubble up for proper
abort/handling.
In `@src/libs/consensus/v2/PoRBFT.ts`:
- Around line 397-407: The code filters mempool into pendingTxs but when
pendingTxs.length === 0 it returns empty failedTxs, dropping duplicates; update
the early return in the function (PoRBFT flow around the mempool filter using
variables mempool, existingTxHashes, failedTxs, pendingTxs) to return the
collected failedTxs instead of an empty array so callers can prune those
duplicate hashes from the mempool.
- Around line 367-368: The rollback helper rollbackGCREditsFromTxs currently
returns whatever HandleGCR.applyTransactions(txs, true) yields, but
applyTransactions can report partial failures; update rollbackGCREditsFromTxs to
check the result of HandleGCR.applyTransactions(txs, true) and if any
transaction failed to rollback (partial failure) throw an error or otherwise
fail hard so the invalid-block path cannot continue with a partially rolled-back
GCR state; reference HandleGCR.applyTransactions and rollbackGCREditsFromTxs
when implementing this guard.
In `@src/libs/l2ps/L2PSConsensus.ts`:
- Around line 334-339: The current flow commits GCR edits with
HandleGCR.saveGCREditChanges() before markProofApplied() (called later in
applyProof()), which can leave edits persisted while the proof status write
fails; make these two writes atomic by performing both the GCR persistence and
the proof-status update in a single transactional operation: either move the
call to HandleGCR.saveGCREditChanges(...) into applyProof() and wrap it together
with markProofApplied(...) in one DB transaction, or extend
HandleGCR.saveGCREditChanges to accept a callback/closure that executes
markProofApplied(proofId, status) inside the same transaction so both commits
succeed or both roll back. Ensure you reference and update the code paths around
saveGCREditChanges, markProofApplied, and applyProof to implement the
single-transaction behavior.
- Around line 602-611: The code calls HandleGCR.applyTransaction and then
unconditionally calls HandleGCR.saveGCREditChanges and resets proof state,
allowing a failed rollback to persist partial changes; modify the flow in the
block around HandleGCR.applyTransaction (the editResult handling for mockTx) to
check editResult.success before saving or changing proof state, and if success
is false, abort further persistence/setting of proof to pending (log or throw
and return early) so that saveGCREditChanges(editResult.accounts,
editResult.sideEffects) and any proof status update only run when
editResult.success is true.
- Around line 300-317: The code currently builds a synthetic transaction with an
empty account when no proof.gcr_edits passes isBatchableGCREdit; before calling
createMockTx/HandleGCR.prepareAccounts/HandleGCR.applyTransaction, validate that
you found a non-empty account (from the loop over proof.gcr_edits using
isBatchableGCREdit) and if none is found reject the proof by throwing an error
or returning a failed result; update the logic around createMockTx(proof,
account) to only proceed when account is set and add a clear error message
indicating the proof contains no account-backed edits so HandleGCR is not
invoked with a blank account.
In `@testing/devnet/run-devnet`:
- Line 150: The runner currently appends a bare flag
FINAL_COMMAND="${START_COMMAND} --no-tui" which causes digestArguments() to
treat it as an unexpected standalone command and exit; fix this by passing the
flag as a key=value pair (e.g., "--no-tui=true") or update digestArguments() to
accept standalone boolean flags (treat arguments starting with "--" and no "="
as boolean true) so that FINAL_COMMAND contains a key=value style argument or
digestArguments() recognizes bare flags; update the code that builds
FINAL_COMMAND or the parsing logic in digestArguments() and ensure tests cover
both "--no-tui" and "--no-tui=true" cases.
In `@testing/loadgen/src/features/l2ps/shared.ts`:
- Around line 60-61: The readiness timeout defaults for RPC and TX are set too
low—change the envInt calls that define waitForRpcSec and waitForTxSec to use
more conservative defaults (e.g., 30 or 60 seconds) so cold starts/CI don’t
flake; update the envInt invocations for waitForRpcSec and waitForTxSec to
higher default values and ensure any callers that assume implicit short waits
still respect these variables rather than hardcoding shorter values (search for
uses of waitForRpcSec and waitForTxSec and make sure they’re passed through to
the readiness/polling logic).
In `@testing/loadgen/src/token_shared.ts`:
- Line 219: The RPC readiness wait default was lowered to 3s causing cold-start
flakes; update the wait invocation at the call to waitForRpcReady to use the
same 120s default used elsewhere by changing the envInt default from 3 to 120
(i.e., envInt("WAIT_FOR_RPC_SEC", 120)) so shared wallet bootstrap uses a
longer, consistent timeout.
In `@testing/scripts/testenv-doctor.ts`:
- Around line 56-67: The unconditional console.log(res) breaks --json output;
update the probe so debug output is only emitted when verbose is enabled and
suppressed in JSON mode: replace the direct console.log(res) with a conditional
check that uses the runtime flags (e.g. --verbose and --json) so that res is
logged only when verbose is true and --json is not set (i.e., if (verbose &&
!jsonMode) console.log(res)); also ensure any other probe debug prints follow
the same gating (e.g., avoid printing in the catch path when JSON mode is
requested).
---
Outside diff comments:
In `@src/libs/blockchain/gcr/gcr_routines/routines/rewardsRoutines.ts`:
- Around line 11-27: The function is mutating the locally named variable account
(result of ensureGCRForUser(address)) but returns entity: accountGCR, so changes
are never persisted; update the routine to operate on the same object you
return—either rename account to accountGCR or assign ensureGCRForUser(address)
to accountGCR, then perform the breakdown/weeklyChallenge push, totalPoints and
lastUpdated updates on that accountGCR object (ensuring points.breakdown and
weeklyChallenge are initialized if missing) and return entity: accountGCR so the
caller's batch persistence picks up the edits.
In `@src/libs/blockchain/mempool_v2.ts`:
- Around line 154-208: The code currently adds every incoming tx.hash into
noSendBackTxs before attempting to persist, causing peers to think those txs
were accepted even if repo.save(tx) fails; change the flow in receive (the loop
using TxUtils.isCoherent, TxUtils.validateSignature and the later save via
this.repo.save) so that noSendBackTxs.set(tx.hash, tx.hash) is only executed
after a successful save OR when you detect a duplicate in the catch (the
QueryFailedError branch); remove the initial noSendBackTxs.set in the first
validation loop and ensure existingHashes (from getMempoolHashMap) still
short-circuits duplicates before save.
In `@src/libs/consensus/v2/PoRBFT.ts`:
- Around line 129-152: The flow skips the phase-4 synchronization so validators
can start forging prematurely; after finishing GCR application
(applyGCREditsFromMergedMempool) and L2PSConsensus.applyPendingProofs(blockRef,
false) you must reintroduce the phase-4 secretary barrier so all validators wait
for phase 4 to complete before forging. Insert a call to the project’s phase-4
synchronization method (e.g., the secretary/PhaseSecretary barrier function used
elsewhere in this file or repo) immediately after
L2PSConsensus.applyPendingProofs returns and before any forging/next-phase logic
in consensusRoutine, ensuring it awaits completion.
---
Nitpick comments:
In `@src/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.ts`:
- Around line 10-15: The apply() implementation duplicates balance-rule logic
from applyToEntity() and they diverge (apply() mutates editOperation.operation
on rollback while applyToEntity() does not); refactor by collapsing one into the
other (e.g., have apply() delegate to applyToEntity() or vice versa) so there is
a single source of truth in GCRBalanceRoutines, remove the commented repository
parameters, and ensure rollback handling is unified — stop mutating
editOperation.operation in one path or move that mutation into the shared
implementation so both batched and non-batched flows behave identically.
In `@src/libs/l2ps/L2PSConsensus.ts`:
- Around line 253-264: Remove the archived per-edit commented code blocks (e.g.,
the commented-out rollbackEdits function and the other commented sections
referenced around the older per-edit flow) so the file only contains the active
batch flow; find occurrences of the commented rollbackEdits code and any other
commented per-edit logic (search for "rollbackEdits", "rollbackEdit", and long
commented blocks in L2PSConsensus) and delete those commented lines entirely —
leave no commented legacy per-edit implementation in the source (it will remain
in git history).
🪄 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: 64d09fd9-c3b2-4904-aa3b-ee133c25f04a
📒 Files selected for processing (36)
src/libs/blockchain/gcr/gcr.tssrc/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.tssrc/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.tssrc/libs/blockchain/gcr/gcr_routines/GCRNonceRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/ethosRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/humanpassportRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/nomisRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/pqcRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/rewardsRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/tlsnRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/udRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/utils.tssrc/libs/blockchain/gcr/gcr_routines/routines/web2Routines.tssrc/libs/blockchain/gcr/gcr_routines/routines/xmRoutines.tssrc/libs/blockchain/gcr/gcr_routines/routines/zkRoutines.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/blockchain/mempool_v2.tssrc/libs/blockchain/routines/Sync.tssrc/libs/consensus/v2/PoRBFT.tssrc/libs/consensus/v2/routines/mergeMempools.tssrc/libs/l2ps/L2PSConsensus.tssrc/libs/network/endpointExecution.tssrc/libs/peer/routines/peerBootstrap.tstesting/devnet/.env.exampletesting/devnet/docker-compose.ymltesting/devnet/run-devnettesting/devnet/scripts/generate-peerlist.shtesting/devnet/scripts/setup.shtesting/docker-compose.perf.ymltesting/loadgen/src/features/l2ps/shared.tstesting/loadgen/src/features/omni/shared.tstesting/loadgen/src/features/tlsnotary/shared.tstesting/loadgen/src/token_shared.tstesting/scripts/run-chaos-token-script-transfer.shtesting/scripts/run-suite.tstesting/scripts/testenv-doctor.ts
💤 Files with no reviewable changes (3)
- src/libs/peer/routines/peerBootstrap.ts
- src/libs/consensus/v2/routines/mergeMempools.ts
- src/libs/blockchain/gcr/gcr_routines/routines/zkRoutines.ts
Comment on lines
+602
to
+611
| const editResult = await HandleGCR.applyTransaction( | ||
| accounts, | ||
| mockTx, | ||
| true, | ||
| false, | ||
| ) | ||
| await HandleGCR.saveGCREditChanges( | ||
| editResult.accounts, | ||
| editResult.sideEffects, | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't continue after a failed rollback batch.
This branch persists editResult.accounts and resets the proof to pending without checking editResult.success. A partial rollback would leave L1 state half-reverted while making the proof eligible for replay.
🛠️ Suggested guard
const editResult = await HandleGCR.applyTransaction(
accounts,
mockTx,
true,
false,
)
+
+ if (!editResult.success) {
+ throw new Error(
+ `Failed to rollback proof ${proof.id}: ${editResult.message}`,
+ )
+ }
+
await HandleGCR.saveGCREditChanges(
editResult.accounts,
editResult.sideEffects,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/l2ps/L2PSConsensus.ts` around lines 602 - 611, The code calls
HandleGCR.applyTransaction and then unconditionally calls
HandleGCR.saveGCREditChanges and resets proof state, allowing a failed rollback
to persist partial changes; modify the flow in the block around
HandleGCR.applyTransaction (the editResult handling for mockTx) to check
editResult.success before saving or changing proof state, and if success is
false, abort further persistence/setting of proof to pending (log or throw and
return early) so that saveGCREditChanges(editResult.accounts,
editResult.sideEffects) and any proof status update only run when
editResult.success is true.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/libs/blockchain/gcr/handleGCR.ts (3)
368-377:⚠️ Potential issue | 🟠 MajorOnly persist mutated accounts, not the full preloaded cache.
prepareAccounts()eagerly creates placeholderGCRMainentities, andsaveGCREditChanges()persists the entire map. Failed/no-op transactions can still produce blank rows.Proposed direction
- static async saveGCREditChanges( - accounts: Map<string, GCRMain>, - sideEffects: (() => Promise<void>)[], - ) { - const entitiesToSave = accounts.values().toArray() + static async saveGCREditChanges( + accounts: Map<string, GCRMain>, + dirtyPubkeys: Set<string>, + sideEffects: (() => Promise<void>)[], + ) { + const entitiesToSave = Array.from(dirtyPubkeys) + .map(pubkey => accounts.get(pubkey)) + .filter((e): e is GCRMain => !!e)// In applyTransaction/applyTransactions: + const dirtyPubkeys = new Set<string>() + // add pubkey only when an edit successfully mutates that account + dirtyPubkeys.add(pubkey)Also applies to: 609-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/handleGCR.ts` around lines 368 - 377, prepareAccounts() currently eagerly creates placeholder GCRMain entities via HandleGCR.createAccount and gcrMainCache, and saveGCREditChanges() persists the entire gcrMainCache which causes blank rows for failed/no-op txns; change the flow to only persist accounts that were actually mutated: add and maintain a mutatedPubkeys Set (or flag on GCRMain) whenever an account is created or modified (e.g., in prepareAccounts(), transaction handlers, and wherever HandleGCR.createAccount is called), and update saveGCREditChanges() to iterate only over mutatedPubkeys to persist those entities (do not save the whole gcrMainCache); ensure placeholders remain in-memory unless marked mutated so they aren’t written to DB.
470-489:⚠️ Potential issue | 🔴 CriticalRollback can double-apply reversals for batchable edits after snapshot restore.
When a non-batchable edit fails, snapshots already restore batchable state, but
rollback(tx, accounts, appliedEdits)still replays all prior edits, including batchables.Proposed fix
- const appliedEdits: GCREdit[] = [] + const appliedEdits: GCREdit[] = [] + const appliedNonBatchableEdits: GCREdit[] = [] ... appliedEdits.push(edit) + if (!this.GCRTxTypes.has(edit.type)) { + appliedNonBatchableEdits.push(edit) + } ... - if (!simulate && !this.GCRTxTypes.has(edit.type)) { - await this.rollback(tx, accounts, appliedEdits) + if (!simulate && !this.GCRTxTypes.has(edit.type)) { + await this.rollback(tx, accounts, appliedNonBatchableEdits) }Also applies to: 504-505, 891-927
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/handleGCR.ts` around lines 470 - 489, The rollback currently replays all prior edits after restoring snapshots, causing batchable edits to be applied twice; update the logic around rollback(tx, accounts, appliedEdits) so it does not re-apply edits whose types are in this.GCRTxTypes (i.e., filter appliedEdits to exclude edits where this.GCRTxTypes.has(edit.type)) or add a flag to rollback to skip batchable edits; apply the same fix to the other identical blocks that use snapshots/accounts and call rollback (the blocks guarded by simulate and GCRTxTypes checks).
571-573:⚠️ Potential issue | 🟠 MajorGuard sender before normalization.
normalizePubkey(...)runs before null checks. If bothfrom_ed25519_addressandfromare missing, this can throw before theif (sender && tx.hash)guard.Proposed fix
- const sender = normalizePubkey( - tx.content?.from_ed25519_address || tx.content.from, - ) - if (sender && tx.hash) { + const senderRaw = + tx.content?.from_ed25519_address ?? tx.content?.from + if (senderRaw && tx.hash) { + const sender = normalizePubkey(senderRaw) if (!assignedTxsUpdates.has(sender)) { assignedTxsUpdates.set(sender, []) } assignedTxsUpdates.get(sender).push(tx.hash) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/handleGCR.ts` around lines 571 - 573, The code calls normalizePubkey(...) immediately on tx.content?.from_ed25519_address || tx.content.from which can throw if both are missing; instead, first read the raw sender value (e.g., rawSender = tx.content?.from_ed25519_address || tx.content?.from), check that rawSender exists (and tx.hash if needed), then call normalizePubkey(rawSender) and assign to sender. Update any existing guards around sender and tx.hash to use the normalized value only after this existence check; refer to normalizePubkey and the sender variable in handleGCR to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libs/blockchain/gcr/handleGCR.ts`:
- Around line 668-677: The switch handling editOperation.type calls
GCRBalanceRoutines.apply, GCRNonceRoutines.apply, and GCRIdentityRoutines.apply
with a nullable account; add an explicit guard before dispatch (e.g., in the
switch or immediately above it) that checks if account is null/undefined and
handles it deterministically (throw a clear error or return a rejected result)
so the routines never receive a null account; reference the switch on
editOperation.type and the three routines GCRBalanceRoutines.apply,
GCRNonceRoutines.apply, GCRIdentityRoutines.apply when locating where to add the
null-check and error handling.
- Line 613: The code uses Iterator.toArray() which is unsupported on Node.js
<24; update the assignment to build the array compatibly by replacing the use of
accounts.values().toArray() when setting const entitiesToSave so it uses
Array.from(accounts.values()) (or an equivalent Array.from(...) conversion) to
ensure compatibility across Node.js versions while keeping the rest of the logic
in handleGCR.ts unchanged.
---
Duplicate comments:
In `@src/libs/blockchain/gcr/handleGCR.ts`:
- Around line 368-377: prepareAccounts() currently eagerly creates placeholder
GCRMain entities via HandleGCR.createAccount and gcrMainCache, and
saveGCREditChanges() persists the entire gcrMainCache which causes blank rows
for failed/no-op txns; change the flow to only persist accounts that were
actually mutated: add and maintain a mutatedPubkeys Set (or flag on GCRMain)
whenever an account is created or modified (e.g., in prepareAccounts(),
transaction handlers, and wherever HandleGCR.createAccount is called), and
update saveGCREditChanges() to iterate only over mutatedPubkeys to persist those
entities (do not save the whole gcrMainCache); ensure placeholders remain
in-memory unless marked mutated so they aren’t written to DB.
- Around line 470-489: The rollback currently replays all prior edits after
restoring snapshots, causing batchable edits to be applied twice; update the
logic around rollback(tx, accounts, appliedEdits) so it does not re-apply edits
whose types are in this.GCRTxTypes (i.e., filter appliedEdits to exclude edits
where this.GCRTxTypes.has(edit.type)) or add a flag to rollback to skip
batchable edits; apply the same fix to the other identical blocks that use
snapshots/accounts and call rollback (the blocks guarded by simulate and
GCRTxTypes checks).
- Around line 571-573: The code calls normalizePubkey(...) immediately on
tx.content?.from_ed25519_address || tx.content.from which can throw if both are
missing; instead, first read the raw sender value (e.g., rawSender =
tx.content?.from_ed25519_address || tx.content?.from), check that rawSender
exists (and tx.hash if needed), then call normalizePubkey(rawSender) and assign
to sender. Update any existing guards around sender and tx.hash to use the
normalized value only after this existence check; refer to normalizePubkey and
the sender variable in handleGCR to locate the change.
🪄 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: 9a02f0bc-c388-4041-8a46-ce24fb20180b
📒 Files selected for processing (3)
src/libs/blockchain/gcr/gcr_routines/routines/utils.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/consensus/v2/PoRBFT.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/libs/blockchain/gcr/gcr_routines/routines/utils.ts
- src/libs/consensus/v2/PoRBFT.ts
| accounts: Map<string, GCRMain>, | ||
| sideEffects: (() => Promise<void>)[], | ||
| ) { | ||
| const entitiesToSave = accounts.values().toArray() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking runtime/compiler config for iterator-helper compatibility..."
fd -i 'package.json|tsconfig*.json' | while read -r f; do
echo ">>> $f"
rg -n '"engines"|"node"|target|lib' "$f" || true
done
echo
echo "Finding iterator toArray usage in TS sources..."
rg -n --type ts '\.toArray\('Repository: kynesyslabs/node
Length of output: 882
🏁 Script executed:
head -50 package.jsonRepository: kynesyslabs/node
Length of output: 3592
🏁 Script executed:
rg -A 5 '"engines"' package.jsonRepository: kynesyslabs/node
Length of output: 42
🏁 Script executed:
# Check what accounts is and verify it's actually a Map or similar iterable
rg -B 10 -A 2 'const entitiesToSave = accounts\.values\(\)\.toArray' src/libs/blockchain/gcr/handleGCR.tsRepository: kynesyslabs/node
Length of output: 504
🌐 Web query:
Iterator.toArray() JavaScript support Node.js version requirements
💡 Result:
Iterator.prototype.toArray is part of the ECMAScript 2025 Iterator Helpers proposal (Stage 4), which adds methods like map, filter, take, and toArray to Iterator.prototype for lazy iterator transformations. It converts an iterator's yielded values into an Array, equivalent to Array.from(iterator) or [...iterator] but chainable with other helpers. As of 2026-04-01 (today's date), Node.js fully supports Iterator.prototype.toArray starting from version 24.0.0 (released May 2025), which includes V8 13.6. This V8 version implements iterator helpers, including toArray. Node.js 24 is LTS ("Krypton"), with ongoing support. Earlier versions like 22.x (V8 12.4) do not support it natively—polyfills like core-js are needed. Browser support (for reference): Chrome 122+, Firefox 131+, Safari 18.4+ (per caniuse/MDN, baseline March 2025). Example: function* fib { let a = 1, b = 1; while (true) { yield a; [a, b] = [b, a + b]; } } const evens = fib.take(10).filter(x => x % 2 === 0).toArray; // [2, 8, 34, ...]
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator/toArray
- 2: https://caniuse.com/mdn-javascript_builtins_iterator_toarray
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator
- 4: https://tc39.es/proposal-iterator-helpers
- 5: https://nodejs.org/en/blog/release/v24.0.0
- 6: https://toolstac.com/news/2025-09-06/javascript-iterator-operators-ecmascript
- 7: https://blog.logrocket.com/iterator-helpers-es2025
- 8: https://2ality.com/2025/06/ecmascript-2025.html
- 9: https://www.nodejs.org
- 10: https://v8.dev/features/iterator-helpers
Iterator.toArray() is unsupported on Node.js < 24.0.0.
Line 613 uses accounts.values().toArray(), which requires Node.js 24.0.0+ (ES2025 Iterator Helpers). Since this project does not specify a minimum Node.js version in engines, it implicitly supports earlier versions where this method is unavailable, causing runtime crashes. Replace with Array.from() for compatibility across all Node.js versions.
Safe fix
- const entitiesToSave = accounts.values().toArray()
+ const entitiesToSave = Array.from(accounts.values())📝 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.
Suggested change
| const entitiesToSave = accounts.values().toArray() | |
| const entitiesToSave = Array.from(accounts.values()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/blockchain/gcr/handleGCR.ts` at line 613, The code uses
Iterator.toArray() which is unsupported on Node.js <24; update the assignment to
build the array compatibly by replacing the use of accounts.values().toArray()
when setting const entitiesToSave so it uses Array.from(accounts.values()) (or
an equivalent Array.from(...) conversion) to ensure compatibility across Node.js
versions while keeping the rest of the logic in handleGCR.ts unchanged.
Comment on lines
668
to
677
| switch (editOperation.type) { | ||
| case "balance": | ||
| result = await GCRBalanceRoutines.apply( | ||
| editOperation, | ||
| repositories.main as Repository<GCRMain>, | ||
| simulate, | ||
| ) | ||
| result = await GCRBalanceRoutines.apply(editOperation, account) | ||
| break | ||
| case "nonce": | ||
| result = await GCRNonceRoutines.apply( | ||
| editOperation, | ||
| repositories.main as Repository<GCRMain>, | ||
| simulate, | ||
| ) | ||
| result = await GCRNonceRoutines.apply(editOperation, account) | ||
| break | ||
| case "identity": | ||
| result = await GCRIdentityRoutines.apply( | ||
| editOperation, | ||
| repositories.main as Repository<GCRMain>, | ||
| simulate, | ||
| ) | ||
| result = await GCRIdentityRoutines.apply(editOperation, account) | ||
| break |
Contributor
There was a problem hiding this comment.
Add an explicit account null-check before routine dispatch.
balance/nonce/identity branches pass account directly, but account is nullable. A missing map entry can become a runtime failure inside routine implementations.
Proposed guard
switch (editOperation.type) {
case "balance":
+ if (!account) {
+ return { success: false, message: "Missing account for balance edit" }
+ }
result = await GCRBalanceRoutines.apply(editOperation, account)
break
case "nonce":
+ if (!account) {
+ return { success: false, message: "Missing account for nonce edit" }
+ }
result = await GCRNonceRoutines.apply(editOperation, account)
break
case "identity":
+ if (!account) {
+ return { success: false, message: "Missing account for identity edit" }
+ }
result = await GCRIdentityRoutines.apply(editOperation, account)
break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libs/blockchain/gcr/handleGCR.ts` around lines 668 - 677, The switch
handling editOperation.type calls GCRBalanceRoutines.apply,
GCRNonceRoutines.apply, and GCRIdentityRoutines.apply with a nullable account;
add an explicit guard before dispatch (e.g., in the switch or immediately above
it) that checks if account is null/undefined and handles it deterministically
(throw a clear error or return a rejected result) so the routines never receive
a null account; reference the switch on editOperation.type and the three
routines GCRBalanceRoutines.apply, GCRNonceRoutines.apply,
GCRIdentityRoutines.apply when locating where to add the null-check and error
handling.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor
HandleGCRto support batch application of GCR edits with in-mem GCR entities.Summary by CodeRabbit
New Features
Bug Fixes
Configuration
Chores