Refactor Authorship#1007
Merged
Merged
Conversation
tomusdrw
commented
Jun 10, 2026
tomusdrw
commented
Member
- Introduce dev-full spec
- Generate tickets out of the main thread in a worker pool
- Simplify authorship worker logic and ticket validation.
- Use new batch-generation and batch-validation APIs from native bandersnatch.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a major refactor of block authorship around batch ticket validation and generation. Configuration is split into separate dev-tiny and dev-full variants. Core safrole VRF generation is extended to batch multiple validators in a single native call, with new helper functions for input/output handling. Ticket validation contracts are updated to process arrays of tickets and return arrays of validated results. A new worker-pool system parallelizes ticket generation across CPU cores using sharded batch execution. Two new modules—EpochAuthoringSlots and EpochTracker—encapsulate epoch boundary detection and local validator slot assignment. The Generator class is renamed to BlockGenerator. The main authorship loop is significantly rewritten to coordinate epoch transitions, verified ticket pool synchronization, and slot-driven block creation using the new components. Supporting changes include protocol messaging updates to batch tickets, logger refactoring in the importer module, and utility enhancements for memory reporting and hash truncation. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/jam/args.ts (1)
31-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
--configexamples with the renamed dev preset.Line 31 documents predefined presets using
DEV_TINY_CONFIG, but Lines 33-37 still show--config=dev. This can mislead users copying examples from--help.Suggested diff
- Example: --${ARGS.CONFIG}=dev --${ARGS.CONFIG}=.chain_spec+={"bootnodes": []} -- will modify only the bootnodes property of the chain spec (merge). - Example: --${ARGS.CONFIG}=dev --${ARGS.CONFIG}=.chain_spec={"bootnodes": []} -- will replace the entire chain spec property with the provided JSON object. - Example: --${ARGS.CONFIG}=dev --${ARGS.CONFIG}=.chain_spec+=bootnodes.json -- you may also use JSON files in your queries. This one will merge the contents of bootnodes.json onto the chain spec. - Example: --${ARGS.CONFIG}=dev --${ARGS.CONFIG}={"chain_spec": { "bootnodes": [] }} -- will merge the provided JSON object onto the "dev" config. - Example: --${ARGS.CONFIG}=dev --${ARGS.CONFIG}=bootnodes.json -- will merge the contents of bootnodes.json onto the "dev" config. + Example: --${ARGS.CONFIG}=${DEV_TINY_CONFIG} --${ARGS.CONFIG}=.chain_spec+={"bootnodes": []} -- will modify only the bootnodes property of the chain spec (merge). + Example: --${ARGS.CONFIG}=${DEV_TINY_CONFIG} --${ARGS.CONFIG}=.chain_spec={"bootnodes": []} -- will replace the entire chain spec property with the provided JSON object. + Example: --${ARGS.CONFIG}=${DEV_TINY_CONFIG} --${ARGS.CONFIG}=.chain_spec+=bootnodes.json -- you may also use JSON files in your queries. This one will merge the contents of bootnodes.json onto the chain spec. + Example: --${ARGS.CONFIG}=${DEV_TINY_CONFIG} --${ARGS.CONFIG}={"chain_spec": { "bootnodes": [] }} -- will merge the provided JSON object onto the "${DEV_TINY_CONFIG}" config. + Example: --${ARGS.CONFIG}=${DEV_TINY_CONFIG} --${ARGS.CONFIG}=bootnodes.json -- will merge the contents of bootnodes.json onto the "${DEV_TINY_CONFIG}" config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/jam/args.ts` around lines 31 - 37, Update the help text examples to use the renamed preset symbol instead of the old "dev" literal: replace occurrences of "--${ARGS.CONFIG}=dev" in the examples with "--${ARGS.CONFIG}=${DEV_TINY_CONFIG}" so the documented presets match the predefined presets list (['${DEV_TINY_CONFIG}', '${DEFAULT_CONFIG}']) declared in the same block; adjust all example lines that chain with ".chain_spec", JSON objects, or files (the lines showing .chain_spec+={...}, .chain_spec={...}, .chain_spec+=bootnodes.json, {"chain_spec": ...}, and bootnodes.json) to use the ${DEV_TINY_CONFIG} preset.packages/jam/ticket-pool/ticket-validator.ts (1)
10-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale documentation: comment references removed
nullcase.The comment states
idis "nullwhen the concrete validator doesn't actually verify", but line 16 changed the type fromEntropyHash | nulltoEntropyHash. Update the comment to reflect that non-verifying validators now return a zeroed hash (e.g.,Bytes.zero(HASH_SIZE).asOpaque()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jam/ticket-pool/ticket-validator.ts` around lines 10 - 12, The doc comment for the `id` field is stale: it still claims `id` can be `null` when a concrete validator doesn't verify (e.g., `AcceptTicketsValidator`), but the type was changed to `EntropyHash`; update the comment to state that non-verifying validators return a zeroed hash instead (for example `Bytes.zero(HASH_SIZE).asOpaque()`), and mention `EntropyHash` and `AcceptTicketsValidator` explicitly so readers know the convention.
🧹 Nitpick comments (4)
packages/jam/config-node/node-config.test.ts (1)
77-210: ⚡ Quick winAdd coverage for the new
dev-fullselector path.
loadConfignow has dedicateddev-fullbehavior, but this suite only validates thedev/devTinypath. Please add at least one assertion forloadConfig(["dev-full"], ...)(and ideally one stacked-order case).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jam/config-node/node-config.test.ts` around lines 77 - 210, Add tests covering the new "dev-full" selector: call loadConfig(["dev-full"], withRelPath) and assert deepStrictEqual against parseFromJson(configs.devFull, NodeConfiguration.fromJson) to verify the dedicated behavior; also add one stacked-order case (e.g., loadConfig(["dev-full", `.database_base_path="/override"`], withRelPath) and assert the result merges configs.devFull with the override) to ensure layering works. Use the same test patterns and helpers (loadConfig, parseFromJson, NodeConfiguration.fromJson, configs) as the existing cases.bin/jam/helpers/generate-full-genesis.ts (2)
74-76: ⚡ Quick winAvoid exception-driven control flow in
main().The
throwinmain()and terminal.catch(...process.exit(1))can be replaced with explicit failure return handling (typed result / exit code path) to align with project error-handling rules.Suggested refactor
async function main() { + let failed = false; await initWasm(); @@ if (ringRootResult.isError) { - throw new Error(`Failed to compute ring commitment: ${ringRootResult.error}`); + console.error("Failed to compute ring commitment"); + failed = true; + return failed; } @@ process.stdout.write(JSON.stringify(out, null, 2)); process.stdout.write("\n"); + return failed; } @@ -main().catch((err) => { - console.error("Error:", err); - process.exit(1); -}); +main().then((failed) => { + if (failed) process.exit(1); +});As per coding guidelines, “Use
Result/tagged unions for errors; avoid exceptions for control flow.”Also applies to: 158-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/jam/helpers/generate-full-genesis.ts` around lines 74 - 76, Replace the throw-driven failure in main() with explicit Result/error-return handling: when ringRootResult.isError (and similarly the other error checks around the main flow at 158-161), do not throw — instead construct and return a failure Result (or set a dedicated exit/error value from main) and propagate that to the top-level caller so the process exit is handled once centrally; update the function signature of main (or its caller) to return a typed Result/exit code and convert existing `throw new Error(...)` at the ringRootResult check (and the corresponding checks later) into returning the error Result with a clear message referencing ringRootResult.error.Source: Coding guidelines
1-2: ⚡ Quick winUse runtime-specific filename for this Node-only script.
This file is Node-specific (
tsxshebang +processAPIs), so it should follow the runtime naming convention (for example,generate-full-genesis.node.ts).As per coding guidelines, “Follow file conventions:
*.node.ts/*.web.tsfor runtime-specific code; otherwise keep files TS-compatible with both node+web.”Also applies to: 132-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/jam/helpers/generate-full-genesis.ts` around lines 1 - 2, This Node-only script uses a tsx shebang and Node process APIs so it must follow runtime naming conventions; rename the file from generate-full-genesis.ts to generate-full-genesis.node.ts and update any imports, package.json/bin entries, and CI/dev scripts that reference the old name to the new one; ensure other Node-specific segments in the same area are similarly renamed (the ones flagged in the review) so all runtime-specific files end with .node.ts to reflect Node-only usage.Source: Coding guidelines
packages/jam/safrole/bandersnatch-vrf.test.ts (1)
303-323: ⚡ Quick winAdd a true multi-validator case here.
This still exercises a 1-validator batch, so an ordering bug between
batchGenerateRingVrfForValidators,parseTicketsBatchOutput, and the downstream flattening would still pass. Please add at least a 2-validator case and assert both the outer batch length and each validator's ticket count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jam/safrole/bandersnatch-vrf.test.ts` around lines 303 - 323, The test only covers a single-validator batch so ordering/flattening bugs in batchGenerateRingVrfForValidators / parseTicketsBatchOutput can be missed; add a true multi-validator test using bandersnatchVrf.generateTickets with at least two prover indices (e.g. pass [0,1] and corresponding secrets) and entropy, then assert genResult.isOk, assert genResult.ok.length === 2 (outer batch length), and for each validator entry assert the per-validator ticket count (e.g. genResult.ok[i].length matches expected); also exercise verifyTickets (or the appropriate per-validator verifier) for each validator using getRingCommitment and ensure each verifyResult.isOk is true. Ensure you reference generateTickets, getRingCommitment, verifyTickets, batchGenerateRingVrfForValidators, and parseTicketsBatchOutput when locating code to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/jam/safrole/bandersnatch-vrf.ts`:
- Around line 242-259: Reject ticketsPerValidator values >= 256 before calling
the wasm routine to prevent uint8 wrapping: add a precondition check in the
caller (the function around buildTicketVrfInputs / the call to
bandersnatch.batchGenerateRingVrfForValidators) and the exported helper that
uses it to return Result.error(...) if ticketsPerValidator > 255 (or < 1 if you
want to forbid zero attempts) with an explanatory message; reference
buildTicketVrfInputs, ticketsPerValidator, parseTicketsBatchOutput and
bandersnatch.batchGenerateRingVrfForValidators so the guard lives immediately
before buildTicketVrfInputs is invoked and the same validation is applied in the
exported helper to keep generation and parsing consistent.
In `@packages/workers/block-authorship/epoch-tracker.ts`:
- Around line 37-41: The current code throws a placeholder Error("xx") when
sealingKeySeriesResult.isError, which discards the typed failure from
Safrole.getSealingKeySeries; change getEpochData() to return a Result/union type
instead of throwing, and when sealingKeySeriesResult.isError immediately
propagate/return that Result failure (including the original error payload)
rather than throwing; update callers (notably main) to inspect the returned
Result and decide retry/skip/terminate with the real error details. Use the
existing sealingKeySeriesResult, getSealingKeySeries, getEpochData, and main
symbols to locate and implement the change so errors are propagated as Results
and not swallowed by an exception.
In `@packages/workers/block-authorship/main.ts`:
- Around line 123-140: The ticket pool is being keyed and published using
epochData.epoch (current epoch) but must use the next epoch (epochData.epoch +
1) because tickets generated during epoch E are consumed for sealing in epoch
E+1; update all usages that index or publish the pool—specifically change the
key passed to verifiedPool.getForEpoch, the epochIndex sent to
networkingComms.sendReplaceTicketPool, and the epoch value passed into
ticketGenerator.generateTickets/onEpochTickets to use the next epoch (e.g.,
compute nextEpoch = epochData.epoch + 1 and use that) so the locally generated
tickets are stored/published under E+1; also add tryAsEpoch to the
`@typeberry/block` import and ensure any BandersnatchTicketValidator logic that
checks state.entropy[1] remains consistent with this shift.
- Around line 257-258: The code currently sleeps a fixed this.slotDurationMs
after work, which accumulates drift; instead compute the remaining milliseconds
until the next slot boundary and sleep that interval. Locate the sleep call that
uses await setTimeout(Number(this.slotDurationMs)) and replace it with logic
that reads the current time (e.g., Date.now() or
getCurrentTimeSlot()/this.slotDurationMs), computes nextSlotStart = ceil(now /
this.slotDurationMs) * this.slotDurationMs, computes delay = Math.max(0,
nextSlotStart - now), and await setTimeout(delay) so wakeups land on the next
slot boundary rather than slotDurationMs from "now".
- Around line 153-156: The branch where validatorIndex === null currently
continues immediately and causes a tight spin; change it to await a slot-wait
before continuing so the loop yields until state.timeslot/newTimeSlot advances.
Specifically, in the loop around validatorIndex, replace the immediate continue
with an await that sleeps until the next timeslot (e.g., await
waitForNextSlot(state.timeslot) or await sleepUntil(newTimeSlot)) so the same
state.timeslot won't be rechecked in a hot loop; keep the existing log line
(logger.log`${logPrefix} Not currently validator, yet ${currentSlot.logId} is
present.`) and then perform the asynchronous wait before continuing. Ensure the
helper you call is non-blocking (returns a Promise) and uses
state.timeslot/newTimeSlot to determine when to resume.
In `@packages/workers/block-authorship/ticket-validator.ts`:
- Around line 28-42: Short-circuit epoch validation in validate(...) before
calling bandersnatchVrf.verifyTickets: derive the allowed epoch range from the
validator state (e.g., using state.currentEpoch and whatever epoch/window fields
the state exposes) and explicitly check that epochIndex falls inside that
window; if it is outside, return a typed ValidationError immediately instead of
invoking bandersnatchVrf.verifyTickets(this.bandersnatch, ...). Update validate
(and any helpers like getTicketEntropy if relied on) to perform this explicit
check and return the failure path so we avoid the expensive proof verification
on obviously invalid epochs.
In `@packages/workers/importer/stats.ts`:
- Around line 79-80: The memoryTracker output is being logged twice back-to-back
via logger.info and logger.trace using this.memory (whose toString() is
stateful), causing redundant/identical entries; fix by either removing one of
the duplicate calls (e.g., drop the logger.trace`...` line) or, if you truly
need both levels, call this.memory.toString() once into a local variable and log
that variable to logger.info and logger.trace so the value is identical without
advancing state; update the lines that reference this.memory (logger.info`...`
and logger.trace`...`) and add a brief comment if keeping both for visibility.
---
Outside diff comments:
In `@bin/jam/args.ts`:
- Around line 31-37: Update the help text examples to use the renamed preset
symbol instead of the old "dev" literal: replace occurrences of
"--${ARGS.CONFIG}=dev" in the examples with
"--${ARGS.CONFIG}=${DEV_TINY_CONFIG}" so the documented presets match the
predefined presets list (['${DEV_TINY_CONFIG}', '${DEFAULT_CONFIG}']) declared
in the same block; adjust all example lines that chain with ".chain_spec", JSON
objects, or files (the lines showing .chain_spec+={...}, .chain_spec={...},
.chain_spec+=bootnodes.json, {"chain_spec": ...}, and bootnodes.json) to use the
${DEV_TINY_CONFIG} preset.
In `@packages/jam/ticket-pool/ticket-validator.ts`:
- Around line 10-12: The doc comment for the `id` field is stale: it still
claims `id` can be `null` when a concrete validator doesn't verify (e.g.,
`AcceptTicketsValidator`), but the type was changed to `EntropyHash`; update the
comment to state that non-verifying validators return a zeroed hash instead (for
example `Bytes.zero(HASH_SIZE).asOpaque()`), and mention `EntropyHash` and
`AcceptTicketsValidator` explicitly so readers know the convention.
---
Nitpick comments:
In `@bin/jam/helpers/generate-full-genesis.ts`:
- Around line 74-76: Replace the throw-driven failure in main() with explicit
Result/error-return handling: when ringRootResult.isError (and similarly the
other error checks around the main flow at 158-161), do not throw — instead
construct and return a failure Result (or set a dedicated exit/error value from
main) and propagate that to the top-level caller so the process exit is handled
once centrally; update the function signature of main (or its caller) to return
a typed Result/exit code and convert existing `throw new Error(...)` at the
ringRootResult check (and the corresponding checks later) into returning the
error Result with a clear message referencing ringRootResult.error.
- Around line 1-2: This Node-only script uses a tsx shebang and Node process
APIs so it must follow runtime naming conventions; rename the file from
generate-full-genesis.ts to generate-full-genesis.node.ts and update any
imports, package.json/bin entries, and CI/dev scripts that reference the old
name to the new one; ensure other Node-specific segments in the same area are
similarly renamed (the ones flagged in the review) so all runtime-specific files
end with .node.ts to reflect Node-only usage.
In `@packages/jam/config-node/node-config.test.ts`:
- Around line 77-210: Add tests covering the new "dev-full" selector: call
loadConfig(["dev-full"], withRelPath) and assert deepStrictEqual against
parseFromJson(configs.devFull, NodeConfiguration.fromJson) to verify the
dedicated behavior; also add one stacked-order case (e.g.,
loadConfig(["dev-full", `.database_base_path="/override"`], withRelPath) and
assert the result merges configs.devFull with the override) to ensure layering
works. Use the same test patterns and helpers (loadConfig, parseFromJson,
NodeConfiguration.fromJson, configs) as the existing cases.
In `@packages/jam/safrole/bandersnatch-vrf.test.ts`:
- Around line 303-323: The test only covers a single-validator batch so
ordering/flattening bugs in batchGenerateRingVrfForValidators /
parseTicketsBatchOutput can be missed; add a true multi-validator test using
bandersnatchVrf.generateTickets with at least two prover indices (e.g. pass
[0,1] and corresponding secrets) and entropy, then assert genResult.isOk, assert
genResult.ok.length === 2 (outer batch length), and for each validator entry
assert the per-validator ticket count (e.g. genResult.ok[i].length matches
expected); also exercise verifyTickets (or the appropriate per-validator
verifier) for each validator using getRingCommitment and ensure each
verifyResult.isOk is true. Ensure you reference generateTickets,
getRingCommitment, verifyTickets, batchGenerateRingVrfForValidators, and
parseTicketsBatchOutput when locating code to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab6e7d15-c9bd-4419-800c-03c33f5c8cbf
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
bin/jam/args.tsbin/jam/helpers/generate-full-genesis.tsbin/jam/helpers/tiny-network.tsbin/jam/package.jsonbiome.jsoncpackages/configs/index.tspackages/configs/typeberry-dev-full.jsonpackages/configs/typeberry-dev-tiny.jsonpackages/core/bytes/bytes.tspackages/core/utils/debug.test.tspackages/core/utils/debug.tspackages/jam/config-node/node-config.test.tspackages/jam/config-node/node-config.tspackages/jam/jamnp-s/tasks/ticket-distribution.tspackages/jam/safrole/bandersnatch-vrf.test.tspackages/jam/safrole/bandersnatch-vrf.tspackages/jam/safrole/bandersnatch-wasm.tspackages/jam/ticket-pool/ticket-validator.test.tspackages/jam/ticket-pool/ticket-validator.tspackages/jam/ticket-pool/verified-ticket-pool.test.tspackages/jam/ticket-pool/verified-ticket-pool.tspackages/workers/block-authorship/block-generator.test.tspackages/workers/block-authorship/block-generator.tspackages/workers/block-authorship/bootstrap-authorship.mjspackages/workers/block-authorship/epoch-authoring-slots.tspackages/workers/block-authorship/epoch-tracker.tspackages/workers/block-authorship/main.tspackages/workers/block-authorship/package.jsonpackages/workers/block-authorship/ticket-generator/bootstrap-main.tspackages/workers/block-authorship/ticket-generator/bootstrap-ticket-generator.mjspackages/workers/block-authorship/ticket-generator/index.tspackages/workers/block-authorship/ticket-generator/protocol.tspackages/workers/block-authorship/ticket-generator/ticket-generator.test.tspackages/workers/block-authorship/ticket-generator/ticket-generator.tspackages/workers/block-authorship/ticket-generator/worker-pool.tspackages/workers/block-authorship/ticket-validator.tspackages/workers/comms-authorship-network/protocol.tspackages/workers/comms-authorship-network/tickets-message.tspackages/workers/importer/importer.tspackages/workers/importer/stats.tspackages/workers/jam-network/main.ts
💤 Files with no reviewable changes (1)
- packages/workers/comms-authorship-network/tickets-message.ts
View all
Benchmarks summary: 83/83 OK ✅ |
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
1 participant
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.