-
Notifications
You must be signed in to change notification settings - Fork 21
fix: update leios endorser block shape #1259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
📝 WalkthroughWalkthroughThe pull request removes the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ledger/leios/leios_test.go (1)
162-200: Consider adding byte-identical encoding verification.The current test verifies that field values are preserved through CBOR round-trips, which is good. However, consider also verifying that re-encoding the decoded block produces byte-identical CBOR:
Add this verification after line 180:
if _, err := cbor.Decode(cborData, &decoded); err != nil { t.Fatalf("Failed to decode LeiosEndorserBlock: %v", err) } + + // Verify byte-identical re-encoding + cborData2, err := cbor.Encode(&decoded) + if err != nil { + t.Fatalf("Failed to re-encode LeiosEndorserBlock: %v", err) + } + if !bytes.Equal(cborData, cborData2) { + t.Error("CBOR encoding is not byte-identical after round-trip") + }Note: You'll need to import
bytesat the top of the file.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ledger/common/tx.go(0 hunks)ledger/leios/leios.go(1 hunks)ledger/leios/leios_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- ledger/common/tx.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/leios/leios_test.go
🧬 Code graph analysis (1)
ledger/leios/leios_test.go (4)
ledger/leios/leios.go (13)
EraLeios(39-42)EraIdLeios(28-28)EraNameLeios(29-29)BlockTypeLeiosRanking(31-31)BlockTypeLeiosEndorser(32-32)BlockHeaderTypeLeios(34-34)TxTypeLeios(36-36)LeiosEndorserBlock(63-69)LeiosEndorserBlock(114-116)LeiosRankingBlock(165-169)LeiosRankingBlock(171-173)LeiosBlockHeader(48-54)LeiosBlockHeaderBody(56-61)ledger/babbage/babbage.go (1)
BabbageBlockHeaderBody(160-172)cbor/encode.go (1)
Encode(27-40)cbor/decode.go (1)
Decode(29-44)
⏰ 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 (5)
ledger/leios/leios.go (1)
68-68: LGTM! Field type change aligns with the updated endorser block specification.The change from a slice of structs to a direct hash-to-size map simplifies the data structure and is appropriate for storing transaction references. CBOR encoding will handle the map deterministically.
ledger/leios/leios_test.go (4)
25-64: LGTM! Constant validation tests are thorough.These tests verify that the Leios era and block type constants are correctly defined.
66-119: LGTM! Type() and Era() method tests are correct.These tests properly validate that block types and eras are returned correctly by their respective methods.
139-160: Same Hash() verification concern applies here.This test has the same potential issue as
TestLeiosEndorserBlock_Hash- callingHash()on a directly-created header that hasn't been encoded may not work correctly ifCbor()returns empty bytes.
121-137: LeiosEndorserBlock.Hash() and LeiosBlockHeader.Hash() hash empty bytes instead of block content when blocks are created programmatically.Both
LeiosEndorserBlockandLeiosBlockHeaderinherit fromDecodeStoreCborand callCbor()in theirHash()methods (lines 135-140 and 82-87, respectively). For blocks created directly (not decoded from CBOR),Cbor()returns nil, causingHash()to computeBlake2b256Hash(nil)instead of hashing the actual block content.Transaction types in other ledger eras (Mary, Shelley, Conway, etc.) implement
Cbor()with a fallback that generates CBOR on-the-fly when stored CBOR is unavailable.LeiosEndorserBlockandLeiosBlockHeaderlack this fallback.The test passes because
Blake2b256Hash(nil)produces a non-zero hash and is consistent (cached), but it validates incorrect behavior. To fix, implement aCbor()method override in both types that falls back to encoding when stored CBOR is unavailable, following the pattern used by Transaction types.⛔ Skipped due to 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.
Summary by CodeRabbit
Refactor
Tests