Check creation of two services with the same ID #686
Merged
Conversation
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
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds duplicate-Create detection for UpdateService items in accumulation: introduces a hasDuplicatedServices helper (added twice by mistake), validates before and after accumulation, and returns ACCUMULATION_ERROR on duplicates. Imports UpdateService and UpdateServiceKind from @typeberry/state were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Accumulate
participant DupCheck as hasDuplicatedServices
Caller->>Accumulate: accumulate(updates)
Accumulate->>DupCheck: Check duplicates in updates.services
alt Duplicates found
DupCheck-->>Accumulate: true
Accumulate-->>Caller: ACCUMULATION_ERROR
else No duplicates
DupCheck-->>Accumulate: false
Accumulate->>Accumulate: compute state
Accumulate->>DupCheck: Check duplicates in state.services
alt Duplicates found
DupCheck-->>Accumulate: true
Accumulate-->>Caller: ACCUMULATION_ERROR
else None
DupCheck-->>Accumulate: false
Accumulate-->>Caller: accumulated state
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
![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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/jam/transition/accumulate/accumulate.ts (2)
311-354: Consensus-critical: accumulation still continues after per-service failure; must abort block.Loop assigns
currentState = stateUpdate === null ? checkpoint : stateUpdateand proceeds. This contradicts #410’s requirement to fail the entire accumulation on any accumulation error, risking unpaid work and consensus divergence.Proposed minimal change: propagate errors with Result and short-circuit on first failure.
Apply these diffs:
- private async accumulateInParallel( + private async accumulateInParallel( accumulateData: AccumulateData, slot: TimeSlot, entropy: EntropyHash, statistics: Map<ServiceId, CountAndGasUsed>, inputStateUpdate: AccumulationStateUpdate, - ): Promise<ParallelAccumulationResult> { + ): Promise<Result<ParallelAccumulationResult, ACCUMULATION_ERROR>> { @@ - const { consumedGas, stateUpdate } = await this.accumulateSingleService( + const { consumedGas, stateUpdate } = await this.accumulateSingleService( serviceId, accumulateData.getOperands(serviceId), accumulateData.getGasCost(serviceId), slot, entropy, currentState, ); @@ - currentState = stateUpdate === null ? checkpoint : stateUpdate; + if (stateUpdate === null) { + logger.error`Accumulation failed for ${serviceId}; aborting entire accumulation.`; + return Result.error(ACCUMULATION_ERROR); + } + currentState = stateUpdate; @@ - return { - state: currentState, - gasCost, - }; + return Result.ok({ + state: currentState, + gasCost, + });And propagate in sequential accumulation:
- private async accumulateSequentially( + private async accumulateSequentially( gasLimit: ServiceGas, reports: WorkReport[], slot: TimeSlot, entropy: EntropyHash, statistics: Map<ServiceId, CountAndGasUsed>, stateUpdate: AccumulationStateUpdate, - ): Promise<SequentialAccumulationResult> { + ): Promise<Result<SequentialAccumulationResult, ACCUMULATION_ERROR>> { @@ - return { + return Result.ok({ accumulatedReports: tryAsU32(0), gasCost: tryAsServiceGas(0), state: stateUpdate, - }; + }); @@ - const { - gasCost, - state: stateAfterParallelAcc, - ...rest - } = await this.accumulateInParallel(accumulateData, slot, entropy, statistics, stateUpdate); + const par = await this.accumulateInParallel(accumulateData, slot, entropy, statistics, stateUpdate); + if (par.isError) return par as Result<SequentialAccumulationResult, ACCUMULATION_ERROR>; + const { gasCost, state: stateAfterParallelAcc, ...rest } = par.ok; @@ - const { - accumulatedReports, - gasCost: seqGasCost, - state, - ...seqRest - } = await this.accumulateSequentially( + const seq = await this.accumulateSequentially( tryAsServiceGas(gasLimit - gasCost), reportsToAccumulateSequentially, slot, entropy, statistics, stateAfterParallelAcc, ); + if (seq.isError) return seq; + const { accumulatedReports, gasCost: seqGasCost, state, ...seqRest } = seq.ok; @@ - return { + return Result.ok({ accumulatedReports: tryAsU32(i + accumulatedReports), gasCost: tryAsServiceGas(gasCost + seqGasCost), state, - }; + });And handle Result at the call site:
- const { accumulatedReports, gasCost, state, ...rest } = await this.accumulateSequentially( + const seqRes = await this.accumulateSequentially( gasLimit, accumulatableReports, slot, entropy, statistics, AccumulationStateUpdate.empty(), ); + if (seqRes.isError) return Result.error(ACCUMULATION_ERROR); + const { accumulatedReports, gasCost, state, ...rest } = seqRes.ok;This enforces block invalidation on any accumulation error as per #410.
90-199: Update Graypaper links to current version (0.1.3).All Graypaper URLs in
packages/jam/transition/accumulate/accumulate.tsstill usev=0.6.7; change them tov=0.1.3(occurrences at lines 93, 116, 169, 185, 195, 203, 218, 233, 296, 395, 418).
🧹 Nitpick comments (2)
packages/jam/transition/accumulate/accumulate.ts (2)
431-445: Tighten logging and naming in duplicate-create detector.
- Use error severity for consensus-invalid condition.
- Prefer clearer naming: this checks duplicate Create actions, not “duplicated services.”
- private hasDuplicatedServices(updateServices: UpdateService[]): boolean { + private hasDuplicateServiceCreates(updateServices: UpdateService[]): boolean { @@ - if (createdServiceIds.has(serviceId)) { - logger.log`Duplicated Service creation detected ${serviceId}. Block is invalid.`; + if (createdServiceIds.has(serviceId)) { + logger.error`Duplicate service creation detected ${serviceId}. Block is invalid.`; return true; }Also update the call site accordingly (see next comment).
486-489: Good: block invalidation on duplicate service creates; add tests and fail earlier if possible.
- Keep this top-level guard. Add an integration test creating two
UpdateService.createentries with the sameserviceIdacross different services to assertResult.error(ACCUMULATION_ERROR).- Optional: fail faster by tracking created IDs inside
accumulateInParalleland aborting immediately when a duplicate is seen (saves PVM work).Would you like me to add the test scaffolding for this path?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jam/transition/accumulate/accumulate.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts:asconversions must not be used. Suggest usingtryAsconversion methods.
**/*.ts: Classes withstatic Codecfield must have private constructor and staticcreatemethod.
**/*.ts: Casting abigint(orU64) usingNumber(x)must have an explanation comment why
it is safe.
**/*.ts: When making changes to code with comments containing links (in classes, constants, methods, etc.)
to graypaper.fluffylabs.dev, ensure those links point to the current version for this update.
Files:
packages/jam/transition/accumulate/accumulate.ts
🧠 Learnings (1)
📚 Learning: 2025-06-10T12:10:10.532Z
Learnt from: tomusdrw
PR: FluffyLabs/typeberry#419
File: packages/jam/state-merkleization/serialize-update.ts:115-126
Timestamp: 2025-06-10T12:10:10.532Z
Learning: In packages/jam/state-merkleization/serialize-update.ts, service removal is handled separately from service updates. The UpdateServiceKind enum does not include a Remove variant. Service removals are handled via the servicesRemoved parameter in serializeUpdate() which is processed by serializeRemovedServices(), while service updates/creations are handled via servicesUpdates parameter processed by serializeServiceUpdates().
Applied to files:
packages/jam/transition/accumulate/accumulate.ts
🧬 Code graph analysis (1)
packages/jam/transition/accumulate/accumulate.ts (4)
packages/jam/state/in-memory-state.ts (1)
updateServices(368-398)packages/jam/state/state-update.ts (2)
UpdateService(114-151)update(129-134)packages/jam/block/common.ts (1)
ServiceId(26-26)packages/jam/jam-host-calls/logger.ts (1)
logger(3-3)
🔇 Additional comments (1)
packages/jam/transition/accumulate/accumulate.ts (1)
32-34: LGTM on imports.Runtime enum usage (UpdateServiceKind) and type-only import (UpdateService) are appropriate.
tomusdrw
approved these changes
Oct 2, 2025
mateuszsikora
approved these changes
Oct 7, 2025
View all
Benchmarks summary: 63/63 OK ✅ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves: #410