Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change threads ChainSpec through JSON deserializers and ticket-related functions: many formerly static FromJson/exported parsers (e.g., headerFromJson, ticketsExtrinsicFromJson, ticketFromJson, StateTransitionGenesis.fromJson, TicketsOrKeys.fromJson, SafroleTest.fromJson) were converted to functions that accept a ChainSpec and call nested spec-aware constructors. Ticket attempt validation and codecs (tryAsTicketAttempt, ticketAttemptCodec) and generateTickets were updated to require a ChainSpec. Call sites across tests, runners, workers, and state dumping were updated to pass the spec. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jam/safrole/bandersnatch-vrf.ts (1)
209-255:⚠️ Potential issue | 🟠 MajorGuard
ticketsPerValidatorto avoid thrown errors.
With the parameter now a plain number, an oversized value will causetryAsTicketAttemptto throw and bypass theResulterror path. Add an upfront bounds check and return aResult.errorinstead.🔧 Suggested fix
async function generateTickets( bandersnatch: BandernsatchWasm, ringKeys: BandersnatchKey[], proverKeyIndex: number, key: BandersnatchSecretSeed, entropy: EntropyHash, ticketsPerValidator: number, chainSpec: ChainSpec, ): Promise<Result<SignedTicket[], null>> { + if (ticketsPerValidator < 0 || ticketsPerValidator > chainSpec.ticketsPerValidator) { + return Result.error( + null, + () => + `ticketsPerValidator ${ticketsPerValidator} is out of bounds [0, ${chainSpec.ticketsPerValidator}]`, + ); + } // Build VRF inputs: JAM_TICKET_SEAL || entropy || attempt_byte for each attempt const vrfInputParts: Uint8Array[] = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/safrole/bandersnatch-vrf.ts` around lines 209 - 255, Add an upfront guard in generateTickets to validate ticketsPerValidator before generating VRF inputs: call tryAsTicketAttempt(ticketsPerValidator - 1, chainSpec) wrapped in a try/catch (or otherwise validate the max allowed attempt) and if it throws/invalid return Result.error(null, () => `Invalid ticketsPerValidator: ${ticketsPerValidator}`); this prevents tryAsTicketAttempt from throwing inside the loop and ensures failures use the Result.error path instead of throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/jam/safrole/bandersnatch-vrf.ts`:
- Around line 209-255: Add an upfront guard in generateTickets to validate
ticketsPerValidator before generating VRF inputs: call
tryAsTicketAttempt(ticketsPerValidator - 1, chainSpec) wrapped in a try/catch
(or otherwise validate the max allowed attempt) and if it throws/invalid return
Result.error(null, () => `Invalid ticketsPerValidator: ${ticketsPerValidator}`);
this prevents tryAsTicketAttempt from throwing inside the loop and ensures
failures use the Result.error path instead of throwing.
View all
Benchmarks summary: 83/83 OK ✅ |
TicketAttemptwas missing proper validation during decoding (same astryAsTicketAttemptconversion).This PR adds the validation and additionally changes the encoding of ticket attempt from fixed-size U8 to variable-size,
which is what GP specifies.
Note that for valid
TicketAttemptvalues there is no difference, since both fixed-size and variable-size encoding will just result in a singlebyte value, however it does affect some degenerate cases.
Additional context:
davxy/jam-conformance#66