refactor(ledger): put error at end of return #1242
Conversation
📝 WalkthroughWalkthroughThe PR changes Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20-30 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ledger/verify_block_test.go (4)
53-56: Update assertions to check success-path invariants (err, vrf, numbers) and tighten failure handlingReturn-order fix is correct. Strengthen the test so valid cases assert nil error and non-empty metadata; fail fast on mismatches.
Apply:
- isValid, _, _, _, err := VerifyBlock(tc.blockHexCbor) - if tc.expectedValid != isValid { - t.Errorf("unexpected error: %v", err) - } + isValid, vrfHex, blockNo, slotNo, err := VerifyBlock(tc.blockHexCbor) + if tc.expectedValid { + if err != nil { + t.Fatalf("expected valid with no error, got: %v", err) + } + if !isValid { + t.Fatalf("expected valid block") + } + if vrfHex == "" || blockNo == 0 || slotNo == 0 { + t.Fatalf("unexpected empty metadata: vrf=%q blockNo=%d slotNo=%d", vrfHex, blockNo, slotNo) + } + } else { + if isValid { + t.Fatalf("expected invalid block") + } + // err may be nil for some invalids (e.g., boolean checks). If you set tc.expectedErr, assert it below. + }
3-5: Use or remove expectedErr; if used, import strings and assert message substringsThe field exists but isn’t used. Either drop it or assert error text when provided.
Option A — assert when set:
import ( - "testing" + "strings" + "testing" ) @@ } else { if isValid { t.Fatalf("expected invalid block") } + if tc.expectedErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.expectedErr) { + t.Fatalf("expected error containing %q, got: %v", tc.expectedErr, err) + } + } }Option B — remove the unused field:
testCases := []struct { name string blockHexCbor BlockHexCbor expectedValid bool - expectedErr string }{Also applies to: 7-13, 53-56
7-7: Rename test to reflect scope (it verifies the whole block, not just the body)Suggest: TestVerifyBlock. Keeps tests discoverable/accurate.
-func TestVerifyBlockBody(t *testing.T) { +func TestVerifyBlock(t *testing.T) {
17-47: Consider moving giant CBOR literals to testdata fixturesLoading from testdata or small helpers will reduce noise and improve readability/DIFFs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/verify_block.go(2 hunks)ledger/verify_block_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/verify_block.go (7)
ledger/block.go (2)
BlockHeader(26-26)NewBlockHeaderFromCbor(55-78)ledger/babbage/babbage.go (2)
NewBabbageBlockHeaderFromCbor(1000-1006)BabbageBlockHeader(152-158)ledger/conway/conway.go (1)
ConwayBlockHeader(152-154)ledger/verify_kes.go (1)
VerifyKes(129-149)ledger/common/vrf.go (1)
VrfResult(21-25)ledger/verify_vrf.go (2)
MkInputVrf(39-55)VrfVerifyAndHash(57-83)ledger/verify_block_body.go (1)
VerifyBlockBody(52-91)
ledger/verify_block_test.go (1)
ledger/verify_block.go (1)
VerifyBlock(31-143)
🔇 Additional comments (5)
ledger/verify_block_test.go (1)
1-59: No legacy VerifyBlock callers — error is returned last.
VerifyBlock's signature is (bool, string, uint64, uint64, error) (ledger/verify_block.go:31) and callers bind err last (e.g., ledger/verify_block_test.go:53: isValid, _, _, _, err := VerifyBlock(...)).ledger/verify_block.go (4)
40-53: LGTM! Proper validation for Leios blocks.The validation logic correctly rejects negative flags and unsupported Leios block types before proceeding with verification. The integer conversion at line 47 is safe since the non-negative check precedes it.
84-94: LGTM! Proper handling of Babbage and Conway headers.The type assertion logic correctly handles both
BabbageBlockHeaderandConwayBlockHeader(which embedsBabbageBlockHeader). The error message includes the actual type for debugging, which is helpful.
96-142: LGTM! Verification logic correctly updated.All KES, VRF, and block body verification calls now use
babbageHeader, and all return statements correctly follow the new signature with error as the final value.
31-31: Function signature change verified and correctly implemented.The refactored signature correctly places
erroras the final return value, aligning with Go best practices. Verification confirms that all callers, including the test file atledger/verify_block_test.go:53, have been updated to handle the new return order correctly:isValid, _, _, _, err := VerifyBlock(tc.blockHexCbor)The unpacking order matches the new signature perfectly.
ledger/verify_block.go
Outdated
| // For backward compatibility, try to decode as Babbage header first | ||
| // This allows the function to work with different eras that have compatible structures | ||
| headerCborByte, headerDecodeError := hex.DecodeString(headerCborHex) | ||
| if headerDecodeError != nil { | ||
| return fmt.Errorf( | ||
| return false, "", 0, 0, fmt.Errorf( | ||
| "VerifyBlock: headerCborByte decode error, %v", | ||
| headerDecodeError.Error(), | ||
| ), false, "", 0, 0 | ||
| ) | ||
| } | ||
| header, headerUnmarshalError := NewBabbageBlockHeaderFromCbor( | ||
| headerCborByte, | ||
| ) | ||
| var header BlockHeader | ||
| var headerUnmarshalError error | ||
| header, headerUnmarshalError = NewBabbageBlockHeaderFromCbor(headerCborByte) | ||
| if headerUnmarshalError != nil { | ||
| return fmt.Errorf( | ||
| "VerifyBlock: header unmarshal error, %v", | ||
| headerUnmarshalError.Error(), | ||
| ), false, "", 0, 0 | ||
| // If Babbage decoding fails, try the era-specific decoder | ||
| header, headerUnmarshalError = NewBlockHeaderFromCbor( | ||
| blockType, | ||
| headerCborByte, | ||
| ) | ||
| if headerUnmarshalError != nil { | ||
| return false, "", 0, 0, fmt.Errorf( | ||
| "VerifyBlock: header unmarshal error, %v", | ||
| headerUnmarshalError.Error(), | ||
| ) | ||
| } | ||
| } | ||
| if header == nil { | ||
| return errors.New("VerifyBlock: header returned empty"), false, "", 0, 0 | ||
| return false, "", 0, 0, errors.New("VerifyBlock: header returned empty") | ||
| } |
There was a problem hiding this comment.
Clarify backward compatibility scope or remove pre-Babbage fallback.
The decoding logic attempts era-specific fallback (lines 69-72) for pre-Babbage blocks, but the subsequent code (lines 86-94) requires Babbage-compatible headers and explicitly rejects non-Babbage types. This means blocks from Shelley, Allegra, Mary, or Alonzo eras will decode successfully via NewBlockHeaderFromCbor but then fail at the type assertion, resulting in a less clear error message.
Consider one of these approaches:
- Remove the fallback if only Babbage+ blocks are supported: validate
blockTypeupfront and return an error for pre-Babbage eras before attempting any decoding. - Support pre-Babbage eras: extend the verification logic to handle header types from earlier eras.
- Improve error messaging: if the fallback is intentionally kept for robustness, ensure the error at line 92 clearly indicates that pre-Babbage blocks aren't supported.
Example of upfront validation approach:
+ // VerifyBlock only supports Babbage and later eras
+ if blockType < BlockTypeBabbage && blockType != BlockTypeConway {
+ return false, "", 0, 0, fmt.Errorf(
+ "VerifyBlock: block verification only supported for Babbage and later eras, got block type: %d",
+ blockType,
+ )
+ }
+
// For backward compatibility, try to decode as Babbage header firstCommittable suggestion skipped: line range outside the PR's diff.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
refactor/verify-block-signature
branch
from
e2d9edb to
87bd6cc
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: Chris Gianelloni wolf31o2@blinklabs.io
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests