Conversation
|
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
WalkthroughThis change replaces several hard-coded gas/erasure-code constants with values sourced from ChainSpec, introduces typed numeric wrappers, adds EC_SEGMENT_SIZE and derived erasureCodedPieceSize, updates encoding and accumulation logic to use ChainSpec fields, removes unused constants, adjusts tests and statistics to reference EC_SEGMENT_SIZE, and adds a numbers dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Accumulate as Accumulate
participant ChainSpec as ChainSpec
Caller->>Accumulate: getGasLimit(input)
Accumulate->>Accumulate: compute calculatedGasLimit
Accumulate->>ChainSpec: read maxBlockGas
Accumulate->>Accumulate: clamp = min(calculatedGasLimit, maxBlockGas)
Accumulate-->>Caller: gasLimit=clamp
note over Accumulate,ChainSpec: Gas ceiling now sourced from ChainSpec.maxBlockGas
sequenceDiagram
autonumber
actor Runtime
participant FetchExt as fetch-externalities
participant ChainSpec as ChainSpec
Runtime->>FetchExt: getEncodedConstants()
FetchExt->>ChainSpec: read validatorsCount (V)
FetchExt->>ChainSpec: read maxRefineGas (G_R)
FetchExt->>ChainSpec: read maxBlockGas (G_T)
FetchExt->>ChainSpec: read erasureCodedPieceSize (W_E)
FetchExt->>ChainSpec: read numberECPiecesPerSegment (W_P)
FetchExt-->>Runtime: encoded constants blob
note over FetchExt: Static constants replaced by ChainSpec-derived values
sequenceDiagram
autonumber
actor StatsCaller
participant Stats as statistics.calculateDAScoreCore
participant Config as Config
StatsCaller->>Stats: calculateDAScoreCore(workPackages)
Stats->>Config: read EC_SEGMENT_SIZE
loop for each workPackage
Stats->>Stats: sum += length + EC_SEGMENT_SIZE * segmentCount
end
Stats-->>StatsCaller: score
note over Stats,Config: W_G replaced with EC_SEGMENT_SIZE
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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 (1)
441-445: Gas limit “clamp” uses max instead of min (breaks cap at maxBlockGas).This returns the larger of calculatedGasLimit and maxBlockGas; it should cap at maxBlockGas.
Apply:
- const gasLimit = tryAsServiceGas( - this.chainSpec.maxBlockGas > calculatedGasLimit ? this.chainSpec.maxBlockGas : calculatedGasLimit, - ); - - return tryAsServiceGas(gasLimit); + const gasLimit = calculatedGasLimit > this.chainSpec.maxBlockGas + ? this.chainSpec.maxBlockGas + : calculatedGasLimit; + return tryAsServiceGas(gasLimit);packages/jam/config/chain-spec.ts (1)
81-99: Avoid Omit for constructor input; define a dedicated init type.Using
Omit<ChainSpec, ...>on a class is brittle and may accidentally include inherited instance members fromWithDebug. Define an explicit init type and use it in the constructor.Apply this diff within the constructor signature:
- constructor(data: Omit<ChainSpec, "validatorsSuperMajority" | "thirdOfValidators" | "erasureCodedPieceSize">) { + constructor(data: ChainSpecInit) {And add this type near the class (outside this hunk):
export type ChainSpecInit = { validatorsCount: U16; coresCount: U16; slotDuration: U16; epochLength: U32; rotationPeriod: U16; contestLength: U32; ticketsPerValidator: U8; maxTicketsPerExtrinsic: U8; numberECPiecesPerSegment: U32; preimageExpungePeriod: U32; maxBlockGas: U64; maxRefineGas: U64; };
🧹 Nitpick comments (7)
packages/jam/transition/accumulate/accumulate.ts (1)
432-434: JSDoc nit: remove W_G reference and clarify clamp semantics.W_G is gone; prefer “min(calculatedGasLimit, maxBlockGas)”.
- * Please note it cannot overflow because we use `BigInt`, and the final result is clamped to `maxBlockGas` (W_G). + * Uses BigInt math; final result is clamped to maxBlockGas (i.e., min(calculatedGasLimit, maxBlockGas)).packages/jam/transition/statistics.ts (1)
111-115: Update inline comment to match EC_SEGMENT_SIZE.The code uses EC_SEGMENT_SIZE but the note still mentions W_G.
- /** Available work report score can be up to `W_R + W_G * ((W_M * 65) / 64) = 0x00C4_2180` */ + /** Available work report score can be up to `W_R + EC_SEGMENT_SIZE * ceil((W_M * 65) / 64) = 0x00C4_2180` */packages/jam/transition/statistics.test.ts (1)
50-52: Mirror production formula with Math.ceil for clarity.Align the bound with code’s ceil-based segment count.
- assert.strictEqual(isU32(W_R + EC_SEGMENT_SIZE * ((W_M * 65) / 64)), true); + assert.strictEqual(isU32(W_R + EC_SEGMENT_SIZE * Math.ceil((W_M * 65) / 64)), true);packages/jam/config/chain-spec.ts (4)
35-37: Add spec link for EC_SEGMENT_SIZE (W_G).Consider adding a Gray Paper link (same versioning scheme used elsewhere) to document the 4104-octet segment size source.
96-96: Guard divisibility of W_G by W_P for clearer errors.
tryAsU32(EC_SEGMENT_SIZE / W_P)will throw on non-integers but with a generic message. Add an explicit check to surface a precise cause.- this.erasureCodedPieceSize = tryAsU32(EC_SEGMENT_SIZE / data.numberECPiecesPerSegment); + if (EC_SEGMENT_SIZE % data.numberECPiecesPerSegment !== 0) { + throw new Error( + `EC_SEGMENT_SIZE (${EC_SEGMENT_SIZE}) must be divisible by numberECPiecesPerSegment (${data.numberECPiecesPerSegment}).` + ); + } + this.erasureCodedPieceSize = tryAsU32(EC_SEGMENT_SIZE / data.numberECPiecesPerSegment);
114-116: UseisSuite(suite, version)helper instead of two checks.Readability nit: the API supports passing version to
isSuite.- Compatibility.isSuite(TestSuite.JAMDUNA) && Compatibility.is(GpVersion.V0_6_4) ? 6 : 32, + Compatibility.isSuite(TestSuite.JAMDUNA, GpVersion.V0_6_4) ? 6 : 32,
121-124: Update comment: “full” vs “tiny” values no longer mostly copied.Several fields differ (contestLength, tickets, W_P, gas, preimage delay). Adjust the doc to avoid confusion.
- * Please note that only validatorsCount and epochLength are "full", the rest is copied from "tiny". + * Note: multiple fields differ from "tiny" (e.g., contest/tickets/W_P/gas/preimage delay); see values below.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (7)
packages/jam/block/gp-constants.ts(0 hunks)packages/jam/config/chain-spec.ts(3 hunks)packages/jam/config/package.json(1 hunks)packages/jam/transition/accumulate/accumulate.ts(2 hunks)packages/jam/transition/externalities/fetch-externalities.ts(3 hunks)packages/jam/transition/statistics.test.ts(3 hunks)packages/jam/transition/statistics.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/jam/block/gp-constants.ts
🧰 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/statistics.tspackages/jam/transition/accumulate/accumulate.tspackages/jam/transition/externalities/fetch-externalities.tspackages/jam/transition/statistics.test.tspackages/jam/config/chain-spec.ts
🧠 Learnings (1)
📚 Learning: 2025-06-18T20:35:13.536Z
Learnt from: tomusdrw
PR: FluffyLabs/typeberry#442
File: packages/core/pvm-debugger-adapter/index.ts:22-40
Timestamp: 2025-06-18T20:35:13.536Z
Learning: The `typeberry/utils` package has browser compatibility issues due to Node.js-specific code like `measure` function using `process.hrtime()` and `testUtils` importing `node:assert`, causing white screens in browser environments.
Applied to files:
packages/jam/config/chain-spec.ts
🧬 Code graph analysis (4)
packages/jam/transition/statistics.ts (1)
packages/jam/config/chain-spec.ts (1)
EC_SEGMENT_SIZE(36-36)
packages/jam/transition/externalities/fetch-externalities.ts (2)
packages/core/numbers/index.ts (2)
tryAsU64(50-53)tryAsU32(43-44)packages/jam/block/gp-constants.ts (4)
W_A(42-42)W_B(45-45)W_C(48-48)W_M(51-51)
packages/jam/transition/statistics.test.ts (3)
packages/core/numbers/index.ts (1)
isU32(47-47)packages/jam/block/gp-constants.ts (2)
W_R(54-54)W_M(51-51)packages/jam/config/chain-spec.ts (1)
EC_SEGMENT_SIZE(36-36)
packages/jam/config/chain-spec.ts (2)
packages/core/numbers/index.ts (8)
U16(21-21)U32(24-24)U8(18-18)U64(27-27)tryAsU16(37-38)tryAsU32(43-44)tryAsU8(31-32)tryAsU64(50-53)packages/core/utils/compatibility.ts (1)
Compatibility(46-102)
🔇 Additional comments (4)
packages/jam/transition/accumulate/accumulate.ts (1)
133-135: Confirm Graypaper link versions
The following URLs still target v=0.6.7 (lines 133, 156, 214, 227, 237, 245, 274, 337, 410, 434) and v=0.7.0 (line 524). If the PR’s semantic changes require a newer Graypaper revision, update these anchors to the correct version.packages/jam/transition/statistics.test.ts (1)
23-24: Import looks good.Using EC_SEGMENT_SIZE from config matches the move away from W_G.
packages/jam/config/chain-spec.ts (2)
1-1: Good move: use typed numeric wrappers from @typeberry/numbers.This aligns with the guideline to avoid
ascasts and adds range checks at construction time.
9-10: Align Gray Paper links to commit-anchored URLs
Update all Gray Paper reader links in this file to the canonical URLs for the specific commit anchors (no?v=query), as verified against the jam-test-vectors references:
- Validators estimate:
https://graypaper.fluffylabs.dev/#/5f542d7/418800418800- Cores:
https://graypaper.fluffylabs.dev/#/5f542d7/414200414200- Epoch length:
https://graypaper.fluffylabs.dev/#/5f542d7/414800414800- Preimage expunge:
https://graypaper.fluffylabs.dev/#/9a08063/445800445800- Rotation period:
https://graypaper.fluffylabs.dev/#/5f542d7/417f00417f00Apply these updates at lines 9–10, 23–24, 31–32, 53–55, and 69–70.
View all
Benchmarks summary: 63/63 OK ✅ |
2 participants
Source: davxy/jam-conformance#42
And: JamBrains/jam-docs#59