Add option to run accumulation sequentially #836
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR adds an accumulateSequentially boolean option and threads it through the test runner CLI, test preparation, and core accumulation paths. Accumulate and OnChain constructors now accept an options object (AccumulateOptions) containing pvm and accumulateSequentially; callers (test runner, chain-stf, importer, w3f accumulate tests) are updated to pass that object. Accumulate's internal flow now conditionally awaits per-service accumulation when sequential mode is requested. Tests are parameterized to run with both sequential and parallel accumulation modes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jam/node/jam-config.ts (1)
75-76: Consider clarifying the comment.The comment "Disable parallel accumulation" describes the effect when the flag is
true, but it might be clearer to describe the flag's purpose more directly. Consider:- /** Disable parallel accumulation */ + /** When true, run accumulation sequentially instead of in parallel */ public readonly accumulateSequentially: boolean,This more clearly indicates the flag's semantics and when each behavior applies.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
bin/jam/args.test.ts(1 hunks)bin/jam/args.ts(4 hunks)bin/jam/index.ts(1 hunks)bin/rpc/test/e2e-setup.ts(1 hunks)bin/tci/index.test.ts(1 hunks)bin/tci/index.ts(1 hunks)bin/test-runner/common.test.ts(2 hunks)bin/test-runner/common.ts(6 hunks)bin/test-runner/state-transition/state-transition.ts(1 hunks)bin/test-runner/w3f/accumulate.ts(2 hunks)packages/jam/config-node/node-config.ts(1 hunks)packages/jam/node/jam-config.ts(4 hunks)packages/jam/node/main-importer.ts(2 hunks)packages/jam/node/main.ts(1 hunks)packages/jam/transition/accumulate/accumulate.test.ts(1 hunks)packages/jam/transition/accumulate/accumulate.ts(5 hunks)packages/jam/transition/accumulate/index.ts(1 hunks)packages/jam/transition/accumulate/options.ts(1 hunks)packages/jam/transition/chain-stf.ts(5 hunks)packages/workers/importer/importer.ts(3 hunks)packages/workers/importer/main.ts(1 hunks)packages/workers/importer/protocol.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: rules from./CODESTYLE.mdshould be adhered to.
**/*.ts: Any function whose documentation mention it must not be used in production code,
can be safely used in*.test.tsfiles. Other usage should be carefuly reviewed
and the comment must explain why it's safe to use there.
**/*.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/config-node/node-config.tsbin/test-runner/common.test.tsbin/tci/index.tsbin/rpc/test/e2e-setup.tspackages/jam/node/main-importer.tspackages/workers/importer/protocol.tsbin/test-runner/common.tspackages/jam/transition/accumulate/options.tspackages/workers/importer/importer.tsbin/jam/args.test.tsbin/test-runner/state-transition/state-transition.tspackages/workers/importer/main.tspackages/jam/transition/accumulate/accumulate.tspackages/jam/node/main.tsbin/jam/args.tsbin/jam/index.tspackages/jam/node/jam-config.tspackages/jam/transition/accumulate/index.tspackages/jam/transition/accumulate/accumulate.test.tsbin/tci/index.test.tsbin/test-runner/w3f/accumulate.tspackages/jam/transition/chain-stf.ts
🧠 Learnings (2)
📚 Learning: 2025-11-03T10:12:27.580Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 757
File: packages/jam/state-vectors/index.ts:18-36
Timestamp: 2025-11-03T10:12:27.580Z
Learning: In TypeScript files, the coding guideline requiring classes with `static Codec` field to have a private constructor and static `create` method applies specifically to classes using `codec.Class`. When using `codec.object`, this pattern is not required because the code only cares about the shape, not the instance.
Applied to files:
packages/workers/importer/protocol.ts
📚 Learning: 2025-04-24T19:48:31.051Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 351
File: packages/jam/state-merkleization/index.ts:19-37
Timestamp: 2025-04-24T19:48:31.051Z
Learning: In the typeberry codebase, `Bytes` inherits from `BytesBlob`, so a `Bytes` instance can be directly used where a `BytesBlob` is expected without needing conversion.
Applied to files:
packages/jam/transition/accumulate/accumulate.ts
🧬 Code graph analysis (13)
bin/tci/index.ts (1)
packages/jam/config-node/node-config.ts (1)
NODE_DEFAULTS(19-24)
bin/rpc/test/e2e-setup.ts (2)
packages/jam/node/jam-config.ts (1)
JamConfig(22-78)packages/jam/config-node/node-config.ts (1)
NODE_DEFAULTS(19-24)
packages/workers/importer/protocol.ts (2)
packages/core/codec/descriptor.ts (1)
CodecRecord(13-15)packages/jam/transition/accumulate/accumulate.ts (1)
accumulateSequentially(364-432)
bin/test-runner/common.ts (1)
packages/jam/transition/accumulate/accumulate.ts (1)
accumulateSequentially(364-432)
packages/workers/importer/importer.ts (2)
packages/jam/transition/accumulate/options.ts (1)
AccumulateOptions(3-6)packages/jam/transition/chain-stf.ts (1)
OnChain(122-436)
bin/jam/args.test.ts (1)
packages/jam/config-node/node-config.ts (1)
NODE_DEFAULTS(19-24)
bin/test-runner/state-transition/state-transition.ts (1)
packages/jam/transition/chain-stf.ts (2)
OnChain(122-436)DbHeaderChain(45-75)
packages/workers/importer/main.ts (1)
packages/workers/importer/importer.ts (1)
Importer(29-204)
packages/jam/transition/accumulate/accumulate.ts (1)
packages/jam/transition/accumulate/options.ts (1)
AccumulateOptions(3-6)
packages/jam/node/jam-config.ts (1)
packages/jam/transition/accumulate/accumulate.ts (1)
accumulateSequentially(364-432)
packages/jam/transition/accumulate/accumulate.test.ts (5)
packages/jam/config/pvm-backend.ts (1)
PvmBackendNames(2-2)packages/jam/block/common.ts (4)
EntropyHash(40-40)tryAsTimeSlot(18-18)tryAsServiceId(28-28)ServiceId(26-26)packages/jam/config/chain-spec.ts (1)
tinyChainSpec(109-126)packages/jam/block/refine-context.ts (1)
WorkPackageHash(18-18)packages/jam/block/gp-constants.ts (1)
MIN_PUBLIC_SERVICE_INDEX(64-64)
bin/tci/index.test.ts (1)
packages/jam/config-node/node-config.ts (1)
NODE_DEFAULTS(19-24)
bin/test-runner/w3f/accumulate.ts (3)
bin/test-runner/common.ts (1)
RunOptions(95-100)packages/jam/transition/accumulate/accumulate.ts (1)
Accumulate(84-740)packages/core/hash/blake2b.ts (1)
Blake2b(8-44)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Setup test vectors cache
- GitHub Check: benchmarks (22.x)
- GitHub Check: test (22.x)
- GitHub Check: e2e (22.x)
🔇 Additional comments (42)
packages/jam/transition/accumulate/options.ts (1)
1-6: LGTM!The type definition is clean and appropriate for threading accumulation options through the system. The structure clearly groups the PVM backend choice with the sequential accumulation flag.
bin/test-runner/w3f/accumulate.ts (3)
140-140: LGTM!The function signature correctly destructures
accumulateSequentiallyfromRunOptions, aligning with the type definition shown in the relevant code snippets.
143-144: LGTM!The options object is correctly constructed with both
pvmandaccumulateSequentiallyfields, matching theAccumulateOptionstype definition.
155-155: LGTM!The
Accumulateconstructor call correctly passes theoptionsobject as the fourth parameter, matching the updated constructor signature shown in the relevant code snippets.bin/test-runner/common.test.ts (1)
15-15: LGTM!The test expectations correctly include
accumulateSequentially: false, matching the default value established inNODE_DEFAULTSand the updated structure ofparseArgsreturn type.Also applies to: 27-27
bin/tci/index.ts (1)
88-88: LGTM!The
accumulateSequentiallyfield is correctly propagated fromNODE_DEFAULTSinto theJamConfigcreation, maintaining consistency with the default configuration values.packages/jam/transition/accumulate/index.ts (1)
4-4: LGTM!The export statement appropriately extends the public API surface to include the new
AccumulateOptionstype, enabling downstream consumers to import it from the main accumulate module.packages/jam/node/jam-config.ts (2)
32-32: LGTM!The parameter is correctly added to the
JamConfig.newfactory method with an appropriate default value offalse(parallel accumulation by default) and proper type annotation.Also applies to: 43-43
54-54: LGTM!The
accumulateSequentiallyparameter is correctly passed through to the private constructor.bin/jam/index.ts (1)
91-91: LGTM!The
accumulateSequentiallyflag is correctly propagated from the parsed CLI arguments through to theJamConfigconstruction, completing the configuration pipeline from command-line input to runtime config.packages/jam/config-node/node-config.ts (1)
23-23: LGTM!The default value of
falseis appropriate, maintaining parallel accumulation as the default behavior for performance reasons while allowing users to opt into sequential mode for trace readability.bin/jam/args.test.ts (1)
15-15: LGTM!The addition of
accumulateSequentiallyto the default options correctly propagates the new configuration field fromNODE_DEFAULTSinto the test expectations.bin/rpc/test/e2e-setup.ts (1)
14-19: LGTM!The
accumulateSequentiallyparameter is correctly added to theJamConfig.newcall, properly sourced fromNODE_DEFAULTS. The formatting is consistent with the existing parameter structure.packages/workers/importer/importer.ts (3)
2-2: LGTM!The import updates correctly reflect the refactoring from using a bare
PvmBackendto the richerAccumulateOptionstype, which encapsulates both the PVM backend and the sequential accumulation flag.Also applies to: 7-7
41-41: LGTM!The constructor parameter change from
pvm: PvmBackendtooptions: AccumulateOptionsis a clean refactoring that enables passing both the PVM backend and the sequential accumulation flag through a single options object.
55-55: LGTM!The
OnChaininstantiation correctly passes theoptionsobject (containing bothpvmandaccumulateSequentially) as the fourth parameter, aligning with the updatedOnChainconstructor signature.packages/jam/node/main-importer.ts (2)
35-43: LGTM!The
accumulateSequentiallyflag is correctly propagated fromconfig.accumulateSequentiallyinto theInMemWorkerConfigworker parameters.
44-53: LGTM!The
accumulateSequentiallyflag is correctly propagated fromconfig.accumulateSequentiallyinto theLmdbWorkerConfigworker parameters, maintaining consistency with the in-memory configuration path.bin/tci/index.test.ts (1)
20-20: LGTM!The
accumulateSequentiallyparameter is correctly added to the test'sJamConfig.newcall, properly sourced fromNODE_DEFAULTS.accumulateSequentially.packages/jam/node/main.ts (1)
63-67: LGTM!The
accumulateSequentiallyflag is correctly propagated fromconfig.accumulateSequentiallyinto theImporterConfig.createcall within the worker parameters.packages/jam/transition/accumulate/accumulate.test.ts (2)
49-52: LGTM!The test parameterization properly covers both sequential (
true) and parallel (false) accumulation modes by iterating over theaccumulateSequentiallyvalues for each PVM backend. Theoptionsobject is correctly constructed with bothpvmandaccumulateSequentiallyfields, and the test suite name clearly indicates which mode is being tested.
197-197: LGTM!The
Accumulateconstructor is correctly invoked with theoptionsobject (containing bothpvmandaccumulateSequentially), ensuring both accumulation modes are tested with the same test logic.Also applies to: 300-300
bin/jam/args.ts (4)
32-34: LGTM!The help text clearly describes the
--accumulate-sequentiallyoption and its default value. The description "Disable parallel accumulation" effectively conveys the purpose of the flag.
55-55: LGTM!The
SharedOptionstype is correctly extended with theaccumulateSequentially: booleanfield, enabling the flag to be propagated through the configuration system.
111-118: LGTM!The
accumulateSequentiallyflag is correctly parsed using the newparseBooleanOptionhelper and included in the returnedSharedOptionsobject. The parsing logic properly handles the boolean CLI flag by checking if the value istrue(when the flag is present without an explicit value).
212-216: LGTM!The
parseBooleanOptionhelper function correctly implements boolean flag parsing for minimist. It checks if the option value istrue(indicating the flag was present), deletes the option from the args object to prevent it from being flagged as unrecognized, and returns the boolean result.packages/workers/importer/main.ts (1)
24-32: LGTM!The options object is correctly constructed and passed to the
Importerconstructor, aligning with theAccumulateOptionstype. The refactoring cleanly bundlespvmandaccumulateSequentiallyinto a single configuration object.bin/test-runner/state-transition/state-transition.ts (1)
100-106: LGTM!The
OnChainconstructor correctly receives the options object containingpvmandaccumulateSequentially. This aligns with the updated constructor signature and properly propagates the sequential accumulation flag from the test runner options.packages/workers/importer/protocol.ts (1)
81-106: LGTM!The
ImporterConfigclass correctly follows the coding guideline requiring classes withstatic Codecto have a private constructor and staticcreatemethod. The newaccumulateSequentiallyfield is properly added to the codec, factory method, and constructor.packages/jam/transition/accumulate/accumulate.ts (2)
84-94: LGTM!The constructor refactoring from a single
pvmparameter to anAccumulateOptionsobject is clean. The warning log when sequential mode is enabled provides helpful visibility for debugging trace readability issues.
540-544: Sequential accumulation implementation looks correct.The approach of conditionally awaiting each promise when
accumulateSequentiallyis true effectively converts parallel execution to sequential while still collecting all results viaPromise.all. Since each promise is already resolved by the time it's added toresultPromises, the finalPromise.alljust aggregates the results.bin/test-runner/common.ts (6)
35-38: LGTM!The
GlobalsOptionstype is correctly extended with the newaccumulateSequentiallyboolean field.
95-100: LGTM!The
RunOptionstype correctly includes theaccumulateSequentiallyfield for propagation to test execution.
139-149: LGTM!The CLI argument parsing correctly handles
--accumulate-sequentiallyflag. Using=== trueis appropriate for minimist's boolean handling.
172-186: LGTM!The
mainfunction correctly accepts and destructuresaccumulateSequentiallyfrom the options parameter.
242-245: LGTM!The
accumulateSequentiallyoption is correctly bundled intoglobalOptionswhen callingprepareTest.
418-427: LGTM!The
accumulateSequentiallyflag is correctly propagated fromglobalOptionsto theRunOptionspassed to test functions, completing the option flow from CLI through to test execution.packages/jam/transition/chain-stf.ts (5)
5-6: LGTM: Import refactoring is correct.The split between type-only import (
ChainSpec) and value import (PvmBackend) is appropriate. The value import ofPvmBackendis necessary for the enum indexing operation at line 301.
32-32: LGTM: AccumulateOptions type import is correct.The import is consistent with the refactoring to centralize PVM backend configuration and the new sequential accumulation flag under a single options object.
152-152: LGTM: Constructor refactoring improves API design.The change from accepting a direct
PvmBackendparameter to anAccumulateOptionsobject is a clean refactoring that supports the new sequential accumulation feature. The options pattern provides better extensibility and encapsulation.
167-169: LGTM: Initialization logic correctly propagates dependencies.The change correctly passes the full
optionsobject toAccumulate(which needs bothpvmandaccumulateSequentially) while extracting onlyoptions.pvmforDeferredTransfers(which only needs the backend type).
301-301: No issues found. The code correctly referencesthis.accumulate.options.pvmfor runtime logging. TheAccumulateclass exposesoptionsas a public field (declared in the constructor), andAccumulateOptionsincludes thepvmfield, making the access pattern safe and valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/test-runner/common.ts (1)
185-191: Consider simplifying or removing the redundant helper.The
getBooleanOptionhelper is trivial and used only twice. Sinceminimistalready handles boolean parsing with the configuration provided, you could directly useparsed[OPTION](or!!parsed[OPTION]for explicit coercion).🔎 Proposed simplification
- const shouldShowHelp = getBooleanOption(parsed[HELP_OPTION]); + const shouldShowHelp = !!parsed[HELP_OPTION]; if (shouldShowHelp) { console.log(HELP_MESSAGE); process.exit(0); } const pvms = getPvms(parsed[PVM_OPTION]); - const accumulateSequentially = getBooleanOption(parsed[ACCUMULATE_SEQUENTIALLY_OPTION]); + const accumulateSequentially = !!parsed[ACCUMULATE_SEQUENTIALLY_OPTION]; return { initialFiles: parsed._, pvms, accumulateSequentially, }; - - function getBooleanOption(value: unknown): boolean { - if (value === true) { - return true; - } - - return false; - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/test-runner/common.test.ts(4 hunks)bin/test-runner/common.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/test-runner/common.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: rules from./CODESTYLE.mdshould be adhered to.
**/*.ts: Any function whose documentation mention it must not be used in production code,
can be safely used in*.test.tsfiles. Other usage should be carefuly reviewed
and the comment must explain why it's safe to use there.
**/*.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:
bin/test-runner/common.ts
🧬 Code graph analysis (1)
bin/test-runner/common.ts (1)
packages/jam/transition/accumulate/accumulate.ts (1)
accumulateSequentially(364-432)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup test vectors cache
- GitHub Check: test (22.x)
- GitHub Check: e2e (22.x)
🔇 Additional comments (3)
bin/test-runner/common.ts (3)
139-160: Excellent addition of comprehensive help documentation.This help message directly addresses the past review comment requesting help documentation. The structure is clear, includes all options with defaults, and provides useful examples.
37-37: LGTM! Clean propagation of the new option through the test runner.The
accumulateSequentiallyoption is correctly:
- Added to
GlobalsOptionsandRunOptionstypes- Accepted in
mainfunction parameters- Threaded through
prepareTest→createTestDefinitions→ test execution- Made available to test runners via
RunOptionsThis implementation maintains type safety and follows the existing pattern for option propagation.
Also applies to: 99-99, 220-220, 227-227, 284-287, 466-466
162-184: End-to-end propagation ofaccumulateSequentiallyis correctly implemented.The argument parsing extracts
accumulateSequentiallywith a default offalseand threads it throughmain→prepareTest→createTestDefinitions→ test execution. Downstream components (Accumulateconstructor at line 91 andOnChaininitialization) properly receive the option viaAccumulateOptionsand conditionally apply sequential accumulation logic (seepackages/jam/transition/accumulate/accumulate.ts:540for conditional await behavior). The propagation chain is complete and functional.
View all
Benchmarks summary: 83/83 OK ✅ |
I added option to run accumulation sequentially. It makes traces more readable.
without
--accumulate-sequentially:with
--accumulate-sequentially(logs are duplicated because of two pvm interpreters):it resolves #804