Add tickets mode to block generator#923
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an optional async ticket validation hook to TicketDistributionTask via setOnTicketReceived so incoming tickets are conditionally enqueued; validation failures or errors drop tickets with a warning. addTicket now ignores tickets from past epochs and only clears pending tickets when the epoch advances. Block-authoring accepts verified pending tickets and prepares ticket extrinsics by filtering accumulator entries, sorting by entropy ID, deduplicating, and capping by chain spec; tickets are included only during the contest period. A per-ticket ReceivedTicketMessage and a fromWorker.receivedTickets handler were added, and tests were updated/added for ticket flows and generator ticket selection. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@fluffylabs-bot benchmark |
Contributor
|
✅ Benchmark workflow triggered successfully! 🎉 🔗 Check the Actions tab for workflow progress. |
Contributor
Picofuzz Benchmark Resultsfallback
safrole
storage
storage_light
🤖 Automated benchmark |
![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: 1
🧹 Nitpick comments (1)
packages/workers/block-authorship/main.ts (1)
310-323: Consider adding a defensive length check on verification results.The code assumes
results.length === tickets.length. IfbandersnatchVrf.verifyTicketsreturns fewer results than expected (e.g., due to a bug or early termination), accessingresults[i]could returnundefined, causing silent filtering of all tickets or runtime errors.💡 Suggestion to add defensive check
async function verifyAndAddToPool(epochIndex: number, tickets: SignedTicket[], state: State): Promise<boolean> { const results = await bandersnatchVrf.verifyTickets( bandersnatch, state.designatedValidatorData.length, state.epochRoot, tickets, getTicketEntropy(epochIndex, state), ); + if (results.length !== tickets.length) { + logger.warn`verifyTickets returned ${results.length} results for ${tickets.length} tickets`; + return false; + } const verified = tickets .map((ticket, i) => ({ ticket, id: results[i].entropyHash })) .filter((_, i) => results[i].isValid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workers/block-authorship/main.ts` around lines 310 - 323, The verifyAndAddToPool function assumes bandersnatchVrf.verifyTickets returns a result per ticket; add a defensive check after calling bandersnatchVrf.verifyTickets to ensure results.length === tickets.length and handle mismatches (e.g., log an error via the existing logger, avoid indexing into undefined, and return false or throw) before mapping/filtering; update references to tickets, results, addToPool and verifyAndAddToPool so you only call addToPool when the verification results align with the tickets and no undefined entries will be accessed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/jam/jamnp-s/tasks/ticket-distribution.ts`:
- Around line 160-168: The current call to
this.onTicketReceivedCallback(epochIndex, ticket).then(...) lacks a rejection
handler and can cause unhandled promise rejections; update the handling for
onTicketReceivedCallback in ticket-distribution.ts so any thrown error or
rejected promise is caught (use Promise.resolve(...) if necessary) and in the
.catch() log a clear message via logger.warn or logger.error including
epochIndex, ticket id/summary and the error, and ensure the ticket is treated as
dropped (i.e., do not call this.addTicket on errors).
---
Nitpick comments:
In `@packages/workers/block-authorship/main.ts`:
- Around line 310-323: The verifyAndAddToPool function assumes
bandersnatchVrf.verifyTickets returns a result per ticket; add a defensive check
after calling bandersnatchVrf.verifyTickets to ensure results.length ===
tickets.length and handle mismatches (e.g., log an error via the existing
logger, avoid indexing into undefined, and return false or throw) before
mapping/filtering; update references to tickets, results, addToPool and
verifyAndAddToPool so you only call addToPool when the verification results
align with the tickets and no undefined entries will be accessed.
🪄 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: 4e029d37-f68c-4015-9127-2bf9e7ae3a2d
📒 Files selected for processing (8)
packages/jam/jamnp-s/tasks/ticket-distribution.test.tspackages/jam/jamnp-s/tasks/ticket-distribution.tspackages/workers/block-authorship/generator.test.tspackages/workers/block-authorship/generator.tspackages/workers/block-authorship/main.tspackages/workers/comms-authorship-network/protocol.tspackages/workers/comms-authorship-network/tickets-message.tspackages/workers/jam-network/main.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jam/jamnp-s/tasks/ticket-distribution.ts (1)
162-172:⚠️ Potential issue | 🟠 MajorOne error-handling gap remains for synchronous validator failures.
The new
.catch()covers rejected promises, but a validator that throws before returning one still escapesonTicketReceived(). Wrap the invocation inPromise.resolve().then(...)or anasynchelper so both failure modes go through the same drop/log path.🛠️ Suggested change
if (this.onTicketReceivedCallback !== null) { + const onTicketReceivedCallback = this.onTicketReceivedCallback; // Validate first; only redistribute if valid to avoid spreading tampered tickets. - this.onTicketReceivedCallback(epochIndex, ticket) + void Promise.resolve() + .then(() => onTicketReceivedCallback(epochIndex, ticket)) .then((isValid) => { if (isValid) { this.addTicket(epochIndex, ticket); } else { logger.warn`Dropping invalid ticket for epoch ${epochIndex} (validation failed)`;#!/bin/bash node <<'NODE' const cb = () => { throw new Error("sync boom"); }; try { cb().catch(() => console.log("never reached")); } catch (error) { console.log("escaped:", error.message); } Promise.resolve() .then(() => cb()) .catch((error) => console.log("normalized:", error.message)); NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/jamnp-s/tasks/ticket-distribution.ts` around lines 162 - 172, The call to onTicketReceivedCallback can throw synchronously and currently escapes the promise catch; wrap the invocation so both sync throws and returned rejections are handled: replace the direct this.onTicketReceivedCallback(epochIndex, ticket).then(...).catch(...) pattern with a Promise.resolve().then(() => this.onTicketReceivedCallback(epochIndex, ticket)).then((isValid) => { if (isValid) this.addTicket(epochIndex, ticket); else logger.warn`Dropping invalid ticket for epoch ${epochIndex} (validation failed)`; }).catch((error) => logger.error`Error validating ticket for epoch ${epochIndex}, attempt ${ticket.attempt}: ${error}`) (or equivalently call an async helper that awaits onTicketReceivedCallback and uses the same then/catch handlers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/jam/jamnp-s/tasks/ticket-distribution.ts`:
- Around line 162-166: A late validator completion can reset state by calling
addTicket(epochIndex, ticket) with an older epoch; before invoking addTicket
from onTicketReceivedCallback's then branch, check the epochIndex against the
currentEpoch and drop/ignore any result older than this.currentEpoch (or
alternatively make addTicket monotonic by having it only advance or equal epochs
and never rollback currentEpoch or clear newer queues when epochIndex <
currentEpoch); update the logic around onTicketReceivedCallback, addTicket and
currentEpoch to enforce this monotonicity so delayed successes do not wipe newer
epoch state.
---
Duplicate comments:
In `@packages/jam/jamnp-s/tasks/ticket-distribution.ts`:
- Around line 162-172: The call to onTicketReceivedCallback can throw
synchronously and currently escapes the promise catch; wrap the invocation so
both sync throws and returned rejections are handled: replace the direct
this.onTicketReceivedCallback(epochIndex, ticket).then(...).catch(...) pattern
with a Promise.resolve().then(() => this.onTicketReceivedCallback(epochIndex,
ticket)).then((isValid) => { if (isValid) this.addTicket(epochIndex, ticket);
else logger.warn`Dropping invalid ticket for epoch ${epochIndex} (validation
failed)`; }).catch((error) => logger.error`Error validating ticket for epoch
${epochIndex}, attempt ${ticket.attempt}: ${error}`) (or equivalently call an
async helper that awaits onTicketReceivedCallback and uses the same then/catch
handlers).
🪄 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: fb50a407-7a68-47fd-9e32-91b6131d2b08
📒 Files selected for processing (2)
packages/jam/jamnp-s/tasks/ticket-distribution.tspackages/workers/block-authorship/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workers/block-authorship/main.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/jam/jamnp-s/tasks/ticket-distribution.ts`:
- Around line 167-175: Prevent duplicate concurrent validations by tracking an
in-flight key composed of epochIndex + ticket signature before calling
onTicketReceivedCallback: in the on-ticket handling path where you currently
call Promise.resolve().then(() => cb(epochIndex, ticket)), add a short-circuit
check using a per-instance Map/Set keyed by `${epochIndex}:${ticket.signature}`
(or equivalent unique signature field) to see if validation is already pending;
if present, skip calling this.onTicketReceivedCallback and return early,
otherwise insert the key, call cb(...), and ensure you remove the in-flight key
in both the .then and .catch (or finally) branches; leave addTicket(epochIndex,
ticket) as the commit step after the validator returns true.
🪄 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: 9d435924-2439-465e-a8b8-9d20f2e3f370
📒 Files selected for processing (1)
packages/jam/jamnp-s/tasks/ticket-distribution.ts
tomusdrw
reviewed
Mar 30, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/workers/block-authorship/main.ts`:
- Around line 425-430: The code only calls buildTicketAuthorshipCache(...)
inside the isNewEpoch branch so a worker that started mid-epoch with no prebuilt
cache (ticketAuthorshipCache === null) will never build it; modify the logic in
main.ts near the isNewEpoch block to also call await
buildTicketAuthorshipCache(selingKeySeriesResult.ok, state.entropy[2]) when
ticketAuthorshipCache === null (i.e., after startup when prebuild was skipped)
so the cache is initialized immediately; reference the ticketAuthorshipCache
variable and the buildTicketAuthorshipCache(...) function and ensure the call is
awaited exactly once when either isNewEpoch is true or ticketAuthorshipCache ===
null.
- Around line 108-117: The eager cache build (call to
buildTicketAuthorshipCache) runs before the ticketAuthorshipCache variable is
declared, causing a TDZ crash; move the cache-building call so it happens after
ticketAuthorshipCache is initialized (or alternatively hoist the
ticketAuthorshipCache declaration above this initialState block). Specifically,
in the initialState handling where you call getSealingKeySeries and
logEpochBlockCreation (symbols: initialState, getSealingKeySeries, initialKeys,
logEpochBlockCreation, buildTicketAuthorshipCache), ensure ticketAuthorshipCache
is assigned/declared before invoking buildTicketAuthorshipCache (or relocate the
await buildTicketAuthorshipCache(...) to just after the ticketAuthorshipCache
initialization).
🪄 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: 575ba6ba-a9a4-42e9-a452-49e28a605f6f
📒 Files selected for processing (2)
packages/jam/jamnp-s/tasks/ticket-distribution.tspackages/workers/block-authorship/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jam/jamnp-s/tasks/ticket-distribution.ts
tomusdrw
approved these changes
May 15, 2026
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
2 participants
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.
No description provided.