Skip to content

Conversation

@wolf31o2
Copy link
Member

@wolf31o2 wolf31o2 commented Nov 18, 2025

Summary by CodeRabbit

Refactor

  • Refined and optimized internal data structures and handling for Leios endorser blocks

Tests

  • Added comprehensive test coverage for Leios era implementation including validation of era identifiers and block type constants, verification of core method operations (Type, Era, Hash), and complete serialization integrity and data preservation testing

Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
@wolf31o2 wolf31o2 requested a review from a team as a code owner November 18, 2025 19:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 18, 2025

📝 Walkthrough

Walkthrough

The pull request removes the TxReference struct type from the common package and refactors its usage. The LeiosEndorserBlock.TxReferences field is changed from a slice of TxReference structs to a map with Blake2b256 keys and uint16 values, representing transaction hashes mapped directly to their sizes. A comprehensive test file is added to validate Leios block constants, methods (Type, Era, Hash), and CBOR round-trip serialization of the updated LeiosEndorserBlock structure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify that the map representation (map[common.Blake2b256]uint16) correctly replaces all prior usages of the TxReference slice and that serialization behavior is preserved
  • Validate that the CBOR round-trip test accurately exercises the updated TxReferences field with the new map structure
  • Check the test file comprehensively covers the constants and methods for the updated block types
  • Ensure no other code paths outside this diff still reference the removed TxReference type

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: updating the LeiosEndorserBlock structure by changing TxReferences from a slice to a map, which directly relates to the block's shape/structure.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/leios-eb-shape

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 bytes at the top of the file.

Based on learnings.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ec28f4 and b58cbfc.

📒 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 - calling Hash() on a directly-created header that hasn't been encoded may not work correctly if Cbor() returns empty bytes.


121-137: LeiosEndorserBlock.Hash() and LeiosBlockHeader.Hash() hash empty bytes instead of block content when blocks are created programmatically.

Both LeiosEndorserBlock and LeiosBlockHeader inherit from DecodeStoreCbor and call Cbor() in their Hash() methods (lines 135-140 and 82-87, respectively). For blocks created directly (not decoded from CBOR), Cbor() returns nil, causing Hash() to compute Blake2b256Hash(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. LeiosEndorserBlock and LeiosBlockHeader lack 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 a Cbor() 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.

@wolf31o2 wolf31o2 merged commit df73e42 into main Nov 18, 2025
10 checks passed
@wolf31o2 wolf31o2 deleted the fix/leios-eb-shape branch November 18, 2025 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants