Bump test-vectors #758
Bump test-vectors #758
Conversation
tomusdrw
commented
Nov 3, 2025
tomusdrw
commented
- latest w3f-davxy main branch
- latest jam-conformance traces
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis changeset: adds a json.fromBigInt helper; switches multiple gas-related JSON deserializers from number to bigint; extends UpdatePreimage.provide to include a providedFor (serviceId) field; propagates providedFor through preimage creation and tests; refactors accumulation to collect per-service parallel results as a Map and introduces mergePerallelAccumulationResults plus helpers (getTransfers, updateStatistics, updateYieldedRoots); adds accumulation-result-merge-utils to merge per-service results; updates accumulate-externalities to remove per-field privileged-service reconciliation and to apply preimage updates to both target and current service when different; adjusts test runners and load-test REF. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@fluffylabs-bot benchmark |
|
✅ Benchmark workflow triggered successfully! 🎉 🔗 Check the Actions tab for workflow progress. |
Picofuzz Benchmark Resultsfallback
safrole
storage
storage_light
🤖 Automated benchmark |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jam/transition/accumulate/accumulate.ts (1)
868-878: Return the actual pending transfers
legacyTransfersonly accumulates values in the pre‑0.7.1 path, so for modern flows we always return an empty array even when services emitted transfers. After fixing the merge above, the real data sits instate.transfers—we should return it.- pendingTransfers: legacyTransfers, + pendingTransfers: Compatibility.isGreaterOrEqual(GpVersion.V0_7_1) + ? state.transfers + : legacyTransfers,Otherwise every transfer executed during accumulation is silently dropped.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/scripts/load-test-ref.sh(1 hunks)bin/test-runner/jam-conformance-070.ts(1 hunks)bin/test-runner/w3f-davxy-071.ts(1 hunks)packages/core/json-parser/descriptors.ts(1 hunks)packages/jam/block-json/work-report.ts(1 hunks)packages/jam/block-json/work-result.ts(1 hunks)packages/jam/state-json/statistics.ts(2 hunks)packages/jam/state/in-memory-state.test.ts(7 hunks)packages/jam/state/state-update.ts(2 hunks)packages/jam/transition/accumulate/accumulate.ts(15 hunks)packages/jam/transition/externalities/accumulate-externalities.test.ts(7 hunks)packages/jam/transition/externalities/accumulate-externalities.ts(3 hunks)packages/jam/transition/preimages.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/core/**/*.ts
⚙️ CodeRabbit configuration file
packages/core/**/*.ts: Core packages must not import any JAM-related packages
(i.e. packages defined underpackages/jam/**)
Files:
packages/core/json-parser/descriptors.ts
**/*.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/core/json-parser/descriptors.tspackages/jam/block-json/work-result.tspackages/jam/state/in-memory-state.test.tspackages/jam/state/state-update.tsbin/test-runner/w3f-davxy-071.tspackages/jam/block-json/work-report.tspackages/jam/transition/preimages.tsbin/test-runner/jam-conformance-070.tspackages/jam/state-json/statistics.tspackages/jam/transition/externalities/accumulate-externalities.test.tspackages/jam/transition/externalities/accumulate-externalities.tspackages/jam/transition/accumulate/accumulate.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 419
File: packages/jam/transition/accumulate/accumulate.ts:193-193
Timestamp: 2025-06-10T12:04:56.072Z
Learning: In the typeberry codebase, the Service.getPreimage() method was updated to return BytesBlob | null directly instead of PreimageItem | null, so the returned value is already a BytesBlob and doesn't need .blob property access.
📚 Learning: 2025-06-10T12:20:17.513Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 419
File: packages/jam/state/state-inmemory.ts:141-149
Timestamp: 2025-06-10T12:20:17.513Z
Learning: In the `InMemoryService.copyFrom` function in `packages/jam/state/state-inmemory.ts`, duplicate checking for `(hash, length)` pairs in the lookup history is not necessary because the function operates under the assumption that the input `ServiceEntries` comes from an existing well-formed state, which already maintains the invariant of unique lookup history entries per hash and length combination.
Applied to files:
packages/jam/state/in-memory-state.test.ts
📚 Learning: 2025-06-10T12:10:10.532Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 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/state/in-memory-state.test.tspackages/jam/state/state-update.tspackages/jam/transition/externalities/accumulate-externalities.test.tspackages/jam/transition/externalities/accumulate-externalities.tspackages/jam/transition/accumulate/accumulate.ts
📚 Learning: 2025-06-10T12:04:56.072Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 419
File: packages/jam/transition/accumulate/accumulate.ts:193-193
Timestamp: 2025-06-10T12:04:56.072Z
Learning: In the typeberry codebase, the Service.getPreimage() method was updated to return BytesBlob | null directly instead of PreimageItem | null, so the returned value is already a BytesBlob and doesn't need .blob property access.
Applied to files:
packages/jam/state/state-update.tspackages/jam/transition/externalities/accumulate-externalities.test.tspackages/jam/transition/externalities/accumulate-externalities.tspackages/jam/transition/accumulate/accumulate.ts
🧬 Code graph analysis (8)
packages/core/json-parser/descriptors.ts (1)
packages/core/json-parser/types.ts (2)
Parser(28-28)FromJsonWithParser(31-31)
packages/jam/block-json/work-result.ts (1)
packages/jam/block/common.ts (1)
tryAsServiceGas(32-32)
packages/jam/state/state-update.ts (2)
packages/jam/state/service.ts (1)
PreimageItem(158-174)packages/jam/block/common.ts (1)
TimeSlot(16-16)
packages/jam/block-json/work-report.ts (1)
packages/jam/block/common.ts (1)
tryAsServiceGas(32-32)
packages/jam/state-json/statistics.ts (2)
packages/jam/block/common.ts (1)
tryAsServiceGas(32-32)packages/core/utils/compatibility.ts (1)
Compatibility(50-109)
packages/jam/transition/externalities/accumulate-externalities.test.ts (3)
packages/jam/block/common.ts (1)
tryAsServiceId(28-28)packages/jam/state/state-update.ts (1)
UpdatePreimage(24-103)packages/jam/state/service.ts (1)
PreimageItem(158-174)
packages/jam/transition/externalities/accumulate-externalities.ts (5)
packages/jam/state/common.ts (1)
PerCore(8-8)packages/jam/block/common.ts (3)
ServiceId(26-26)ServiceGas(31-31)tryAsServiceId(28-28)packages/jam/state/privileged-services.ts (1)
PrivilegedServices(10-48)packages/jam/state/state-update.ts (2)
update(130-135)UpdatePreimage(24-103)packages/jam/state/service.ts (1)
PreimageItem(158-174)
packages/jam/transition/accumulate/accumulate.ts (8)
packages/jam/jam-host-calls/externalities/state-update.ts (1)
AccumulationStateUpdate(51-138)packages/jam/block/common.ts (5)
ServiceGas(31-31)ServiceId(26-26)tryAsServiceGas(32-32)TimeSlot(16-16)EntropyHash(40-40)packages/core/numbers/index.ts (3)
U32(21-21)sumU64(93-104)tryAsU32(45-48)packages/jam/jam-host-calls/externalities/pending-transfer.ts (1)
PendingTransfer(12-37)packages/jam/transition/statistics.ts (1)
CountAndGasUsed(51-54)packages/jam/transition/accumulate/accumulate-data.ts (1)
AccumulateData(26-215)packages/jam/state/privileged-services.ts (1)
PrivilegedServices(10-48)packages/jam/state/common.ts (1)
tryAsPerCore(10-16)
⏰ 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). (6)
- GitHub Check: e2e (22.x)
- GitHub Check: benchmarks (22.x)
- GitHub Check: run (22.x)
- GitHub Check: state_transition (22.x)
- GitHub Check: run (22.x)
- GitHub Check: test (22.x)
🔇 Additional comments (8)
bin/test-runner/jam-conformance-070.ts (1)
9-10: LGTM! Appropriate handling of buggy test vectors.The addition of these ignored test vectors is well-documented and aligns with the test-vectors bump in this PR.
.github/scripts/load-test-ref.sh (1)
2-2: LGTM! Test vectors reference updated.The update to the new commit hash aligns with the PR objective of bumping test-vectors.
bin/test-runner/w3f-davxy-071.ts (1)
9-9: LGTM! Test coverage expanded.Removing previously ignored test vectors suggests these tests are now passing with the updated test-vectors.
packages/core/json-parser/descriptors.ts (1)
15-26: LGTM! Well-designed BigInt parser.The new
fromBigIntfunction provides flexible handling of numeric inputs by accepting bothnumberandbigint, converting numbers to BigInt before parsing. Using"object"as the discriminator (similar tofromAny) is appropriate for handling multiple input types.packages/jam/block-json/work-result.ts (1)
80-80: LGTM! Consistent BigInt migration for gas field.The change from
fromNumbertofromBigIntforaccumulate_gasis compatible withtryAsServiceGaswhich acceptsnumber | bigint, and aligns with the BigInt migration for gas fields across the codebase.packages/jam/block-json/work-report.ts (1)
50-50: LGTM! Consistent BigInt migration for gas field.The migration of
auth_gas_usedtofromBigIntis consistent with the broader gas field updates across the codebase.packages/jam/state-json/statistics.ts (1)
48-48: LGTM! Comprehensive BigInt migration for all gas fields.All gas-related fields in statistics (
gas_used,refinement_gas_used,accumulate_gas_used,on_transfers_gas_used) have been consistently migrated to usefromBigInt, aligning with the broader codebase changes.Also applies to: 80-80, 86-86, 90-90
packages/jam/transition/preimages.ts (1)
84-84: LGTM! Appropriate addition of providedFor metadata.Adding
providedFor: requesterto track which service the preimage is being provided for is logically sound and aligns with the infrastructure changes for preimage provision metadata tracking mentioned in the PR summary.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jam/transition/accumulate/accumulate.ts (1)
646-728: Surface transfers produced by the ≥0.7.1 accumulation path.In the modern branch we never mutate
legacyTransfers, yet we still return it aspendingTransfers, so every transfer emitted during accumulation disappears from the transition output. Thread the actual transfers coming back fromaccumulateSequentiallyand return them here (e.g. extendSequentialAccumulationResultto includetransfersand propagate that value).- const { accumulatedReports, gasCost, state, ...rest } = Compatibility.isGreaterOrEqual(GpVersion.V0_7_1) + const { + accumulatedReports, + gasCost, + state, + transfers: pendingTransfers, + ...rest + } = Compatibility.isGreaterOrEqual(GpVersion.V0_7_1) ? await this.accumulateSequentially( gasLimit, accumulatableReports, - [], + [], slot, entropy, statistics, AccumulationStateUpdate.empty(), autoAccumulateServices, yieldedRoots, ) : await this.accumulateSequentiallyLegacy( gasLimit, accumulatableReports, slot, entropy, statistics, AccumulationStateUpdate.empty(), autoAccumulateServices, yieldedRoots, legacyTransfers, ); @@ - pendingTransfers: legacyTransfers, + pendingTransfers: Compatibility.isGreaterOrEqual(GpVersion.V0_7_1) ? pendingTransfers : legacyTransfers,(Be sure to update both sequential helpers so the ≥0.7.1 path returns the accumulated transfers.)
♻️ Duplicate comments (1)
packages/jam/transition/accumulate/accumulation-result-merge-utils.ts (1)
200-208: Don’t drop preimages added by other services.Overwriting
outputState.services.preimageswith onlycurrentServiceUpdatesdiscards entries already queued for this service by other providers (or frominputState), so the hand-off vanishes as soon as we merge the recipient’s own result. Merge with the existing array instead of replacing it.- outputState.services.preimages.set(serviceId, currentServiceUpdates); + if (currentServiceUpdates.length > 0) { + const existing = outputState.services.preimages.get(serviceId) ?? []; + outputState.services.preimages.set(serviceId, [...existing, ...currentServiceUpdates]); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jam/transition/accumulate/accumulate.ts(14 hunks)packages/jam/transition/accumulate/accumulation-result-merge-utils.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/transition/accumulate/accumulation-result-merge-utils.tspackages/jam/transition/accumulate/accumulate.ts
🧠 Learnings (2)
📚 Learning: 2025-06-10T12:10:10.532Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 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/accumulation-result-merge-utils.tspackages/jam/transition/accumulate/accumulate.ts
📚 Learning: 2025-06-10T12:20:17.513Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 419
File: packages/jam/state/state-inmemory.ts:141-149
Timestamp: 2025-06-10T12:20:17.513Z
Learning: In the `InMemoryService.copyFrom` function in `packages/jam/state/state-inmemory.ts`, duplicate checking for `(hash, length)` pairs in the lookup history is not necessary because the function operates under the assumption that the input `ServiceEntries` comes from an existing well-formed state, which already maintains the invariant of unique lookup history entries per hash and length combination.
Applied to files:
packages/jam/transition/accumulate/accumulate.ts
🧬 Code graph analysis (2)
packages/jam/transition/accumulate/accumulation-result-merge-utils.ts (8)
packages/jam/config/chain-spec.ts (1)
ChainSpec(41-106)packages/jam/transition/accumulate/accumulate-state.ts (1)
AccumulateState(23-32)packages/jam/jam-host-calls/externalities/state-update.ts (1)
AccumulationStateUpdate(51-138)packages/jam/block/common.ts (2)
ServiceGas(31-31)tryAsServiceGas(32-32)packages/jam/jam-host-calls/externalities/pending-transfer.ts (1)
PendingTransfer(12-37)packages/jam/state/privileged-services.ts (1)
PrivilegedServices(10-48)packages/jam/state/common.ts (1)
tryAsPerCore(10-16)packages/core/numbers/index.ts (1)
sumU64(93-104)
packages/jam/transition/accumulate/accumulate.ts (5)
packages/jam/jam-host-calls/externalities/state-update.ts (1)
AccumulationStateUpdate(51-138)packages/jam/block/common.ts (5)
ServiceGas(31-31)tryAsServiceGas(32-32)ServiceId(26-26)TimeSlot(16-16)EntropyHash(40-40)packages/jam/transition/accumulate/accumulation-result-merge-utils.ts (2)
mergePerallelAccumulationResults(9-27)ParallelAccumulationResult(33-33)packages/jam/transition/statistics.ts (1)
CountAndGasUsed(51-54)packages/jam/transition/accumulate/accumulate-data.ts (1)
AccumulateData(26-215)
⏰ 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). (6)
- GitHub Check: run (22.x)
- GitHub Check: test (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: e2e (22.x)
- GitHub Check: state_transition (22.x)
View all
Benchmarks summary: 63/63 OK ✅ |
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
lgtm! Can't approve, because it's my PR 😅
|
@mateuszsikora please address the comments and add any other stuff that you wanted in a separate PR. |
