refactor: use ledger block on verify block #1264
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces multi-era block verification support across the Cardano ledger. The core changes rewrite Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="ledger/verify_block_body.go">
<violation number="1" location="ledger/verify_block_body.go:93">
The mismatch error claims to print the derived body hash but actually logs the raw block data, so callers cannot see which hash was computed when verification fails.</violation>
</file>
<file name="ledger/verify_block.go">
<violation number="1" location="ledger/verify_block.go:239">
VerifyBlock now skips VRF, KES, and block-body validation by hard-coding their results to true, so invalid blocks will still be reported as valid.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
wolf31o2
force-pushed
the
refactor/verify-block-interface
branch
from
74f13c7 to
77de2f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/verify_kes.go (1)
128-169: Continuing verification to assess exposure:Now I need to check where
VerifyKesis being called and whetherslotsPerKesPeriodis being validated before use:Guard VerifyKesComponents against slotsPerKesPeriod == 0 and simplify period math
The refactor routing
VerifyKesthroughVerifyKesComponentsis good, but two improvements are needed:
Potential division-by-zero panic
currentKesPeriod := currentSlot / slotsPerKesPeriodwill panic ifslotsPerKesPeriod == 0. Go best practice is to return error values for anticipated errors rather than panic, and sinceVerifyKesComponentsis exported, defensive validation is appropriate.Redundant locals
startOfKesPeriodandcurrentSlotinVerifyKesComponentsare unused copies of input arguments and can be removed for clarity.A concrete fix:
func VerifyKesComponents( - bodyCbor []byte, - signature []byte, - hotVkey []byte, - kesPeriod uint64, - slot uint64, - slotsPerKesPeriod uint64, + bodyCbor []byte, + signature []byte, + hotVkey []byte, + kesPeriod uint64, + slot uint64, + slotsPerKesPeriod uint64, ) (bool, error) { - startOfKesPeriod := kesPeriod - currentSlot := slot - currentKesPeriod := currentSlot / slotsPerKesPeriod + if slotsPerKesPeriod == 0 { + return false, fmt.Errorf("slotsPerKesPeriod must be greater than 0") + } + currentKesPeriod := slot / slotsPerKesPeriod t := uint64(0) - if currentKesPeriod >= startOfKesPeriod { - t = currentKesPeriod - startOfKesPeriod + if currentKesPeriod >= kesPeriod { + t = currentKesPeriod - kesPeriod } return verifySignedKES(hotVkey, t, bodyCbor, signature), nil }This preserves existing semantics for valid inputs while turning a potential misconfiguration into a controlled error.
♻️ Duplicate comments (2)
ledger/verify_block.go (1)
103-280: Validation logic is not implemented.Lines 242-243 hardcode
vrfValidandkesValidtotrue, and Line 276 hardcodesisBodyValidtotrue. This means the function extracts block metadata but performs no actual verification of VRF, KES signatures, or block body integrity. The PR objectives mention introducingVerifyKesComponentsandVerifyVrfhelpers, but they are not called here.ledger/verify_block_body.go (1)
84-90: Error message logs incorrect value for debugging.The error message claims to print the "derived bodyHex" but actually logs the raw input
datainstead of the calculated hash. This prevents debugging when verification fails.Apply this diff to log the calculated hash:
if !bytes.Equal(calculateBlockBodyHashByte[:32], blockBodyHashByte) { return false, fmt.Errorf( "body hash mismatch, derived bodyHex: %s", - data, + hex.EncodeToString(calculateBlockBodyHashByte[:]), ) }
🧹 Nitpick comments (3)
ledger/common/rewards.go (1)
397-401: Saturation refactor is a no-op; consider sharing logic with CalculatePoolSaturationThe multi-line
math.Mincall is behaviorally identical to the previous expression, so no functional change here. As a follow-up to the existing TODO, it may be worth routing this throughCalculatePoolSaturation(or a shared helper) to avoid the hard-coded0.05and keep saturation logic centralized.ledger/verify_block_test.go (1)
50-60: Clarify semantics of the “should fail” testcase (expectedValidvsexpectedErr)For the
"TestVerifyBlockBody should fail"case you set:expectedValid: true, expectedErr: "unknown proto major 8 for 10-field header",Given the name, it looks like this block is expected to be rejected because
DetermineBlockTypecan’t classify the header. In the current harness, that expectation is expressed viaexpectedErr, andexpectedValidis effectively unused because the function returns early on the type‑discovery error.To avoid confusion and future breakage if the control flow changes, consider either:
- Setting
expectedValid: falsefor this case, or- Renaming the test and/or adding a short comment that this case only asserts the
DetermineBlockTypeerror path, not overall block validity.ledger/verify_block_body.go (1)
201-201: TODO: Non-valid transaction handling not implemented.The comment indicates that invalid transaction handling should be implemented here. Currently,
txSeqNonValidis always treated as empty (Line 239).Would you like me to open a new issue to track implementing support for non-valid transactions in block body hash calculation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cbor/encode.go(2 hunks)connection_test.go(1 hunks)ledger/common/rewards.go(1 hunks)ledger/verify_block.go(1 hunks)ledger/verify_block_body.go(11 hunks)ledger/verify_block_test.go(2 hunks)ledger/verify_kes.go(1 hunks)ledger/verify_vrf.go(1 hunks)protocol/localstatequery/client_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
cbor/encode.goprotocol/localstatequery/client_test.goledger/verify_block_test.goledger/verify_block_body.go
🧬 Code graph analysis (3)
ledger/verify_block_test.go (4)
ledger/verify_block.go (2)
DetermineBlockType(37-101)VerifyBlock(103-280)ledger/block.go (1)
NewBlockHeaderFromCbor(55-78)cbor/decode.go (1)
Decode(29-44)cbor/value.go (1)
LazyValue(367-369)
ledger/verify_block_body.go (4)
cbor/encode.go (4)
EncMode(43-48)Encode(27-40)IndefLengthMap(144-144)IndefLengthList(100-100)ledger/common/certs.go (3)
PoolRegistrationCertificate(435-448)PoolRetirementCertificate(617-623)PoolKeyHash(336-336)ledger/compat.go (3)
PoolRegistrationCertificate(67-67)PoolRetirementCertificate(66-66)NewBlake2b224(30-32)ledger/common/common.go (1)
NewBlake2b224(89-93)
ledger/verify_block.go (9)
cbor/decode.go (1)
Decode(29-44)ledger/shelley/shelley.go (3)
BlockTypeShelley(34-34)NewShelleyBlockHeaderFromCbor(733-739)ShelleyBlockHeader(143-149)ledger/allegra/allegra.go (3)
BlockTypeAllegra(30-30)NewAllegraBlockHeaderFromCbor(434-440)AllegraBlockHeader(139-141)ledger/mary/mary.go (3)
BlockTypeMary(33-33)NewMaryBlockHeaderFromCbor(618-624)MaryBlockHeader(153-155)ledger/alonzo/alonzo.go (3)
BlockTypeAlonzo(36-36)NewAlonzoBlockHeaderFromCbor(840-846)AlonzoBlockHeader(192-194)ledger/babbage/babbage.go (3)
BlockTypeBabbage(36-36)NewBabbageBlockHeaderFromCbor(1003-1009)BabbageBlockHeader(152-158)ledger/conway/conway.go (3)
BlockTypeConway(36-36)NewConwayBlockHeaderFromCbor(730-736)ConwayBlockHeader(152-154)ledger/block.go (1)
Block(25-25)ledger/common/vrf.go (1)
VrfResult(21-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
connection_test.go (1)
22-25: Explicit alias for ouroboros-mock is consistent and non-breakingUsing the
ouroboros_mockalias matches all existing references in this file and improves clarity without changing behavior.protocol/localstatequery/client_test.go (1)
25-25: Explicit ouroboros alias keeps tests consistent with usageAliasing
github.com/blinklabs-io/gouroborosasouroborosmatches the existing type and option usage in this file and mirrors aliasing patterns elsewhere. No behavior change.ledger/verify_vrf.go (1)
384-395: VerifyVrf wrapper cleanly exposes VRF verification semantics
VerifyVrfdelegates toVrfVerifyAndHashand then performs a constant‑time equality check againstexpectedOutput, cleanly separating verification errors (false, err) from a valid proof whose output doesn’t match (false, nil). This is a sensible public helper and aligns well with the existing VRF code.Please ensure new call sites treat
false, nilas “proof valid but output mismatch” and only treat non‑nil errors as hard failures.ledger/verify_block.go (2)
27-33: LGTM!The era-specific imports align with the per-era block header handling strategy introduced in this refactor.
36-101: Verify the legitimacy of proto major 4 and 5 appearing in both 15-field and 10-field header branches.The function currently allows proto majors 4 (Mary) and 5 (Alonzo) to be returned from both the 15-field (Shelley-era) and 10-field (Babbage-era) header branches. While this may be intentional, it's unclear whether:
- These proto majors legitimately appear with 10-field headers in practice
- If so, whether dispatching to BlockTypeMary/BlockTypeAlonzo for a 10-field header is semantically correct (since downstream code expects the header structure to match the block type)
Please confirm this behavior is correct or adjust the logic if proto 4 and 5 should only appear with 15-field headers.
ledger/verify_block_body.go (6)
32-38: LGTM!The generic helper correctly converts typed slices to
[]anyfor use with CBOR indefinite-length encoding.
93-120: LGTM!The encoding logic correctly uses the new
cbor.EncMode()helper while preserving the definite vs. indefinite length behavior based on list size.
128-148: LGTM!The function correctly implements Cardano's CBOR encoding rules, switching between definite and indefinite length maps based on the number of items.
203-218: LGTM!The transaction body and witness encoding correctly uses indefinite-length lists via the
IndefLengthListwrapper, consistent with Cardano's CBOR encoding.
279-329: LGTM!The function signature correctly updates to return common package certificate types, improving consistency across the codebase.
373-395: LGTM!The certificate helper functions correctly use common package types and properly extract pool IDs from the
OperatorandPoolKeyHashfields.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
wolf31o2
force-pushed
the
refactor/verify-block-interface
branch
from
77de2f7 to
1bd00d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/verify_block_body.go (1)
93-120: Critical: Usecbor.IndefLengthListwrapper instead of undefinedcbor.EncModeAPI.Lines 101-113 attempt to manually encode indefinite arrays using
cbor.EncMode()which doesn't exist. The codebase providescbor.IndefLengthListfor this purpose (seecbor/encode.go:112-133and similar usage inprotocol/txsubmission/messages.goandledger/verify_block_body.go:147, 203-204).Replace the manual indefinite array encoding with:
func EncodeCborTxSeq(data []uint) ([]byte, error) { if len(data) <= MAX_LIST_LENGTH_CBOR { return cbor.Encode(data) } return cbor.Encode(cbor.IndefLengthList(convertToAnySlice(data))) }
♻️ Duplicate comments (2)
ledger/verify_block_test.go (1)
82-105: Good: explicit hex/CBOR error checks; minor redundancy on header decodeThe added error checks around
hex.DecodeStringandcbor.Decodeimprove robustness and address earlier concerns about ignored errors, especially before unsafe type assertions.You can optionally avoid decoding the header hex twice by reusing
headerBytes:- // Decode components - headerCborBytes, err := hex.DecodeString(tc.blockHexCbor.HeaderCbor) - if err != nil { - t.Fatalf("failed to decode header hex: %v", err) - } + // Reuse the already-decoded header bytes + headerCborBytes := headerBytesThis keeps the intent the same while eliminating a duplicate decode path.
ledger/verify_block_body.go (1)
84-90: Error message improvement resolves previous concern.The mismatch error now correctly reports the derived hash using
hex.EncodeToString(calculateBlockBodyHashByte[:])instead of the raw block data. This addresses the previous review comment.
🧹 Nitpick comments (6)
cbor/encode.go (1)
137-186: Deterministic IndefLengthMap encoding correctly addresses prior non‑determinismSorting
kvPairsby the CBOR‐encoded key (keyCbor) before emitting key/value pairs ensures deterministic ordering for indefinite‑length maps and aligns with theSortCoreDeterministicgoal used elsewhere. This resolves the earlier risk of non‑reproducible block body hashes.One tiny optional clean‑up (no behavior change):
- You can avoid redundant conversions and just use
len(i)andfor key, value := range isinceIndefLengthMap’s underlying type is alreadymap[any]any.For example:
- kvPairs := make([]keyValue, 0, len(map[any]any(i))) - for key, value := range map[any]any(i) { + kvPairs := make([]keyValue, 0, len(i)) + for key, value := range i {Functionally what you have is correct as‑is.
Based on learnings, this deterministic encoding approach matches the repository’s preference for reproducible CBOR without relying on stored CBOR.
ledger/verify_kes.go (1)
129-171: KES verification helper extraction looks correct; consider explicit handling for “future” periodsThe split into
VerifyKes→VerifyKesComponentskeeps the existing logic while making it reusable and easier to test. The new guard againstslotsPerKesPeriod == 0is a good safety improvement, and the computation ofcurrentKesPeriodandtmatches the previous behavior before refactoring.One optional refinement (for clarity, not correctness):
- When
currentKesPeriod < kesPeriod,tstays at 0 and verification proceeds as if the certificate started at period 0. If that situation is considered an invalid op‑cert in your protocol assumptions, you might instead:
- return
(false, nil)explicitly, or- return an error indicating that
slot/slotsPerKesPeriod < kesPeriod.That would make this edge case more observable to callers, but it would be a behavior change, so only do it if you’re confident no callers depend on the current semantics.
ledger/verify_block_test.go (2)
49-59: Clarify semantics ofexpectedValidwhenexpectedErrshort‑circuits the testWith the new
DetermineBlockTypeflow, whenDetermineBlockTypereturns an error that matchestc.expectedErr, the test returns early and never checkstc.expectedValid. For the"TestVerifyBlockBody should fail"case (Lines 49–59),expectedValidis set totruebut is effectively unused because the header‑parsing error is the success condition.Consider either:
- Setting
expectedValidtofalsefor such cases, or- Documenting that
expectedErrtakes precedence andexpectedValidis ignored whenever an earlier step (likeDetermineBlockType) fails.This doesn’t break correctness, but it makes the intent of the test table clearer to future readers.
Also applies to: 62-81
344-361: VerifyBlock error assertions cover both expected and unexpected pathsThe new checks around
VerifyBlock’s error return now:
- Assert that expected error substrings are present when
tc.expectedErris non‑empty.- Treat any non‑nil
errwhentc.expectedErr == ""as a test failure.That aligns the test with the intended explicit error behavior of
VerifyBlockand ensures unexpected error paths can’t slip through unnoticed.If you want stricter behavior, you could optionally make unexpected errors fatal to stop the subtest immediately:
- if tc.expectedErr == "" && err != nil { - t.Errorf("unexpected error: %v", err) - } + if tc.expectedErr == "" && err != nil { + t.Fatalf("unexpected VerifyBlock error: %v", err) + }The current implementation is already functionally correct; this would just tighten failure reporting.
ledger/verify_block.go (2)
106-247: Header decoding is duplicated and incurs a double CBOR decode; consider centralizingYou first generically decode the header in
DetermineBlockType, then re-decode the same CBOR per era viaNew*BlockHeaderFromCbor, with very similar error handling and slot/blockNo extraction in each case. A small helper (e.g.,parseHeader(headerCbor []byte) (blockType, header, slot, blockNo)) or reusing the initial decode would reduce duplication, simplify maintenance when adding new eras, and save an extra CBOR pass.
372-407: Body-hash recomputation is tightly coupled to positional CBOR layoutReconstructing the body as
{raw[1], raw[2], raw[3]}assumes a fixed[header, body_part1, body_part2, body_part3, ...]layout across all non-Byron eras. That’s brittle for future ledger changes and spreads CBOR layout knowledge into VerifyBlock.Longer term, it would be cleaner if each era-specific block type exposed either the canonical “body CBOR” or the exact body-hash preimage, so this function doesn’t depend on raw index positions into
[]cbor.RawMessage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cbor/encode.go(2 hunks)connection_test.go(1 hunks)ledger/allegra/allegra.go(1 hunks)ledger/alonzo/alonzo.go(1 hunks)ledger/babbage/babbage.go(2 hunks)ledger/byron/byron.go(4 hunks)ledger/common/block.go(1 hunks)ledger/common/rewards.go(1 hunks)ledger/conway/conway.go(1 hunks)ledger/leios/leios.go(3 hunks)ledger/mary/mary.go(1 hunks)ledger/shelley/shelley.go(2 hunks)ledger/verify_block.go(1 hunks)ledger/verify_block_body.go(11 hunks)ledger/verify_block_test.go(2 hunks)ledger/verify_kes.go(2 hunks)ledger/verify_vrf.go(1 hunks)protocol/localstatequery/client_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- protocol/localstatequery/client_test.go
- ledger/common/rewards.go
- ledger/verify_vrf.go
- connection_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
cbor/encode.goledger/verify_block_test.goledger/verify_block_body.go
🧬 Code graph analysis (6)
ledger/mary/mary.go (3)
ledger/mary.go (1)
MaryBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/allegra/allegra.go (3)
ledger/allegra.go (1)
AllegraBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/alonzo/alonzo.go (3)
ledger/alonzo.go (1)
AlonzoBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/leios/leios.go (1)
ledger/leios.go (2)
LeiosBlockHeader(24-24)LeiosRankingBlock(26-26)
ledger/shelley/shelley.go (3)
ledger/shelley.go (2)
ShelleyBlock(24-24)ShelleyBlockHeader(25-25)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/verify_block_test.go (4)
ledger/verify_block.go (1)
DetermineBlockType(40-104)ledger/block.go (1)
NewBlockHeaderFromCbor(55-78)ledger/common/block.go (2)
Block(5-11)BlockHeader(13-23)cbor/decode.go (1)
Decode(29-44)
🪛 GitHub Actions: go-test
ledger/verify_block_body.go
[error] 101-101: Go compile error: undefined: cbor.EncMode during 'go test ./...'
🪛 GitHub Actions: golangci-lint
ledger/verify_block_body.go
[error] 101-101: golangci-lint: undefined: cbor.EncMode (typecheck) in ledger/verify_block_body.go
🪛 GitHub Actions: nilaway
ledger/verify_block_body.go
[error] 101-101: nilaway step failed: undefined: cbor.EncMode in ledger/verify_block_body.go:101.18
🪛 GitHub Check: go-test (1.24.x, ubuntu-latest)
ledger/verify_block_body.go
[failure] 101-101:
undefined: cbor.EncMode
[failure] 101-101:
undefined: cbor.EncMode
🪛 GitHub Check: lint
ledger/verify_block_body.go
[failure] 101-101:
undefined: cbor.EncMode (typecheck)
🪛 GitHub Check: nilaway
ledger/verify_block_body.go
[failure] 101-101:
undefined: cbor.EncMode
[failure] 101-101:
undefined: cbor.EncMode
[failure] 101-101:
undefined: cbor.EncMode
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (23)
ledger/common/block.go (1)
13-23: BlockHeader.BlockBodyHash extension looks consistent with per‑era implementationsAdding
BlockBodyHash() Blake2b256toBlockHeadercleanly centralizes access to the body hash for all eras and matches the new per‑era header methods introduced elsewhere. No issues from an interface/contract standpoint.ledger/mary/mary.go (1)
153-155: MaryBlock.BlockBodyHash delegation is consistent with the new header interfaceDelegating
MaryBlock.BlockBodyHash()tob.Header().BlockBodyHash()cleanly reuses the per‑era header implementation and aligns with the updatedBlockHeaderinterface. This is consistent with the other era block types added in this PR.ledger/alonzo/alonzo.go (1)
192-194: AlonzoBlock.BlockBodyHash matches the cross‑era pattern
AlonzoBlock.BlockBodyHash()correctly forwards tob.Header().BlockBodyHash(), matching the newBlockHeaderinterface and the implementations in other eras. No issues.cbor/encode.go (1)
67-73: Invalid review comment — syntax is valid in Go 1.22+The loop
for i := range typeSrc.NumField()is valid Go syntax starting with Go 1.22 (released March 2024). This project explicitly usesgo 1.24.0, which fully supports range-over-int. The code compiles and runs correctly as written. The same pattern is consistently used incbor/decode.go:142, confirming intentional use.No fixes required.
Likely an incorrect or invalid review comment.
ledger/allegra/allegra.go (1)
139-141: LGTM! Consistent delegation pattern.The
BlockBodyHash()accessor correctly delegates to the header's implementation, providing a convenient way to retrieve the block body hash directly from the block instance.ledger/babbage/babbage.go (2)
152-154: LGTM! Clean delegation.The block-level accessor appropriately delegates to the header's
BlockBodyHash()method.
235-237: LGTM! Direct field access.The header-level accessor correctly returns the block body hash from the header body structure.
ledger/conway/conway.go (1)
152-154: LGTM! Consistent implementation.The accessor correctly delegates to the header. Note that
ConwayBlockHeaderembedsbabbage.BabbageBlockHeader(line 157), so it inherits theBlockBodyHash()implementation from Babbage.ledger/shelley/shelley.go (2)
143-145: LGTM! Proper delegation.The block-level accessor appropriately delegates to the header's
BlockBodyHash()method.
216-218: LGTM! Field access is correct.The header-level accessor correctly returns the
BlockBodyHashfield from the header body structure (line 164).ledger/byron/byron.go (3)
809-811: LGTM! Consistent delegation.The accessor correctly delegates to the header's
BlockBodyHash()method.
878-880: LGTM! Consistent delegation.The accessor correctly delegates to the header's
BlockBodyHash()method.
738-740: Remove reliance on non-existent reference pattern.The review references applying "the same defensive pattern" from
ByronMainBlockHeader.BlockBodyHash(), but that method uses an identical unchecked type assertion (line 140):return h.BodyProof.(common.Blake2b256). No defensive pattern exists in the reference code.If a defensive pattern is genuinely needed for type safety, it should be specified directly (e.g., use the comma-ok idiom), and both methods should be updated consistently. If the unchecked assertion is safe due to an invariant, document it. The review as written cannot be acted upon because it references a pattern that doesn't exist.
Likely an incorrect or invalid review comment.
ledger/leios/leios.go (2)
114-116: LGTM! Direct field access.The header-level accessor correctly returns the
BlockBodyHashfield from the header body.
259-261: LGTM! Consistent delegation.The
LeiosRankingBlockaccessor correctly delegates to its header'sBlockBodyHash()method, and the block has a properBlockHeaderfield (line 175).ledger/verify_block_body.go (5)
32-38: Generic helper looks good.The
convertToAnySlicehelper correctly converts typed slices to[]anyfor use with the CBOR encoding functions.
128-148: Refactored encoding properly handles indefinite length maps.The
encodeAuxDatafunction correctly usescbor.IndefLengthMapfor maps larger than 23 entries, matching Cardano's CBOR encoding conventions.
203-218: Consistent use of IndefLengthList pattern.The encoding of transaction sequences properly uses
cbor.IndefLengthList(convertToAnySlice(...))to match Cardano's encoding rules for large lists.
279-329: Type migration to common package types is correct.The signature change to return
common.PoolRegistrationCertificateandcommon.PoolRetirementCertificatealigns with the codebase's move to common package types. The certificate handling logic correctly uses type assertions to identify pool registration and retirement certificates.
373-395: Pool ID extraction updated correctly.Both helper functions properly use
NewBlake2b224to compute pool IDs from theOperator(registration) andPoolKeyHash(retirement) fields of the common certificate types.ledger/verify_block.go (3)
39-104: DetermineBlockType era/protoMajor mapping looks correct and defensiveThe CBOR shape checks plus explicit protoMajor mapping give clear failures on malformed or unknown headers, which is appropriate for steering the new era-specific pipeline. Just remember to extend this function when adding future eras.
253-307: Previous issue about VRF/KES/body checks being skipped is now resolved
vrfValidandkesValidare now driven by real calls toVerifyVrfandVerifyKesComponents, and block-body hashing is enforced (with explicit errors on mismatch). The finalisValid = isBodyValid && vrfValid && kesValidcleanly separates “invalid block” (bool=false) from operational errors (non-nil error), addressing the earlier concern where these were hard-coded to true.Also applies to: 357-370, 410-412
106-110: The review comment is incorrect—validation already exists inVerifyKesComponents.The suggested precondition check is unnecessary.
VerifyKesComponents(ledger/verify_kes.go:162-164) already validatesslotsPerKesPeriod == 0before the division operation, andslotsPerKesPeriodis never used for arithmetic inVerifyBlockbefore being passed to that function. No changes are required.Likely an incorrect or invalid review comment.
wolf31o2
force-pushed
the
refactor/verify-block-interface
branch
from
1bd00d8 to
4486d29
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ledger/verify_block_test.go (1)
104-131: Avoid per-indexnilmetadata entries in Shelley branchIn the Shelley branch,
transactionMetadataSetis populated withnilvalues for every index (Lines 109–121). Other eras now pass an empty map without per-index keys, which is less surprising and avoids future ambiguity where “presence of a key” might be interpreted as “metadata exists”.For consistency and simpler semantics, you can drop the per-index assignments and use an empty map:
- transactionMetadataSet := make(map[uint]*cbor.LazyValue) - for i, tx := range txs { + for i, tx := range txs { txArray := tx.([]interface{}) if _, err := cbor.Decode(txArray[0].([]byte), &transactionBodies[i]); err != nil { t.Fatalf( "failed to decode Shelley transaction body %d: %v", i, err, ) } - // Skip metadata for test - set to nil - transactionMetadataSet[uint(i)] = nil } shelleyHeader := header.(*shelley.ShelleyBlockHeader) block = &shelley.ShelleyBlock{ BlockHeader: shelleyHeader, TransactionBodies: transactionBodies, TransactionWitnessSets: make( []shelley.ShelleyTransactionWitnessSet, len(txs), ), - TransactionMetadataSet: transactionMetadataSet, + TransactionMetadataSet: map[uint]*cbor.LazyValue{}, }This keeps Shelley aligned with the other eras’ “no metadata in this test” behavior.
🧹 Nitpick comments (3)
ledger/verify_block_test.go (1)
62-81: Clarify semantics ofexpectedErrvs header-type failures
expectedErris currently used both to validateDetermineBlockTypeerrors and laterVerifyBlockerrors. That works for the current cases (only the “unknown proto major” test uses it), but it will misbehave if you later add tests that expect a VerifyBlock error whileDetermineBlockTypesucceeds (they’ll hit the “expected error but got none” branch at Line 78).Consider either:
- Splitting header-type tests into a separate table/test, or
- Introducing a distinct
expectedHeaderErr(or similar) so VerifyBlock error expectations stay orthogonal.This is a structural test-harness nuance, not a blocker.
ledger/verify_block.go (2)
39-104: Consider defensive handling for type assertions.
DetermineBlockTypeperforms multiple unchecked array accesses (e.g.,body[13],body[9]) that could panic if the CBOR structure is malformed in unexpected ways. While you do checklen(body), specific field access doesn't validate array bounds before indexing.Add explicit bounds checks for the specific indices you access:
switch lenBody { case 15: // Shelley era + if len(body) <= 13 { + return 0, errors.New("header body too short for proto major field") + } protoMajor, ok := body[13].(uint64) if !ok { return 0, errors.New("invalid proto major") } // ... case 10: // Babbage era + if len(body) <= 9 { + return 0, errors.New("header body too short for proto version field") + } protoVersion, ok := body[9].([]any) if !ok || len(protoVersion) < 1 { return 0, errors.New("invalid proto version") }This prevents panics if the CBOR decoder returns an array shorter than the length check indicated.
374-407: Skipping body hash validation for empty CBOR may hide issues.Lines 376-378 skip body hash validation when
block.Cbor()is empty, with a comment "Skip validation for manually created blocks (e.g., in tests)". This creates a validation bypass that could mask bugs if production code accidentally creates blocks without stored CBOR.Consider one of these approaches:
- Make the bypass explicit by adding a parameter or checking a specific flag:
- if len(rawCbor) == 0 { - // Skip validation for manually created blocks (e.g., in tests) - isBodyValid = true + if len(rawCbor) == 0 { + // Manually created blocks without CBOR cannot be validated + return false, "", 0, 0, errors.New( + "VerifyBlock: cannot validate block body hash without stored CBOR", + )
- Document the bypass more clearly if it's intentional:
if len(rawCbor) == 0 { - // Skip validation for manually created blocks (e.g., in tests) + // Skip validation for test blocks that lack CBOR encoding. + // Production blocks must always have stored CBOR. isBodyValid = trueThis makes the validation gap explicit and prevents accidental bypasses in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cbor/encode.go(2 hunks)connection_test.go(1 hunks)ledger/allegra/allegra.go(1 hunks)ledger/alonzo/alonzo.go(1 hunks)ledger/babbage/babbage.go(2 hunks)ledger/byron/byron.go(4 hunks)ledger/common/block.go(1 hunks)ledger/common/rewards.go(1 hunks)ledger/conway/conway.go(1 hunks)ledger/leios/leios.go(4 hunks)ledger/leios/leios_test.go(3 hunks)ledger/mary/mary.go(1 hunks)ledger/shelley/shelley.go(2 hunks)ledger/verify_block.go(1 hunks)ledger/verify_block_body.go(10 hunks)ledger/verify_block_test.go(2 hunks)ledger/verify_kes.go(2 hunks)ledger/verify_vrf.go(1 hunks)protocol/localstatequery/client_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- protocol/localstatequery/client_test.go
- ledger/conway/conway.go
- ledger/mary/mary.go
- ledger/allegra/allegra.go
- ledger/common/rewards.go
- ledger/babbage/babbage.go
- cbor/encode.go
- ledger/alonzo/alonzo.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
ledger/verify_block_body.goledger/verify_block_test.goledger/leios/leios_test.goledger/verify_block.go
🧬 Code graph analysis (4)
ledger/leios/leios_test.go (1)
ledger/leios/leios.go (1)
LeiosEndorserBlockBody(63-67)
ledger/leios/leios.go (5)
cbor/cbor.go (1)
StructAsArray(45-48)ledger/common/tx.go (1)
Transaction(25-36)cbor/encode.go (1)
Encode(28-41)ledger/common/common.go (1)
Blake2b256Hash(73-85)ledger/common/block.go (1)
Block(5-11)
ledger/shelley/shelley.go (3)
ledger/shelley.go (2)
ShelleyBlock(24-24)ShelleyBlockHeader(25-25)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/verify_block.go (10)
cbor/decode.go (1)
Decode(29-44)ledger/shelley/shelley.go (3)
BlockTypeShelley(34-34)NewShelleyBlockHeaderFromCbor(741-747)ShelleyBlockHeader(147-153)ledger/allegra/allegra.go (3)
BlockTypeAllegra(30-30)NewAllegraBlockHeaderFromCbor(438-444)AllegraBlockHeader(143-145)ledger/mary/mary.go (3)
BlockTypeMary(33-33)NewMaryBlockHeaderFromCbor(622-628)MaryBlockHeader(157-159)ledger/alonzo/alonzo.go (3)
BlockTypeAlonzo(36-36)NewAlonzoBlockHeaderFromCbor(844-850)AlonzoBlockHeader(196-198)ledger/babbage/babbage.go (3)
BlockTypeBabbage(36-36)NewBabbageBlockHeaderFromCbor(1011-1017)BabbageBlockHeader(156-162)ledger/conway/conway.go (3)
BlockTypeConway(36-36)NewConwayBlockHeaderFromCbor(734-740)ConwayBlockHeader(156-158)ledger/common/block.go (2)
BlockHeader(13-23)Block(5-11)ledger/verify_vrf.go (2)
MkInputVrf(39-55)VerifyVrf(384-395)ledger/verify_kes.go (1)
VerifyKesComponents(154-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
ledger/verify_vrf.go (1)
384-395: LGTM!The implementation correctly wraps VRF verification with constant-time comparison of the output, which is essential for preventing timing attacks in cryptographic operations. The error handling is appropriate, and the function signature is clear.
ledger/leios/leios_test.go (2)
123-128: LGTM!The test data correctly uses the new
Body: &LeiosEndorserBlockBody{...}structure, consistent with the refactoring to encapsulate transaction references within a body type.
164-206: LGTM!The CBOR round-trip test correctly validates the new structure with appropriate nil checks and accesses
decoded.Body.TxReferencesinstead of the previously direct field access. This ensures proper serialization/deserialization of the nested body structure.ledger/verify_kes.go (1)
144-172: LGTM!The refactoring cleanly extracts KES verification logic into
VerifyKesComponents, enabling reuse. The validation forslotsPerKesPeriod == 0at line 162 is essential to prevent division by zero at line 165. The future certificate check (lines 166-169) provides proper validation of operational certificate timing.ledger/common/block.go (1)
22-22: LGTM!The addition of
BlockBodyHash() Blake2b256to theBlockHeaderinterface provides uniform access to block body hashes across all ledger eras, supporting the era-aware verification pipeline introduced in this PR.ledger/shelley/shelley.go (2)
143-145: LGTM!The
ShelleyBlock.BlockBodyHash()accessor correctly delegates to the header, following the consistent pattern across other eras.
216-218: LGTM!The
ShelleyBlockHeader.BlockBodyHash()accessor correctly returns the typedBlockBodyHashfield from the header body.ledger/byron/byron.go (2)
823-825: LGTM, but depends on resolution of header method.The delegation pattern is correct, but this method's safety depends on resolving the panic issue in
ByronMainBlockHeader.BlockBodyHash()(lines 139-148).
892-894: LGTM, but depends on resolution of header method.The delegation pattern is correct, but this method's safety depends on resolving the panic issue in
ByronEpochBoundaryBlockHeader.BlockBodyHash()(lines 745-754).ledger/verify_block_test.go (1)
82-290: Per-era block construction and VerifyBlock wiring look solidThe refactored flow—hex decode with error checks, era detection via
DetermineBlockType,NewBlockHeaderFromCborfor typed headers, CBOR body decoding with explicit error handling per era, and then callingVerifyBlock(block, eta0, slotsPerKesPeriod)—is cohesive and matches the new API design.Body-decoding branches correctly:
- Handle
[]bytevsstringTX bodies (with hex decode + failure messages),- Use era-specific transaction body types and block header types,
- Construct fully-typed
Blockinstances before verification,- Treat unexpected
VerifyBlockerrors as test failures whenexpectedErris empty.This gives good coverage of the new per-era VerifyBlock pipeline without re-parsing or ignoring decode errors.
Also applies to: 336-355
ledger/verify_block_body.go (1)
298-305: Review comment is incorrect — no type compatibility issue exists.The bare names
PoolRegistrationCertificateandPoolRetirementCertificateused in the type assertions are type aliases defined inledger/compat.go(lines 66-67) that reference the common package types. In Go, a type alias creates an alternate name for the same underlying type within package scope, so appending dereferenced values to[]common.Pool*slices is valid. No conversion or explicit qualification is needed.Likely an incorrect or invalid review comment.
connection_test.go (2)
22-25: Import aliasouroboros_mockis clear and consistentUsing an explicit
ouroboros_mockalias for the mock package matches the usage in this test and keeps imports unambiguous; no issues here.
58-83: Proposed fix is incorrect and would deadlockThe race condition on
ErrorChan()is a valid concern. However, the suggested fix usingfor err := range oConn.ErrorChan()will not work:Connection.errorChanis never closed by the codebase (only the internalMuxer.errorChanis closed at muxer.go:114). The goroutine will block indefinitely on the range loop,donenever closes, and the test times out.A corrected approach should use a separate
donechannel as proposed, but signal completion explicitly rather than relying on channel closure:done := make(chan struct{}) go func() { err, ok := <-oConn.ErrorChan() if !ok { close(done) return } panic(fmt.Sprintf("unexpected Ouroboros connection error: %s", err)) }() // ... rest of test ... select { case <-done: case <-time.After(10 * time.Second): t.Errorf("did not shutdown within timeout") }This preserves single-consumer semantics without assuming channel closure.
wolf31o2
force-pushed
the
refactor/verify-block-interface
branch
from
4486d29 to
e583b30
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cbor/encode.go (1)
66-73: Fix iteration over struct fields in EncodeGeneric
for i := range typeSrc.NumField()will not compile in Go (rangecan’t be used on anint). This should be an indexed loop over the field count:- for i := range typeSrc.NumField() { + for i := 0; i < typeSrc.NumField(); i++ { tmpField := typeSrc.Field(i) if tmpField.IsExported() && tmpField.Name != "DecodeStoreCbor" { srcTypeFields = append(srcTypeFields, tmpField) } }Without this change, any use of
EncodeGenericwill fail at build time.
🧹 Nitpick comments (5)
cbor/encode.go (1)
137-186: Deterministic indefinite-length map encoding looks correctSorting entries by their CBOR-encoded keys before emitting key/value pairs gives you stable, deterministic bytes for
IndefLengthMapwhile still using the indefinite-length map form, which aligns with the repository’s deterministic CBOR goal and fixes the earlier non-determinism in map iteration. Error handling is straightforward and you only encode each key once. Based on learnings, this is the right direction for CBOR determinism.ledger/verify_block_test.go (2)
64-80: ClarifyexpectedErrsemantics between header parsing and VerifyBlockRight now
expectedErris checked in two different places with different meanings:
- Lines 70–76: used to match
DetermineBlockTypeerrors and early‑return.- Line 79: if
blockTypeis determined successfully butexpectedErris non‑empty, the test fails immediately (before callingVerifyBlock), even though some future tests might legitimately expect an error fromVerifyBlockinstead of from type‑detection.- Lines 346–352:
expectedErris also used to matchVerifyBlockerrors.This coupling makes it impossible to add a case where header parsing succeeds but
VerifyBlockis expected to fail with a specific error string.Consider separating concerns, e.g.:
- Add distinct fields like
expectedHeaderErrandexpectedVerifyErr, or- Treat
expectedErrstrictly as theVerifyBlockerror and handle unexpectedDetermineBlockTypefailures with a separate check, not by reusingexpectedErr.That will keep the harness future‑proof and avoid confusing double use of the same field.
Also applies to: 334-352
97-102: Guard CBOR shape assumptions / consider factoring repeated decode helperThe per‑era transaction decoding assumes a very specific CBOR shape:
var txs []interface{}thentxArray := tx.([]interface{})without checking the assertion.txArray[0]is assumed to be either[]byteorstring; any other shape panics via type assertions.Given these are test fixtures that should be well‑formed, this is acceptable but brittle: a malformed fixture or upstream CBOR change will yield panics rather than clear test failures.
If you want slightly more robust and DRY tests, consider:
Introducing a small helper like:
func decodeTxBodyBytes(t *testing.T, v interface{}) []byte { switch b := v.(type) { case []byte: return b case string: body, err := hex.DecodeString(b) if err != nil { t.Fatalf("failed to hex decode transaction body string: %v", err) } return body default: t.Fatalf("unexpected type for tx body: %T", v) return nil } }And then, in each era branch:
txArray := tx.([]interface{}) txBodyBytes := decodeTxBodyBytes(t, txArray[0]) if _, err := cbor.Decode(txBodyBytes, &transactionBodies[i]); err != nil { t.Fatalf("failed to decode <Era> transaction body %d: %v", i, err) }This avoids duplication across Allegra/Mary/Alonzo/Babbage/Conway and centralizes the shape assumptions.
Also applies to: 111-112, 137-151, 175-189, 214-228, 255-268, 295-308
ledger/verify_block.go (1)
54-78: Redundant length check in Shelley era branch.Line 57 checks
len(body) <= 13after line 54 already confirmedlenBody == 15. This condition will never be true, making the error at lines 58-59 unreachable.Apply this diff to remove the redundant check:
case 15: // Shelley era - if len(body) <= 13 { - return 0, errors.New("header body too short for proto major field") - } protoMajor, ok := body[13].(uint64) if !ok { return 0, errors.New("invalid proto major") }ledger/verify_block_body.go (1)
219-220: Consider using cbor.Encode for empty array instead of hardcoding.Lines 219-220 hardcode
[]byte{0x80}as the CBOR encoding of an empty array. While this is correct, it's fragile if CBOR encoding rules change or the deterministic encoding mode is enabled.Consider using
cbor.Encodefor consistency with the rest of the function:- // txSeqNonValid is always empty, so we encode an empty list - txSeqNonValidBytes := []byte{0x80} // CBOR encoding of empty array + // txSeqNonValid is always empty, so we encode an empty list + txSeqNonValidBytes, _ := cbor.Encode([]any{}) // CBOR encoding of empty arrayThis ensures consistent encoding behavior across all components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cbor/encode.go(2 hunks)connection_test.go(1 hunks)ledger/allegra/allegra.go(1 hunks)ledger/alonzo/alonzo.go(1 hunks)ledger/babbage/babbage.go(2 hunks)ledger/byron/byron.go(4 hunks)ledger/common/block.go(1 hunks)ledger/common/rewards.go(1 hunks)ledger/conway/conway.go(1 hunks)ledger/leios/leios.go(5 hunks)ledger/leios/leios_test.go(3 hunks)ledger/mary/mary.go(1 hunks)ledger/shelley/shelley.go(2 hunks)ledger/verify_block.go(1 hunks)ledger/verify_block_body.go(10 hunks)ledger/verify_block_test.go(2 hunks)ledger/verify_kes.go(2 hunks)ledger/verify_vrf.go(1 hunks)protocol/localstatequery/client_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ledger/common/rewards.go
🚧 Files skipped from review as they are similar to previous changes (7)
- ledger/common/block.go
- ledger/shelley/shelley.go
- ledger/conway/conway.go
- ledger/verify_vrf.go
- ledger/babbage/babbage.go
- ledger/leios/leios_test.go
- protocol/localstatequery/client_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
ledger/verify_block.goledger/verify_block_body.goledger/verify_block_test.gocbor/encode.go
🧬 Code graph analysis (4)
ledger/alonzo/alonzo.go (3)
ledger/alonzo.go (1)
AlonzoBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/mary/mary.go (3)
ledger/mary.go (1)
MaryBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/allegra/allegra.go (3)
ledger/allegra.go (1)
AllegraBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/verify_block_test.go (9)
ledger/verify_block.go (2)
DetermineBlockType(40-112)VerifyBlock(195-422)ledger/block.go (1)
NewBlockHeaderFromCbor(55-78)ledger/common/block.go (2)
Block(5-11)BlockHeader(13-23)ledger/shelley/shelley.go (4)
BlockTypeShelley(34-34)ShelleyBlockHeader(147-153)ShelleyBlock(50-57)ShelleyBlock(70-72)ledger/allegra/allegra.go (4)
BlockTypeAllegra(30-30)AllegraBlockHeader(143-145)AllegraBlock(46-53)AllegraBlock(66-68)ledger/mary/mary.go (4)
BlockTypeMary(33-33)MaryBlockHeader(157-159)MaryBlock(49-56)MaryBlock(80-82)ledger/alonzo/alonzo.go (4)
BlockTypeAlonzo(36-36)AlonzoBlockHeader(196-198)AlonzoBlock(52-60)AlonzoBlock(113-115)ledger/babbage/babbage.go (4)
BlockTypeBabbage(36-36)BabbageBlockHeader(156-162)BabbageBlock(52-60)BabbageBlock(73-75)ledger/conway/conway.go (4)
BlockTypeConway(36-36)ConwayBlockHeader(156-158)ConwayBlock(52-60)ConwayBlock(73-75)
🔇 Additional comments (16)
connection_test.go (2)
23-23: LGTM!The import alias
ouroboros_mockimproves clarity and aligns with the consistency improvements across the PR's test files.
44-53: LGTM!The alias is used consistently throughout the test setup. The refactoring maintains all existing test logic while improving code clarity.
ledger/verify_kes.go (1)
144-172: KES verification extraction and validation look soundDelegating from
VerifyKesintoVerifyKesComponentskeeps the header-specific wiring separate from the core logic, and the newslotsPerKesPeriod == 0guard prevents a potential divide-by-zero. ComputingcurrentKesPeriodfromslot / slotsPerKesPeriod, rejecting certificates whose start period is in the future, and then passingt = currentKesPeriod - kesPeriodintoverifySignedKESall match the intended KES period semantics.ledger/alonzo/alonzo.go (1)
192-194: Alonzo BlockBodyHash accessor is consistent and minimalDelegating
BlockBodyHashthroughHeader()matches the existing pattern for other header-backed accessors and aligns Alonzo with the common block interface that exposes body hashes.ledger/mary/mary.go (1)
153-155: Mary BlockBodyHash accessor matches the shared block APIThis simple delegation to
Header().BlockBodyHash()keeps Mary’s block type aligned with the common block interface and with the patterns used in other eras.ledger/allegra/allegra.go (1)
139-141: Allegra BlockBodyHash accessor is aligned with other erasThe new
BlockBodyHashmethod cleanly delegates to the header and brings Allegra blocks in line with the updated common block/body-hash interface.ledger/byron/byron.go (4)
139-148: Improved handling: graceful failure instead of panic.The previous review concern about panicking in the verification path has been addressed. Returning a zero hash when the type assertion fails allows validation to fail gracefully rather than crashing, which prevents potential DoS from malformed input while ensuring that invalid blocks are rejected downstream.
745-754: LGTM: Consistent implementation with main block header.The implementation follows the same graceful failure pattern as
ByronMainBlockHeader.BlockBodyHash(), ensuring consistent behavior across both Byron block header types.
823-825: LGTM: Clean delegation pattern.The block-level
BlockBodyHash()method correctly delegates to the header's implementation, maintaining consistency with the overall design pattern.
892-894: LGTM: Consistent delegation across block types.The epoch boundary block correctly delegates to its header's
BlockBodyHash()implementation, mirroring the pattern used inByronMainBlock.ledger/leios/leios.go (2)
73-84: LGTM with caveat: panic acceptable for experimental code.The panic on CBOR encoding failure (lines 78-82) is appropriately documented as experimental development behavior (lines 74-76, 80-81). The NOTE at lines 17-19 explicitly states Leios is in development and validation may be skipped.
Before production use, consider refactoring to return
(common.Blake2b256, error)instead of panicking, allowing callers to handle encoding failures gracefully.Do you plan to refactor this to return errors before Leios moves to production?
182-199: LGTM: Proper nil-checking and direct Body delegation.The nil checks for
b.Bodyat lines 183 and 195 properly guard against nil pointer dereferences. TheBlockBodyHash()implementation (lines 194-199) now correctly delegates tob.Body.BlockBodyHash()instead of going through the placeholder header, which resolves the concern raised in past reviews about the empty header at lines 169-171 causing zero hash returns.ledger/verify_block_body.go (4)
32-38: LGTM: Clean generic helper for type conversion.The
convertToAnySlicefunction provides a clean, type-safe way to convert typed slices to[]anyfor CBOR encoding. The implementation is straightforward and correct.
84-90: LGTM: Error message now correctly reports calculated hash.The mismatch error at lines 84-90 now properly includes the hex-encoded calculated body hash (line 87), addressing the past review comment that noted the error was printing the raw block data instead.
109-129: LGTM: Proper definite/indefinite length map encoding.The refactored
encodeAuxDatafunction correctly implements:
- Empty map handling (lines 112-114)
- Definite-length encoding for ≤23 elements (lines 115-122)
- Indefinite-length encoding with
IndefLengthMapfor >23 elements (lines 123-128)The use of
IndefLengthMapaligns with the PR's introduction of deterministic CBOR encoding for indefinite-length structures.
297-306: No type compatibility concerns—code uses only common types directly.Script results show no local type definitions for
PoolRegistrationCertificateorPoolRetirementCertificateexist inverify_block_body.go. Both types are defined only inledger/common/certs.goand are used directly in the code. The switch cases and field accesses (.Operatorat line 360,.PoolKeyHashat line 372) are operating on common types, not converting between local and common types. No type compatibility verification is needed.Likely an incorrect or invalid review comment.
wolf31o2
force-pushed
the
refactor/verify-block-interface
branch
from
e583b30 to
8d49b9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cbor/encode.go (1)
67-72: Fix invalidrangeoverNumField()in EncodeGeneric
for i := range typeSrc.NumField()is invalid Go (you can't range over anint) and will not compile. This should iterate from0up toNumField()explicitly.- srcTypeFields := []reflect.StructField{} - for i := range typeSrc.NumField() { + srcTypeFields := []reflect.StructField{} + for i := 0; i < typeSrc.NumField(); i++ { tmpField := typeSrc.Field(i) if tmpField.IsExported() && tmpField.Name != "DecodeStoreCbor" { srcTypeFields = append(srcTypeFields, tmpField) } }ledger/verify_block_body.go (1)
268-318: Fix type assertions to use qualified common package names.The type assertions at lines 307 and 310 use bare type names (
*PoolRegistrationCertificate,*PoolRetirementCertificate), buttx.Certificates()returns[]common.Certificate. The assertions will never match and silently fail because the concrete types are*common.PoolRegistrationCertificateand*common.PoolRetirementCertificate. Update both case statements to use the qualified names.
♻️ Duplicate comments (1)
ledger/verify_block.go (1)
379-414: Test bypass in block body hash verification still undermines production safety.Lines 381-386 skip body hash validation when
len(rawCbor) == 0, with a comment stating this bypass "should never occur in production code." However, there's no runtime enforcement preventing this code path in production builds. This issue was flagged in previous reviews and remains unresolved.Consider one of these approaches:
Option 1: Fail explicitly and require callers to handle test blocks differently:
if len(rawCbor) == 0 { - // Skip validation for test blocks that lack CBOR encoding. - // Production blocks must always have stored CBOR for validation. - // This bypass should never occur in production code. - isBodyValid = true + return false, "", 0, 0, errors.New( + "VerifyBlock: block missing CBOR encoding required for body hash validation", + ) } else {Option 2: Add an explicit parameter to VerifyBlock indicating test mode, making the bypass intentional:
func VerifyBlock( block Block, eta0Hex string, slotsPerKesPeriod uint64, + allowTestBlocks bool, ) (bool, string, uint64, uint64, error) {Then gate the bypass:
if len(rawCbor) == 0 { - // Skip validation for test blocks that lack CBOR encoding. - // Production blocks must always have stored CBOR for validation. - // This bypass should never occur in production code. - isBodyValid = true + if !allowTestBlocks { + return false, "", 0, 0, errors.New( + "VerifyBlock: block missing CBOR encoding required for body hash validation", + ) + } + // Test mode: skip validation for blocks without CBOR + isBodyValid = true } else {
🧹 Nitpick comments (10)
ledger/leios/leios_test.go (2)
166-171: Consider testing the Transactions field.The construction correctly uses the new
Bodyfield structure. However, the round-trip test only verifiesTxReferencesand does not exercise theTransactionsfield that is part ofLeiosEndorserBlockBody.Add a test case that includes transactions:
original := &LeiosEndorserBlock{ Body: &LeiosEndorserBlockBody{ + Transactions: []common.Transaction{ + /* test transaction data */ + }, TxReferences: map[common.Blake2b256]uint16{ {0x01, 0x02}: 100, {0x03, 0x04}: 200, }, }, }
187-206: Optionally verify byte-identical re-encoding.The nil checks and field access through
Body.TxReferencesare correct. The test properly verifies that the map contents survive the round-trip.Based on learnings, you could optionally enhance this test to verify byte-identical re-encoding (without using stored CBOR):
// Re-encode and verify byte-identical output reencoded, err := cbor.Encode(&decoded) if err != nil { t.Fatalf("Failed to re-encode: %v", err) } if !bytes.Equal(cborData, reencoded) { t.Error("CBOR encoding is not deterministic") }cbor/encode.go (1)
137-186: Deterministic IndefLengthMap encoding aligns with CBOR hashing requirementsSorting entries by CBOR-encoded key before emitting the indefinite-length map gives you deterministic bytes while preserving the intended on-chain indefinite form. This addresses the earlier non-determinism from ranging directly over
map[any]anyand is consistent with the repo’s deterministic CBOR goals. Based on learnings, this is the right direction.ledger/byron/byron.go (2)
139-148: ByronMainBlockHeader.BlockBodyHash now fails gracefully instead of panickingCasting
BodyProoftocommon.Blake2b256and falling back to a zero hash if the type doesn’t match avoids panics in the verification path while still causing any malformed header to fail body‑hash checks.
745-754: ByronEpochBoundaryBlockHeader.BlockBodyHash mirrors the main-block behavior safelySame pattern as the main header—use
BodyProofwhen it’s the expected type, otherwise return a zero hash so verification fails rather than panicking. This is an appropriate hardening for boundary blocks.ledger/verify_kes.go (1)
154-172: VerifyKesComponents encapsulation and defensive checks look good—confirm signature length guaranteesThe new helper cleanly encapsulates KES period arithmetic, adds a necessary guard against
slotsPerKesPeriod == 0, and returns(false, nil)for certificates that aren’t yet valid.One thing to double‑check:
verifySignedKESpanics if the KES signature byte slice isn’t exactly the expected size. Please ensure that upstream CBOR decoding or validation guarantees the signature length before callingVerifyKesComponents, otherwise a malformed block or external caller could still trigger a panic here.ledger/verify_block_test.go (2)
62-80: ClarifyexpectedErrsemantics between block‑type detection and VerifyBlock
expectedErris consumed in two places: once forDetermineBlockType(short‑circuiting the test when it fails with the expected message) and again afterVerifyBlock. That’s fine for the current fixture where the onlyexpectedErrcomes from the header path, but it becomes brittle if you later add cases that expect an error fromVerifyBlockwhile header parsing still succeeds—those would trip theexpectedErr != ""check here beforeVerifyBlockruns.Consider either:
- Adding a separate
expectedHeaderErr(vsexpectedVerifyErr), or- Guarding this block with something like
if tc.expectedErr != "" && err == nil && <we only expect header failure>,so the harness remains future‑proof when new error cases are introduced.
97-330: Per‑era tx‑body decoding is correct but highly duplicatedThe per‑era branches correctly:
- Decode the body CBOR into
[]interface{}.- Extract
txArray[0]as[]byteor hexstringwhere needed.- Call
cbor.Decodeinto the proper*TransactionBodyslice with error checks.- Construct a concrete
Blockwith appropriate header and witness types.Functionally this looks sound and aligns with the new typed‑block API, but the
txBodyBytesextraction andcbor.Decodepattern is copied almost verbatim across Allegra, Mary, Alonzo, Babbage, and Conway. If you touch this again, consider a small local helper like:func decodeTxBodyBytes(t *testing.T, v interface{}) []byte { t.Helper() switch b := v.(type) { case []byte: return b case string: txBodyBytes, err := hex.DecodeString(b) if err != nil { t.Fatalf("failed to hex decode transaction body string: %v", err) } return txBodyBytes default: t.Fatalf("unexpected type for tx body: %T", v) return nil } }and then use it in each era branch to keep the test shorter and easier to maintain.
ledger/verify_block.go (1)
182-184: Remove unreachable nil check.The switch statement above handles all block types and returns an error for unsupported types. If execution reaches line 182,
headeris guaranteed to be non-nil. This check is unreachable and can be removed.Apply this diff:
- if header == nil { - return 0, nil, 0, 0, errors.New("header returned empty") - } - slot := header.SlotNumber()ledger/verify_block_body.go (1)
32-38: Add documentation for the helper function.This generic helper is used throughout the file to prepare slices for indefinite-length CBOR encoding. A brief doc comment would clarify its purpose.
Apply this diff:
+// convertToAnySlice converts a typed slice to []any for use with CBOR indefinite-length encoding. func convertToAnySlice[T any](slice []T) []any { result := make([]any, len(slice)) for i, v := range slice { result[i] = v } return result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cbor/encode.go(2 hunks)connection_test.go(1 hunks)ledger/allegra/allegra.go(1 hunks)ledger/alonzo/alonzo.go(1 hunks)ledger/babbage/babbage.go(2 hunks)ledger/byron/byron.go(4 hunks)ledger/common/block.go(1 hunks)ledger/common/rewards.go(1 hunks)ledger/conway/conway.go(1 hunks)ledger/leios/leios.go(5 hunks)ledger/leios/leios_test.go(3 hunks)ledger/mary/mary.go(1 hunks)ledger/shelley/shelley.go(2 hunks)ledger/verify_block.go(1 hunks)ledger/verify_block_body.go(10 hunks)ledger/verify_block_test.go(2 hunks)ledger/verify_kes.go(2 hunks)ledger/verify_vrf.go(1 hunks)protocol/localstatequery/client_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ledger/common/rewards.go
- ledger/conway/conway.go
- ledger/common/block.go
- ledger/allegra/allegra.go
- ledger/babbage/babbage.go
- connection_test.go
- ledger/leios/leios.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
protocol/localstatequery/client_test.goledger/leios/leios_test.gocbor/encode.goledger/verify_block.goledger/verify_block_body.goledger/verify_block_test.go
🧬 Code graph analysis (5)
ledger/mary/mary.go (3)
ledger/mary.go (1)
MaryBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/shelley/shelley.go (3)
ledger/shelley.go (2)
ShelleyBlock(24-24)ShelleyBlockHeader(25-25)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/leios/leios_test.go (1)
ledger/leios/leios.go (1)
LeiosEndorserBlockBody(67-71)
ledger/alonzo/alonzo.go (3)
ledger/alonzo.go (1)
AlonzoBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/verify_block_test.go (2)
ledger/verify_block.go (1)
DetermineBlockType(40-109)ledger/block.go (1)
NewBlockHeaderFromCbor(55-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (17)
ledger/leios/leios_test.go (1)
123-128: LGTM!The updated construction correctly uses the new
Bodyfield structure to nestTxReferenceswithinLeiosEndorserBlockBody.protocol/localstatequery/client_test.go (1)
25-25: LGTM: Explicit alias improves clarity.The explicit
ouroborosalias clarifies the distinction between the main package andouroboros_mock, especially helpful since both packages are used together (e.g., lines 88-90).ledger/verify_vrf.go (1)
384-395: VerifyVrf wrapper looks correct and reuses existing VRF core safelyThe helper cleanly delegates to
VrfVerifyAndHashand usessubtle.ConstantTimeComparefor the final check; behavior and error propagation look sound.ledger/mary/mary.go (1)
153-155: MaryBlock.BlockBodyHash delegation is consistent with other erasDelegating to
Header().BlockBodyHash()matches the new common BlockHeader API and keeps Mary in sync with the cross‑era pattern.ledger/shelley/shelley.go (2)
143-145: ShelleyBlock.BlockBodyHash accessor matches the common Block APISimple delegation to the header is correct and keeps the Shelley block type aligned with other eras’ BlockBodyHash behavior.
216-218: ShelleyBlockHeader.BlockBodyHash correctly exposes the header body hashReturning
h.Body.BlockBodyHashis the expected implementation for the newBlockBodyHashrequirement on headers; no additional logic is needed.ledger/alonzo/alonzo.go (1)
192-194: AlonzoBlock.BlockBodyHash delegation is correct and idiomaticThis mirrors the pattern used for other eras by forwarding to the embedded Shelley header’s
BlockBodyHash, satisfying the common interface without new edge cases.ledger/byron/byron.go (2)
823-825: ByronMainBlock.BlockBodyHash delegation is consistent with other block typesForwarding to
Header().BlockBodyHash()keeps the Byron main block aligned with the common Block interface and the new header implementation.
892-894: ByronEpochBoundaryBlock.BlockBodyHash delegation is correctThe epoch boundary block now exposes its body hash via the header, matching the pattern across eras and enabling uniform use in verification.
ledger/verify_kes.go (1)
144-151: VerifyKes refactor correctly delegates to VerifyKesComponentsThe wiring keeps the original behavior: message is
CBOR(header.Body), and you pass through the same signature, hot key, KES period, slot, and slotsPerKesPeriod. This makes the KES logic reusable without changing semantics.ledger/verify_block_test.go (1)
334-352: VerifyBlock assertions now robustly surface unexpected errorsThe revised assertions after
VerifyBlocklook good:
tc.expectedErr == "" && err != nilis treated as a hard failure, so unexpected error paths can’t slip by.- When
expectedErris set, the test both checks validity and asserts thaterrcontains the expected substring.This is a solid harness for the new explicit error reporting in
VerifyBlock, and it closes the earlier gap where non‑nil errors could be ignored when only validity was asserted.ledger/verify_block.go (3)
259-264: Good overflow protection for slot value.The check ensures the slot value fits within int64 before passing to
MkInputVrf, preventing potential overflow issues in downstream computations.
284-290: CBOR encoding errors now properly handled.The previous review comment flagged ignored errors from
cbor.Encodecalls. This issue has been fixed - all era branches now check the error and return early with a descriptive message.
76-105: The review comment's concern is unfounded; proto versions 4 & 5 correctly appear in both header structures.The
DetermineBlockTypefunction properly handles block detection by using header body length as the primary discriminator (15 vs 10 fields), then proto version as secondary. Proto versions 4 (Mary) and 5 (Alonzo) legitimately appear in both the 15-field (Shelley-like) and 10-field (Babbage-like) branches because:
- The header structure (field count) is the era indicator
- The proto version disambiguates within that era structure
- During hard fork transitions, blocks produced with 10-field headers may carry earlier proto versions (e.g., proto 5 blocks in Babbage-era format)
The logic is intentional and correct, not an edge case requiring special handling. Existing tests validate this behavior.
Likely an incorrect or invalid review comment.
ledger/verify_block_body.go (3)
84-90: Error message correctly reports the derived hash.The previous review comment flagged that the error message printed the raw block data instead of the calculated hash. This has been fixed - line 87 now correctly encodes the calculated hash for the error message.
184-186: CBOR encoding consistently uses indefinite-length lists.The transition to
cbor.IndefLengthListwithconvertToAnySliceis applied uniformly for transaction bodies, witnesses, and the empty non-valid list. This aligns with the Cardano ledger specification referenced in comments.Also applies to: 197-199, 220-222
109-129: No issues found. IndefLengthMap correctly handles uint64 keys with deterministic ordering.The implementation encodes each key to CBOR bytes via
Encode(key), then sorts by the CBOR representation usingbytes.Compare, ensuring deterministic output regardless of the original key type. Themap[any]anywrapper forIndefLengthMapis intentional and correctly preserves deterministic encoding when uint64 keys are present.
|
@coderabbitai you definitely can range over int and this is the third time I've told you. Maybe add it to learnings to check the Go version before making this recommendation. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: Go added "range over integer" as a concise for-loop form (since Go 1.22). Usage and rules:
Sources: [1][2][3] References Citations:
@wolf31o2, I sincerely apologize for the repeated mistake. You're absolutely right—ranging over integers has been supported since Go 1.22. I should have remembered this from your previous corrections. The syntax Thank you for your patience in correcting me multiple times on this. ✏️ Learnings added
|
wolf31o2
force-pushed
the
refactor/verify-block-interface
branch
from
8d49b9c to
233a2d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ledger/verify_block.go (1)
188-270: VRF/KES wiring is now proper, but the CBOR‑missing body‑hash bypass is still a live security hole.Positives:
- VRF: you now derive
vrfMsgviaMkInputVrf, enforceslot <= math.MaxInt64, and callVerifyVrfwith era‑specificvrfKey/vrfResult. That removes the previous “always true” stub and makes VRF failure correctly fatal.- KES: per‑era
cbor.Encode(h.Body)with explicit error handling, followed byVerifyKesComponents, is exactly what we want; no more ignored encode errors and KES validity now actually feeds intoisValid.- Final verdict:
isValid = isBodyValid && vrfValid && kesValidensures any of the three checks failing will fail the block.Concern:
- Body hash verification still silently disables itself when
block.Cbor()is empty:
- For non‑Byron eras,
len(rawCbor) == 0yieldsisBodyValid = truewith a comment acknowledging this as a “known security issue”.- That means a caller that constructs a
Blockwithout stored CBOR (or with CBOR accidentally dropped) will completely skip body‑hash verification and still getisValiddriven only by VRF/KES.Given this function is the main verification entrypoint, keeping an unconditional bypass here is risky even if tests rely on it.
I’d strongly suggest tightening this up so production code can’t accidentally take the bypass path. For example:
- Treat
len(rawCbor) == 0as an error for all non‑Byron eras and make tests use a dedicated test helper (or a test‑only verifier) instead; or- Gate the bypass behind an explicit, test‑only signal (env var, build tag, or a parameter) so normal callers can’t get
isBodyValid = truewithout CBOR.Because a previous review already raised this body‑hash‑bypass issue, this is reiterating an existing concern rather than a brand‑new one.
If you’d like, I can sketch a minimal change that keeps tests working (e.g., optional
allowMissingCbor boolargument or test‑only wrapper) while making the defaultVerifyBlockpath fail hard when CBOR is missing.Also applies to: 272-365, 372-413
🧹 Nitpick comments (3)
ledger/verify_kes.go (1)
144-179: Good refactoring with improved error handling.The extraction of VerifyKesComponents makes KES verification reusable and adds valuable input validation (slotsPerKesPeriod and signature length checks). However, the hardcoded KES depth of 6 and signature length of 448 bytes are tightly coupled to Cardano's current protocol parameters.
Consider documenting why the KES depth is fixed at 6 levels, as this is a protocol constant that may not be immediately obvious to future maintainers. For example:
const ( // CardanoKesDepth is the fixed KES tree depth used in Cardano's consensus protocol CardanoKesDepth = 6 // CardanoKesSignatureSize is the expected KES signature size: 64 + depth*64 CardanoKesSignatureSize = 448 )Then reference these constants in both VerifyKesComponents and verifySignedKES for consistency and clarity.
ledger/leios/leios.go (1)
17-20: Leios BlockBodyHash wiring is consistent, but consider defensive checks around nil headers.This all lines up nicely with the new
BlockHeader.BlockBodyHash()API:
LeiosEndorserBlockBody.BlockBodyHash()hashes the CBOR encoding of the body and panics on encode error, which is reasonable for an experimental, structurally well‑known type.LeiosBlockHeader.BlockBodyHash()delegates toh.Body.BlockBodyHash, andLeiosRankingBlock.BlockBodyHash()delegates tob.Header().BlockBodyHash(), keeping the access path uniform across Leios ranking blocks.LeiosEndorserBlock.BlockBodyHash()usesBody.BlockBodyHash()whenBodyis non‑nil and otherwise returns a zero hash, which aligns with the rest of the placeholder behavior on endorser blocks.Two minor robustness thoughts (given the “experimental/OK to skip validation” note):
- For
LeiosRankingBlock.BlockBodyHash(), a nilBlockHeaderwould cause a panic; if there’s any risk of partially constructed ranking blocks leaking into general verification, a simple nil‑check with a zero hash (or a loud panic) would make the failure mode clearer.- For endorser blocks, you might eventually want to distinguish “no body” from “empty body” in the hash result (e.g., by failing early) once Leios validation stops being explicitly skippable.
These are nice‑to‑have hardening tweaks rather than blockers, especially while Leios stays behind an experimental flag.
Based on learnings, using
cbor.Encodedirectly here (instead of any stored CBOR pattern) matches the repo’s preferred CBOR round‑trip approach.Also applies to: 67-84, 136-138, 194-199, 287-289
ledger/verify_block_test.go (1)
115-287: Harden CBOR tx shape handling and de‑duplicate per‑era loopsAll era branches assume
txis[]interface{}and thattxArray[0]has the expected raw body representation. If CBOR decoding regresses or fixtures drift, these unchecked type assertions will panic with opaque messages.You can make failures clearer and reduce duplication by:
- Adding explicit
okchecks on the slice assertion, and- Reusing
decodeTxBodyBytesin the Shelley branch as well, so CBOR body extraction is uniform.For example:
- for i, tx := range txs { - txArray := tx.([]interface{}) - if _, err := cbor.Decode(txArray[0].([]byte), &transactionBodies[i]); err != nil { + for i, tx := range txs { + txArray, ok := tx.([]interface{}) + if !ok { + t.Fatalf("unexpected Shelley tx element type %T", tx) + } + txBodyBytes := decodeTxBodyBytes(t, txArray[0]) + if _, err := cbor.Decode(txBodyBytes, &transactionBodies[i]); err != nil { t.Fatalf( "failed to decode Shelley transaction body %d: %v", i, err, ) } }You could factor a small helper that takes
txsand a decode target slice to share this pattern across all non‑Byron eras, but that’s an optional clean‑up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
connection_test.go(1 hunks)ledger/allegra/allegra.go(1 hunks)ledger/alonzo/alonzo.go(1 hunks)ledger/babbage/babbage.go(2 hunks)ledger/byron/byron.go(4 hunks)ledger/common/block.go(1 hunks)ledger/common/rewards.go(1 hunks)ledger/conway/conway.go(1 hunks)ledger/leios/leios.go(4 hunks)ledger/mary/mary.go(1 hunks)ledger/shelley/shelley.go(2 hunks)ledger/verify_block.go(1 hunks)ledger/verify_block_body.go(10 hunks)ledger/verify_block_test.go(2 hunks)ledger/verify_kes.go(2 hunks)ledger/verify_vrf.go(1 hunks)protocol/localstatequery/client_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ledger/common/rewards.go
- protocol/localstatequery/client_test.go
- connection_test.go
- ledger/alonzo/alonzo.go
- ledger/verify_vrf.go
- ledger/mary/mary.go
- ledger/common/block.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
ledger/verify_block.goledger/verify_block_test.goledger/leios/leios.goledger/verify_block_body.go
🧬 Code graph analysis (8)
ledger/conway/conway.go (3)
ledger/conway.go (1)
ConwayBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/verify_block.go (4)
ledger/common/block.go (2)
BlockHeader(13-23)Block(5-11)ledger/common/vrf.go (1)
VrfResult(21-25)ledger/verify_vrf.go (2)
MkInputVrf(39-55)VerifyVrf(384-395)ledger/verify_kes.go (1)
VerifyKesComponents(154-179)
ledger/shelley/shelley.go (3)
ledger/shelley.go (2)
ShelleyBlock(24-24)ShelleyBlockHeader(25-25)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/byron/byron.go (3)
ledger/byron.go (2)
ByronMainBlock(25-25)ByronEpochBoundaryBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/allegra/allegra.go (3)
ledger/allegra.go (1)
AllegraBlock(24-24)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/babbage/babbage.go (3)
ledger/babbage.go (2)
BabbageBlock(24-24)BabbageBlockHeader(25-25)ledger/common/common.go (1)
Blake2b256(41-41)ledger/compat.go (1)
Blake2b256(27-27)
ledger/leios/leios.go (2)
cbor/encode.go (1)
Encode(28-41)ledger/leios.go (2)
LeiosBlockHeader(24-24)LeiosRankingBlock(26-26)
ledger/verify_block_body.go (4)
cbor/encode.go (3)
Encode(28-41)IndefLengthList(93-93)IndefLengthMap(137-137)ledger/common/certs.go (3)
PoolRegistrationCertificate(435-448)PoolRetirementCertificate(617-623)PoolKeyHash(336-336)ledger/compat.go (3)
PoolRegistrationCertificate(67-67)PoolRetirementCertificate(66-66)NewBlake2b224(30-32)ledger/common/common.go (1)
NewBlake2b224(89-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (17)
ledger/allegra/allegra.go (1)
139-141: LGTM!The delegation pattern is clean and consistent with other era implementations. This provides uniform access to the block body hash across all block types.
ledger/conway/conway.go (1)
152-154: LGTM!Consistent delegation to the header's BlockBodyHash() method. This aligns with the pattern established across all era-specific block implementations.
ledger/babbage/babbage.go (2)
152-154: LGTM!The block-level accessor correctly delegates to the header, maintaining consistency with other era implementations.
235-237: LGTM!Direct access to the body hash from the header body structure is straightforward and efficient.
ledger/shelley/shelley.go (2)
143-145: LGTM!Consistent delegation pattern that provides uniform block body hash access across all Shelley-era blocks.
216-218: LGTM!Direct field access provides efficient retrieval of the block body hash from the header.
ledger/byron/byron.go (4)
139-148: LGTM!The type assertion with zero-hash fallback is a safe approach for Byron's
any-typed BodyProof field. This prevents DoS attacks from malformed input while allowing validation to fail gracefully.
745-754: LGTM!Consistent implementation with ByronMainBlockHeader - the safe type assertion pattern appropriately handles Byron's legacy block structure.
823-825: LGTM!Standard delegation to the header's BlockBodyHash() method, consistent with all other era implementations.
892-894: LGTM!Consistent delegation pattern for epoch boundary blocks.
ledger/verify_block.go (1)
39-109: Era detection and header parsing look sound, but future-era brittleness is something to watch.The
DetermineBlockType+parseHeadersplit is clean and the error reporting is useful. One thing to keep in mind is that this logic is tightly coupled to the current header body shapes (lenBody == 15vslenBody == 10and specificprotoMajorvalues). Any future era or header-body tweak will funnel through here and fail with “unknown header body length” or “unknown proto major” until this function is updated.That’s fine for now, but it’s worth treating
DetermineBlockTypeas part of the “era upgrade surface” and making sure new eras or proto-major transitions update this function and its tests in lockstep.If you haven’t already, I’d recommend a quick grep to ensure tests cover both the 15‑field and 10‑field paths with the proto majors you care about (Shelley-like and Babbage-like).
Also applies to: 111-186
ledger/verify_block_body.go (3)
32-38: CBOR list encoding helpers look good and align with deterministic-encoding goals.The new
convertToAnySlice+ usage ofcbor.IndefLengthList(...)in:
EncodeCborTxSeq(for[]uint), andCalculateBlockBodyHash(for[]cbor.RawMessagetx bodies and witnesses, and the empty non-valid list)keeps the code simple while ensuring:
- Lists with length ≤
MAX_LIST_LENGTH_CBORuse normal definite-length encoding.- Longer lists flip to indefinite-length encoding with elements materialized as
[]any, which is what the CBOR encoder expects forIndefLengthList.Given
cbor.Encodeis already configured withSortCoreDeterministic, this approach preserves deterministic encodings without relying on stored CBOR blobs, matching the repository’s CBOR testing strategy.Based on learnings, this is a good example of using deterministic
cbor.Encodedirectly instead of introducing any stored-CBOR shortcuts.Also applies to: 93-101, 184-200, 219-222
108-129: Map encoding via encodeAuxData correctly distinguishes definite vs. indefinite-length metadata.
encodeAuxDatanicely separates:
- Empty metadata → empty
map[uint64]anyencoded definitively.- Small metadata (
len <= MAX_LIST_LENGTH_CBOR) → standardmap[uint64]anyCBOR encoding.- Large metadata →
cbor.IndefLengthMap(map[any]any)for indefinite-length maps.Then
CalculateBlockBodyHashfeeds the result intoblake2b.Sum256, aligning with how the othertxSeq*components are treated. Since encoding goes through the sharedcbor.Encodepath, key ordering remains deterministic even for indefinite-length maps.This is a solid cleanup compared to hand-rolled map encoding, and it keeps the AuxData handling centralized.
Also applies to: 209-231
84-91: Body-hash mismatch error is now accurate; pool certificate plumbing looks correct.
VerifyBlockBody: the mismatch error now includeshex.EncodeToString(calculateBlockBodyHashByte[:])instead of the rawdata, so callers can see the derived hash that failed comparison. This directly addresses the earlier comment about misleading error text.GetBlockOutput: switching the accumulator types to[]common.PoolRegistrationCertificateand[]common.PoolRetirementCertificatewhile still type-switching on*PoolRegistrationCertificate/*PoolRetirementCertificate(the compat aliases) keeps the external API on public common types without changing the on-ledger representation.GetListRegisCertPoolId/GetListUnregisCertPoolId: taking slices ofcommon.PoolRegistrationCertificate/common.PoolRetirementCertificateand deriving pool IDs viaNewBlake2b224(cert.Operator[:])andNewBlake2b224(cert.PoolKeyHash[:])matches the underlying type definitions and avoids reintroducing local alias structs.All of this hangs together cleanly and simplifies the public interface around pool certificates without changing semantics.
If you haven’t already, it’s worth adding/expanding tests that assert the exact pool IDs produced from known certs, just to guard against future changes in the underlying
PoolKeyHash/Operatorlayouts.Also applies to: 268-318, 362-381
ledger/verify_block_test.go (3)
17-33: decodeTxBodyBytes helper is well‑scoped and robustUsing
testing.T.Helper()plus strict type checking for[]bytevsstring(with hex decode and fatal on mismatch) is a clean way to support mixed CBOR representations in tests. No changes needed here.
80-114: Header decoding and block‑type detection flow looks solidDecoding header hex once, feeding the bytes to
DetermineBlockType, and short‑circuiting when an expectedtc.expectedErris returned yields clear, deterministic behaviour. The additional fatal on unexpected determine‑type errors is appropriate for a test harness.
292-310: VerifyBlock invocation and error assertions align with new APICalling
VerifyBlockwith the era‑specificBlock,Eta0, andSpk, then (a) asserting validity, (b) matchingtc.expectedErrsubstrings when set, and (c) treating any non‑nil error as fatal whenexpectedErr == ""ensures unexpected error paths are not silently accepted. This matches the intent of the refactor without overcomplicating the test logic.
| // as per the Cardano Byron era specification | ||
| if bodyProof, ok := h.BodyProof.(common.Blake2b256); ok { | ||
| return bodyProof | ||
| } |
There was a problem hiding this comment.
I don't think this is actually going to work. The BodyProof field is type any, the CBOR decoder won't use common.Blake2b256 on its own in this case, and we do not explicitly assign to that field anywhere.
| // as per the Cardano Byron era specification | ||
| if bodyProof, ok := h.BodyProof.(common.Blake2b256); ok { | ||
| return bodyProof | ||
| } |
There was a problem hiding this comment.
I don't think this is actually going to work. The BodyProof field is type any, the CBOR decoder won't use common.Blake2b256 on its own in this case, and we do not explicitly assign to that field anywhere.
ledger/verify_block.go
Outdated
| headerCborByte := block.Header().Cbor() | ||
|
|
||
| // Parse header and extract block type, header, slot, and block number | ||
| blockType, specificHeader, slot, blockNo, err := parseHeader(headerCborByte) |
There was a problem hiding this comment.
This whole thing seems entirely unnecessary. It's grabbing the CBOR from the already-decoded header and passing it through a series of functions that use questionable tricks to return information that we already have readily available in our already-decoded header.
ledger/verify_block.go
Outdated
| var kesValid bool | ||
| var vrfResult common.VrfResult | ||
| var vrfKey []byte | ||
| switch blockType { |
There was a problem hiding this comment.
We can do a type switch on the header here instead:
switch h := block.Header().(type) {
case *shelley.ShelleyBlockHeader:
...
}
ledger/verify_block.go
Outdated
| var signature []byte | ||
| var hotVkey []byte | ||
| var kesPeriod uint64 | ||
| switch blockType { |
There was a problem hiding this comment.
Use a type switch on block.Header() as noted above
ledger/verify_block.go
Outdated
| // Verify block body hash | ||
| expectedBodyHash := block.BlockBodyHash() | ||
| isBodyValid := true | ||
| if block.Era().Name != "Byron" { |
There was a problem hiding this comment.
This should be a comparison to a pre-defined value for Byron rather than a string. Something like this:
if block.Era() != byron.EraByron {
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
refactor/verify-block-interface
branch
from
233a2d6 to
916471c
Compare
Summary by cubic
Refactored block verification to use typed ledger blocks with era-aware headers and body hash validation. This improves readability, reuses KES/VRF checks, and surfaces clearer errors.
Refactors
Migration
Written for commit 916471c. Summary will update automatically on new commits.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.