Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
🟡 Changes recommended
The v1 testnet override still allows an effectively reduced verification result to enter the cache.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prevents reduced XDPoS header verification results from being cached as fully verified.
Changes:
- Restricts v1 and v2 cache insertion to full verification.
- Adds a v1 regression test for reduced and full verification caching.
File summaries
| File | Description |
|---|---|
engine_v1/engine.go |
Guards verified-header cache insertion. |
engine_v1/verify_header_test.go |
Tests cache behavior by verification level. |
engine_v2/verifyHeader.go |
Applies the equivalent v2 cache guard. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
85f16c1 to
e3062ec
Compare
…ader cache verifiedHeaders remembers a header as verified, keyed by its hash, and both the single-header and the batch path answer nil from it before running any check. The flag that selects full verification is not part of that key, so a header admitted by a reduced check was remembered as if it had passed all of them, and the next caller to ask for full verification was answered from the cache without looking at the validator signature, the double validation of v1 or the masternode set a v2 epoch-switch block claims for itself. Only remember a header when the full verification passed. Consulting the cache under a reduced check stays correct, because passing the full set covers it.
e3062ec to
a2e8c04
Compare
Summary
XDPoS_v1.verifiedHeadersandXDPoS_v2.verifiedHeadersremember a header as verified, keyed by its hash, and every caller answersnilfrom the cache before running a single check. ThefullVerifyflag that selects how much of the verification runs is not part of that key, so a header admitted by a reduced check was remembered as if it had passed all of them - and the next caller asking for full verification was answered from the cache without ever looking at the validator signature, the double validation of v1, or the masternode set a v2 epoch-switch block claims for itself.Both engines now only remember a header when the full verification passed. Consulting the cache under a reduced check stays correct, because passing the full set covers the reduced one.
Behaviour and risk
shouldDisableFullVerifypath), and the cache is an LRU bounded byutils.InMemorySnapshots.Tests
consensus/XDPoS/engines/engine_v1/verify_header_test.go:TestVerifiedHeadersOnlyRemembersFullVerificationturns onSkipV1Validationso that both levels succeed without a chain to read, which makes the verification level the only difference between the two calls - the reduced call must leave no entry behind, the full one must.Removing the
&& fullVerifyguard makes it fail witha reduced verification must not be remembered as a full one.The v2 side is the symmetric change and has no dedicated test: building a header that passes the whole v2 verification is too expensive for a test of the cache.