Fix invalid signature traces#846
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe PR threads per-validator data (currentValidatorData and previousValidatorData) through the reports transition pipeline by converting formerly internal Reports state into explicit input parameters and updating types and call sites. Reports.getGuarantorAssignment and verifyCredentials signatures changed to accept the two validator-data arguments; ReportsState exposure was narrowed. Tests and test utilities were updated to provide the new fields via tryAsPerValidator. The conformance test runner had nine entries removed from its ignored list (exposing those vectors) and the global test-runner timeout was increased from 20 to 25 minutes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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). (5)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/test-runner/jam-conformance-072.ts (1)
14-15: Clarify the uncertainty in the comment.The question mark added to this comment suggests uncertainty about whether the block should be rejected. If the expected behavior is unclear, this should be investigated and documented. Consider either:
- Removing the test case from the ignored list if it should pass
- Documenting why the behavior is uncertain
- Filing an issue to investigate the expected behavior
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bin/test-runner/jam-conformance-072.tsbin/test-runner/w3f/reports.tspackages/jam/transition/chain-stf.tspackages/jam/transition/reports/index.test.tspackages/jam/transition/reports/input.tspackages/jam/transition/reports/reports.tspackages/jam/transition/reports/test.utils.tspackages/jam/transition/reports/verify-contextual.test.tspackages/jam/transition/reports/verify-credentials.test.ts
💤 Files with no reviewable changes (1)
- packages/jam/transition/reports/test.utils.ts
🧰 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/reports/input.tspackages/jam/transition/chain-stf.tspackages/jam/transition/reports/verify-credentials.test.tsbin/test-runner/jam-conformance-072.tspackages/jam/transition/reports/verify-contextual.test.tsbin/test-runner/w3f/reports.tspackages/jam/transition/reports/reports.tspackages/jam/transition/reports/index.test.ts
🧠 Learnings (3)
📚 Learning: 2025-06-18T20:35:13.536Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 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/transition/reports/verify-credentials.test.tspackages/jam/transition/reports/verify-contextual.test.ts
📚 Learning: 2025-06-18T20:35:13.536Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 442
File: packages/core/pvm-debugger-adapter/index.ts:22-40
Timestamp: 2025-06-18T20:35:13.536Z
Learning: The `measure` function in `typeberry/utils/debug.ts` attempts environment detection by checking `process === undefined` but still causes bundling issues because bundlers see the `process.hrtime` reference in the Node.js branch.
Applied to files:
packages/jam/transition/reports/verify-credentials.test.tspackages/jam/transition/reports/verify-contextual.test.ts
📚 Learning: 2025-06-10T12:06:32.535Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 419
File: packages/jam/database-lmdb/states.test.ts:158-160
Timestamp: 2025-06-10T12:06:32.535Z
Learning: In test code, tomusdrw prefers pragmatic solutions over strict type safety when listing all entries would be cumbersome. Using `Object.assign({}, state)` for state updates in tests is acceptable even if it compromises type safety, prioritizing test maintainability and readability.
Applied to files:
packages/jam/transition/reports/reports.tspackages/jam/transition/reports/index.test.ts
🧬 Code graph analysis (1)
packages/jam/transition/reports/verify-contextual.test.ts (3)
packages/jam/block/common.ts (1)
tryAsPerValidator(56-62)packages/jam/transition/reports/test.utils.ts (1)
initialValidators(297-323)packages/jam/config/chain-spec.ts (1)
tinyChainSpec(109-126)
⏰ 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). (4)
- GitHub Check: benchmarks (22.x)
- GitHub Check: Setup test vectors cache
- GitHub Check: e2e (22.x)
- GitHub Check: test (22.x)
🔇 Additional comments (17)
packages/jam/transition/chain-stf.ts (1)
280-281: LGTM! Validator data correctly passed from safrole to reports.The validator data fields are properly sourced from the safrole state update (lines 242-243) and passed to the reports transition, aligning with the PR objective to move these fields from state to input.
packages/jam/transition/reports/input.ts (1)
60-67: LGTM! ReportsInput properly extended with validator data fields.The new fields are well-documented with Greek letter references and properly typed using SafroleStateUpdate's type definitions, ensuring type safety and consistency.
packages/jam/transition/reports/verify-contextual.test.ts (3)
2-2: LGTM! Import updated to include tryAsPerValidator.
14-22: LGTM! Test utilities import extended with initialValidators.
43-44: LGTM! Test inputs consistently extended with validator data.All test cases properly populate
currentValidatorDataandpreviousValidatorDatausing the same validator set. This is appropriate for contextual validation tests that don't focus on validator rotation scenarios.Also applies to: 80-81, 114-115, 149-150, 183-184, 219-220, 255-256, 293-294, 333-334, 378-379, 416-417
packages/jam/transition/reports/index.test.ts (2)
2-2: LGTM! Imports updated for validator data support.Also applies to: 9-9
22-23: LGTM! Test input properly extended with validator data.packages/jam/transition/reports/verify-credentials.test.ts (2)
2-2: LGTM! Import updated to include tryAsPerValidator.
42-43: LGTM! All credential verification tests consistently updated.All test cases properly include the required validator data fields, maintaining consistency with the refactoring objectives.
Also applies to: 77-78, 108-109, 139-140, 170-171, 201-202, 232-233
bin/test-runner/w3f/reports.ts (4)
4-4: LGTM! PerValidator type imported for validator data support.
69-69: LGTM! Return type correctly omits validator data fields.The validator data fields are intentionally omitted from the return type since they're added later from the test state, maintaining proper separation of concerns.
114-115: LGTM! Test state properly returns validator data.The test harness correctly separates current and previous validators from the JSON test vectors and converts them to
PerValidatorarrays usingtryAsPerValidator, enabling proper testing of validator rotation scenarios.Also applies to: 137-138
279-283: LGTM! Validator data correctly passed to reports transition.The spread pattern cleanly merges the input with validator data from the pre-state, aligning with the refactoring to move validator data from state to input.
packages/jam/transition/reports/reports.ts (3)
21-25: LGTM! ReportsState correctly updated to remove validator data fields.The validator data fields are properly removed from the state since they're now passed as input parameters, completing the refactoring objective.
141-150: LGTM! Validator data correctly passed to getGuarantorAssignment.The
verifyCredentialsmethod properly forwards the validator data from the input togetGuarantorAssignment, maintaining the data flow for credential verification.
197-255: LGTM! getGuarantorAssignment correctly refactored to accept validator data as parameters.The method signature is properly updated to receive validator data as explicit parameters instead of reading from state. The logic for selecting between current and previous validator data based on rotation is preserved (lines 230, 242), maintaining the correct behavior for different epoch and rotation scenarios.
bin/test-runner/jam-conformance-072.ts (1)
7-28: Unable to verify the claim about un-ignored test vectors. The git history for this file is not accessible in the current repository state (detached HEAD at FETCH_HEAD), and no diff information is available to confirm which entries were removed from the ignored list. Additionally, the test runner cannot be executed in this environment to verify whether previously ignored tests now pass. Manual verification of the actual changes to the ignored list and test execution results is required.
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
View all
Benchmarks summary: 83/83 OK ✅ |
3 participants
Changes:
currentValidatorDataandpreviousValidatorDatamoved from state to input in reportscurrentValidatorDataandpreviousValidatorDatafrom state replaced with corresponding values from safrole state updateit should be merged after: #845