Merge stabilisation GCR hotfixes into petri#755
Merged
Conversation
+ more logging
+ update test scenario targets
+ update gcr method names + clean up log.onlys
Improve Transaction Throughput
…isation-into-petri # Conflicts: # testing/devnet/docker-compose.yml
…rge report speculativeExecutor was still using the old Repository-based GCR routine calls (GCRBalanceRoutines.apply(edit, repo, simulate)) which broke after the stabilisation GCR refactor. Updated to use HandleGCR.prepareAccounts() + HandleGCR.applyTransaction() pattern consistent with all other callers. Added MERGE_REPORT_stabilisation_into_petri.md documenting: - All 9 stabilisation hotfixes merged - Conflict resolutions (port staggering) - Code adaptations needed and completed - Follow-up action items https://claude.ai/code/session_01WWNp94cGgFCEVWmTtE2wvV
…ge reports omniprotocol/handlers/gcr.ts was passing Repository<GCRMain> to GCRIdentityRoutines.apply() which now expects a GCRMain entity. Load the entity first, pass it, and persist after successful apply. This was the only merge-introduced type regression (verified via diff against petri baseline — 69 pre-existing errors unchanged). https://claude.ai/code/session_01WWNp94cGgFCEVWmTtE2wvV
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (43)
Disabled knowledge base sources:
WalkthroughThis PR refactors the GCR (Global Consensus Registry) application layer from a per-edit, repository-based approach with simulation flags to a transaction-level batch processing model using in-memory entity mutations. Key changes include: new batched APIs in Changes
Sequence DiagramsequenceDiagram
participant Tx as Transaction
participant HG as HandleGCR
participant Mem as In-Memory<br/>Cache
participant DB as Repository
participant SE as Side Effects
Tx->>HG: applyTransactions([txs])
activate HG
HG->>Mem: prepareAccounts([txs])
activate Mem
Mem->>DB: Load GCRMain entities
DB-->>Mem: Existing + Create entities
Mem-->>HG: accounts Map
deactivate Mem
HG->>Mem: applyTransaction(accounts, tx, false, true)
activate Mem
Mem->>Mem: Clone entities (snapshot)
Mem->>Mem: Apply GCR edits<br/>(in-memory mutations)
Mem-->>HG: GCRApplyResult<br/>(success, entities, sideEffects)
deactivate Mem
alt applyTransaction Success
HG->>HG: Collect sideEffects
HG->>DB: saveGCREditChanges(accounts)
activate DB
DB->>DB: Persist modified entities
DB-->>HG: Saved
deactivate DB
HG->>SE: Execute sideEffects sequentially
activate SE
SE->>SE: IncentiveManager calls
SE-->>HG: Completed
deactivate SE
else applyTransaction Fails
HG->>HG: Rollback (discard snapshots)
HG-->>Tx: Failure result
end
HG-->>Tx: Success result
deactivate HG
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 QodoMerge stabilisation GCR hotfixes into petri with in-memory entity refactor and port staggering
WalkthroughsDescription• **Merges 9 stabilisation hotfixes** into petri, including critical GCR in-memory edit refactor and TX count inversion bug fix • **Refactors GCR API to in-memory entity pattern**: All GCR routines (HandleGCR, GCRBalanceRoutines, GCRIdentityRoutines, etc.) now accept pre-loaded GCRMain entities instead of repository parameters, enabling batch processing • **Implements batch transaction processing**: New prepareAccounts() and applyTransaction() methods in HandleGCR replace per-edit database operations with deferred bulk SQL updates via bulkUpdateAssignedTxs() • **Defers side effects**: Identity and rewards routines now return async sideEffect callbacks for execution after all edits succeed, improving transaction atomicity • **Fixes petri merge regressions**: Updates speculativeExecutor.ts and omniprotocol/handlers/gcr.ts to use new unified GCR batch API • **Resolves docker-compose port conflicts**: Adopts staggered port scheme (RPC/Omni interleaved: 53551/53552, 53553/53554, etc.) across all test infrastructure and devnet configuration • **Fixes 3 bugs**: TX count inversion in mempool metrics, bash array handling in test scripts, docker compose command syntax, and PeerConnection state transition validation • **Improves test infrastructure**: Updates all test targets, timeouts, and port configurations to align with new staggered scheme; adds readable connection state names to omni reconnection tests Diagramflowchart LR
A["Old Repository-based<br/>GCR API"] -->|"Refactor"| B["New In-Memory<br/>Entity Pattern"]
B -->|"prepareAccounts()"| C["Batch Load<br/>Accounts"]
C -->|"applyTransaction()"| D["Process All<br/>Edits In-Memory"]
D -->|"saveGCREditChanges()"| E["Bulk Persist<br/>+ Side Effects"]
F["Sequential Ports<br/>53551-53554"] -->|"Stagger"| G["Interleaved Ports<br/>RPC/Omni"]
H["speculativeExecutor.ts<br/>omniprotocol/handlers/gcr.ts"] -->|"Adapt"| B
File Changes1. src/libs/blockchain/gcr/handleGCR.ts
|
Contributor
Code Review by Qodo
1. Omni creates invalid GCRMain
|
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+132
to
+151
| let accountGCR = await gcrMainRepository.findOneBy({ | ||
| pubkey: editOperation.account, | ||
| }) | ||
| if (!accountGCR) { | ||
| accountGCR = gcrMainRepository.create({ | ||
| pubkey: editOperation.account, | ||
| }) | ||
| } | ||
|
|
||
| // Apply the identity operation (simulate = false for actual execution) | ||
| const result = await gcrIdentityRoutines.apply( | ||
| editOperation, | ||
| gcrMainRepository, | ||
| accountGCR, | ||
| false, // simulate = false (actually apply changes) | ||
| ) | ||
|
|
||
| // Persist the modified entity | ||
| if (result.success) { | ||
| await gcrMainRepository.save(accountGCR) | ||
| } |
Contributor
There was a problem hiding this comment.
1. Omni creates invalid gcrmain 🐞 Bug ≡ Correctness
handleIdentityAssign() creates a GCRMain with only pubkey and passes it into GCRIdentityRoutines.apply(), which assumes required fields (e.g., identities/nonce/balance/assignedTxs) exist. This can throw during identity application or fail when saving due to non-null columns lacking defaults.
Agent Prompt
## Issue description
`handleIdentityAssign` creates a new `GCRMain` with only `pubkey`, then passes it into `GCRIdentityRoutines.apply()` and persists it. `GCRMain` has required fields without defaults, so the entity must be fully initialized before routines run and before saving.
## Issue Context
`HandleGCR.createAccount(pubkey, fillData, skipSave)` already encapsulates correct initialization for `balance`, `nonce`, `identities`, `assignedTxs`, etc.
## Fix Focus Areas
- src/libs/omniprotocol/protocol/handlers/gcr.ts[128-151]
- src/libs/blockchain/gcr/handleGCR.ts[1004-1065]
## Proposed fix
- When `findOneBy` returns null, replace `gcrMainRepository.create({ pubkey })` with `await HandleGCR.createAccount(pubkey, {}, true)` (skipSave=true), then continue applying edits.
- Keep `save(accountGCR)` after success, or alternatively call `createAccount(pubkey)` (skipSave=false) to insert immediately, then apply edits and save again if needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+355
to
+357
| // Batch load all affected GCRMain entities | ||
| const gcrMainRepo = dataSource.getRepository(GCRMain) | ||
| const gcrMainCache = new Map<string, GCRMain>() |
Contributor
There was a problem hiding this comment.
2. Uninitialized datasource usage 🐞 Bug ☼ Reliability
HandleGCR.prepareAccounts() and saveGCREditChanges() use the exported dataSource directly, but the repo initializes it only through Datasource.getInstance(). If these methods run before any getInstance() call, TypeORM will throw “DataSource is not initialized” and crash transaction simulation/consensus/sync paths.
Agent Prompt
## Issue description
`HandleGCR.prepareAccounts()` / `saveGCREditChanges()` call `dataSource.getRepository(...)` directly. This assumes the exported `dataSource` has already been initialized, but initialization only happens via `Datasource.getInstance()`.
## Issue Context
`Datasource.getInstance()` is the codebase’s standard entrypoint that calls `dataSource.initialize()` exactly once.
## Fix Focus Areas
- src/libs/blockchain/gcr/handleGCR.ts[332-382]
- src/libs/blockchain/gcr/handleGCR.ts[609-620]
- src/model/datasource.ts[39-90]
## Proposed fix
- In both methods, replace direct `dataSource.getRepository(...)` usage with:
- `const db = await Datasource.getInstance()`
- `const repo = db.getDataSource().getRepository(GCRMain)`
- Alternatively, add a small internal helper like `await this.getRepositories()` (already ensures `Datasource.getInstance()`) and use that repo instead of the global export.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+486
to
+489
| // INFO: If on a serious run, rollback hard edits | ||
| if (!simulate && !this.GCRTxTypes.has(edit.type)) { | ||
| await this.rollback(tx, accounts, appliedEdits) | ||
| } |
Contributor
There was a problem hiding this comment.
3. Rollback skips db edits 🐞 Bug ≡ Correctness
applyTransaction() decides whether to rollback based on the *failed* edit type, so a tx can perform DB-writing edits (e.g., storageProgram) and then fail on a “batchable” edit without rolling back the earlier DB writes. This can commit partial state for a transaction that is ultimately rejected.
Agent Prompt
## Issue description
`applyTransaction()` currently triggers rollback only when the *failing* edit is not in `GCRTxTypes`. This can skip rollback even if earlier edits in the same tx performed DB writes.
## Issue Context
- `storageProgram` and `tlsnotary` routines can persist directly to DB when `simulate=false`.
- `appliedEdits` already tracks edits successfully applied before the failure.
## Fix Focus Areas
- src/libs/blockchain/gcr/handleGCR.ts[450-497]
- src/libs/blockchain/gcr/handleGCR.ts[130-144]
## Proposed fix
- Change the rollback condition to consider `appliedEdits`, not the failing `edit.type`, e.g.:
- `const needsRollback = appliedEdits.some(e => !this.GCRTxTypes.has(e.type))`
- `if (!simulate && needsRollback) await this.rollback(tx, accounts, appliedEdits)`
- Also ensure `GCRTxTypes` only contains truly in-memory/no-DB-write edit types (likely just `balance`, `nonce`, `identity`), so DB-writing types always trigger rollback when present in `appliedEdits`.
- (Stronger) Wrap per-tx application in a DB transaction for non-batchable edits so failures are atomic.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
3 participants
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.
Summary
speculativeExecutor.ts,omniprotocol/handlers/gcr.ts) still used the oldRepository-based GCR API after mergeStabilisation hotfixes brought in
979eb8cGCRMainentity instead ofRepository628937bprepareAccounts/applyTransactionAPI74a4a6efdd86f0e82ce9b5dd0a9bb57292aPetri-specific fixes (post-merge)
speculativeExecutor.ts— refactored to useHandleGCR.prepareAccounts()+applyTransaction()instead of directGCRBalanceRoutines.apply(edit, repo, simulate)omniprotocol/handlers/gcr.ts— loadsGCRMainentity before passing toGCRIdentityRoutines.apply(), persists after successTypecheck
omni_reconnection.ts)Test plan
See
MERGE_REPORT_stabilisation_into_petri.mdin repo root for full analysis.https://claude.ai/code/session_01WWNp94cGgFCEVWmTtE2wvV
Summary by CodeRabbit
Refactor
Chores