fix: resolve protocol parameter type errors across era transitions #1006
Conversation
📝 WalkthroughWalkthroughThis PR fixes protocol-parameter error handling, adds era-aware initialization, and introduces test helpers. Key changes: correct error checks and add a nil-guard in database/pparams.go; always compute certificate deposits and treat the specific "pparams are not expected type" error as a historical-era mismatch in ledger/certs.go; add GetEraById and comprehensive unit tests in ledger/eras/eras_test.go; and refactor ledger/state.go to bootstrap genesis protocol parameters across eras, change validation gating, and expose multiple test-facing setters/getters and loaders. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45-60 minutes
Pre-merge checks and finishing touches✅ Passed checks (4 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 |
wolf31o2
force-pushed
the
fix/pparam-loading
branch
from
0041ef0 to
75a00da
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
database/pparams.go(3 hunks)ledger/certs.go(1 hunks)ledger/eras/eras.go(1 hunks)ledger/eras/eras_test.go(1 hunks)ledger/state.go(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state.go
🧬 Code graph analysis (2)
ledger/eras/eras_test.go (8)
ledger/eras/eras.go (3)
EraDesc(23-35)GetEraById(63-70)Eras(37-45)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/allegra.go (1)
AllegraEraDesc(30-41)ledger/eras/mary.go (1)
MaryEraDesc(30-41)ledger/eras/alonzo.go (1)
AlonzoEraDesc(30-41)ledger/eras/babbage.go (1)
BabbageEraDesc(30-41)ledger/eras/conway.go (1)
ConwayEraDesc(33-45)
ledger/state.go (9)
ledger/eras/shelley.go (2)
HardForkShelley(81-93)ShelleyEraDesc(30-41)config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/eras/allegra.go (2)
AllegraEraDesc(30-41)HardForkAllegra(81-94)ledger/eras/mary.go (2)
MaryEraDesc(30-41)HardForkMary(81-94)ledger/eras/alonzo.go (2)
AlonzoEraDesc(30-41)HardForkAlonzo(81-98)ledger/eras/babbage.go (2)
BabbageEraDesc(30-41)HardForkBabbage(81-94)ledger/eras/conway.go (2)
ConwayEraDesc(33-45)HardForkConway(85-102)ledger/eras/eras.go (3)
GetEraById(63-70)Eras(37-45)EraDesc(23-35)database/models/epoch.go (2)
Epoch(17-27)Epoch(29-31)
⏰ 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). (3)
- GitHub Check: nilaway
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
database/pparams.go (1)
32-45: Good catch on bubblingppErr.Swapping to the metadata error ensures we stop on real retrieval failures instead of marching forward with empty results. Nice.
ledger/eras/eras.go (1)
61-69: New lookup helper looks solid.Linear search over the small static slice is fine and returning the slice element keeps us aligned with future updates.
ledger/eras/eras_test.go (1)
23-247: Appreciate the exhaustive coverage.These table-driven tests hit every supported era and the gap cases, so we’ll notice immediately if new IDs aren’t wired into the lookup.
ledger/state.go (1)
925-930: Nice safeguard on keeping bootstrapped params.Only overwriting when the DB actually returns something (or when we truly have nothing) keeps the genesis-derived parameters intact.
wolf31o2
force-pushed
the
fix/pparam-loading
branch
from
75a00da to
2508d77
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ledger/eras/eras_test.go (1)
138-181: Consider testing all eras for consistency, not just those with matching indices.The test currently only verifies IDs 0-4, which happen to match their array indices. While this is noted in the comment (line 141), the test could be more comprehensive by verifying that all eras in the
Erasarray can be retrieved viaGetEraByIdusing their actual ID. The fourth test (TestGetEraById_AllKnownEras) already does this more thoroughly, which somewhat duplicates this test's purpose.Consider either:
- Expanding this test to include all eras (removing the
validArrayIndicesfilter), or- Removing this test since
TestGetEraById_AllKnownErasprovides more comprehensive coverage of the same consistency check.ledger/state.go (1)
933-1067: Consider refactoring to reduce duplication in the era transition chain.The function correctly bootstraps protocol parameters for each era, but there's significant code duplication as each era (Allegra through Conway) repeats the full chain of previous era transitions. While this works correctly, it could be refactored to build the chain iteratively:
func (ls *LedgerState) loadGenesisProtocolParameters() (lcommon.ProtocolParameters, error) { // Start with Shelley (which Byron also uses as base) currentPParams, err := eras.HardForkShelley(ls.config.CardanoNodeConfig, nil) if err != nil { return nil, err } // If target era is Byron or Shelley, return early if ls.currentEra.Id <= eras.ShelleyEraDesc.Id { return currentPParams, nil } // Transition through each era up to the target eraTransitions := []struct { id uint fn func(*cardano.CardanoNodeConfig, lcommon.ProtocolParameters) (lcommon.ProtocolParameters, error) }{ {eras.AllegraEraDesc.Id, eras.HardForkAllegra}, {eras.MaryEraDesc.Id, eras.HardForkMary}, {eras.AlonzoEraDesc.Id, eras.HardForkAlonzo}, {eras.BabbageEraDesc.Id, eras.HardForkBabbage}, {eras.ConwayEraDesc.Id, eras.HardForkConway}, } for _, transition := range eraTransitions { if ls.currentEra.Id < transition.id { break } currentPParams, err = transition.fn(ls.config.CardanoNodeConfig, currentPParams) if err != nil { return nil, err } } return currentPParams, nil }This approach:
- Eliminates duplication by building the chain once
- Makes it easier to add new eras in the future
- Maintains the same correctness guarantees
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
database/pparams.go(3 hunks)ledger/certs.go(1 hunks)ledger/eras/eras.go(1 hunks)ledger/eras/eras_test.go(1 hunks)ledger/state.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- database/pparams.go
- ledger/certs.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/eras/eras_test.goledger/state.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state.go
🧬 Code graph analysis (2)
ledger/eras/eras_test.go (8)
ledger/eras/eras.go (3)
EraDesc(23-35)GetEraById(63-70)Eras(37-45)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/allegra.go (1)
AllegraEraDesc(30-41)ledger/eras/mary.go (1)
MaryEraDesc(30-41)ledger/eras/alonzo.go (1)
AlonzoEraDesc(30-41)ledger/eras/babbage.go (1)
BabbageEraDesc(30-41)ledger/eras/conway.go (1)
ConwayEraDesc(33-45)
ledger/state.go (9)
ledger/eras/shelley.go (2)
HardForkShelley(81-93)ShelleyEraDesc(30-41)config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/eras/allegra.go (2)
AllegraEraDesc(30-41)HardForkAllegra(81-94)ledger/eras/mary.go (2)
MaryEraDesc(30-41)HardForkMary(81-94)ledger/eras/alonzo.go (2)
AlonzoEraDesc(30-41)HardForkAlonzo(81-98)ledger/eras/babbage.go (2)
BabbageEraDesc(30-41)HardForkBabbage(81-94)ledger/eras/conway.go (2)
ConwayEraDesc(33-45)HardForkConway(85-102)ledger/eras/eras.go (3)
GetEraById(63-70)Eras(37-45)EraDesc(23-35)database/models/epoch.go (2)
Epoch(17-27)Epoch(29-31)
⏰ 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). (4)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
ledger/eras/eras.go (1)
61-70: LGTM!The function implementation is clean and correct. The linear search is appropriate given the small size of the
Erasslice, and returning a pointer allows the caller to distinguish between a found era and a missing one.ledger/eras/eras_test.go (3)
23-136: LGTM!The test is well-structured with comprehensive coverage of both valid era IDs (0-6) and invalid cases (non-existent IDs). The table-driven approach makes it easy to verify all cases.
183-202: LGTM!Good edge case coverage ensuring the function handles non-existent era IDs gracefully without panicking.
204-247: LGTM!Excellent comprehensive test that verifies all eras in the
Erasarray can be correctly retrieved by their ID, and that the pointer equality check (line 239) ensures the same descriptor instance is returned.ledger/state.go (4)
715-717: LGTM!The validation gating correctly ensures blocks are validated only when
ValidateHistoricalis enabled AND the block is within the stability window. This aligns with the PR's objective to conditionally validate historical blocks.
925-930: Good defensive programming to avoid overwriting with nil.The guard correctly prevents overwriting existing protocol parameters with nil when the database query returns nothing. This is important for the genesis bootstrap scenario where parameters may already be set.
1069-1149: LGTM!The modifications correctly handle several important scenarios:
- Lines 1079-1086: Guards against unknown era IDs from the database, preventing crashes
- Lines 1088-1124: Bootstraps genesis protocol parameters when database lacks them (e.g., loading from immutable data without metadata)
- Line 1137: Correctly initializes to Byron era when starting from genesis, then transitions through intermediate eras
These changes align with the PR objectives to fix protocol parameter loading errors during era transitions.
1265-1309: Test helpers are appropriately exposed for external test packages.The exported test helper methods enable comprehensive testing from external test packages (like
eras_testand likelyledger_test). While the comment correctly warns these are for testing only, Go's visibility rules require them to be exported for external packages to access them.As per learnings: The test setup in this codebase uses external test packages, which requires these helpers to be exported.
wolf31o2
force-pushed
the
fix/pparam-loading
branch
from
2508d77 to
6f98b8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ledger/state.go (2)
933-1067: Consider extracting era chain transition logic to reduce duplication.The function correctly bootstraps protocol parameters through the complete era transition chain for each target era. However, there's significant code duplication across the era cases—each later era repeats the same transition chain (Shelley → Allegra → Mary → Alonzo → Babbage → Conway) with only the final step differing.
Consider refactoring with a helper function that chains era transitions:
func (ls *LedgerState) chainEraTransitions(targetEraId uint) (lcommon.ProtocolParameters, error) { var pparams lcommon.ProtocolParameters var err error // Start with Shelley pparams, err = eras.HardForkShelley(ls.config.CardanoNodeConfig, nil) if err != nil { return nil, err } // Chain through each era up to target for eraId := eras.AllegraEraDesc.Id; eraId <= targetEraId; eraId++ { era := eras.Eras[eraId] pparams, err = era.HardForkFunc(ls.config.CardanoNodeConfig, pparams) if err != nil { return nil, err } } return pparams, nil }This would reduce the function from ~135 lines to ~20 lines while maintaining the same functionality. However, the current explicit approach does make the transition chain very clear, so this refactor is optional.
1265-1309: Test helpers are functional but could follow better Go practices.The public test helper methods correctly expose internal state and private methods for testing purposes. The comment and "Public" suffix make the intent clear. However, as an optional improvement, consider using Go build tags to restrict these to test builds:
//go:build testing // +build testing // test_helpers.go func (ls *LedgerState) SetConfig(config LedgerStateConfig) { ls.config = config } // ... other test helpersThis would prevent these methods from being accidentally used in production code while still making them available in tests. However, this is purely a code organization suggestion—the current implementation is functional and the naming convention makes the test-only intent clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
database/pparams.go(3 hunks)ledger/certs.go(1 hunks)ledger/eras/eras.go(1 hunks)ledger/eras/eras_test.go(1 hunks)ledger/state.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ledger/eras/eras_test.go
- ledger/certs.go
- database/pparams.go
🧰 Additional context used
🧬 Code graph analysis (1)
ledger/state.go (10)
ledger/eras/shelley.go (2)
HardForkShelley(81-93)ShelleyEraDesc(30-41)config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/eras/allegra.go (2)
AllegraEraDesc(30-41)HardForkAllegra(81-94)ledger/eras/mary.go (2)
MaryEraDesc(30-41)HardForkMary(81-94)ledger/eras/alonzo.go (2)
AlonzoEraDesc(30-41)HardForkAlonzo(81-98)ledger/eras/babbage.go (2)
BabbageEraDesc(30-41)HardForkBabbage(81-94)ledger/eras/conway.go (2)
ConwayEraDesc(33-45)HardForkConway(85-102)ledger/eras/eras.go (3)
GetEraById(63-70)Eras(37-45)EraDesc(23-35)database/plugin/log.go (1)
Logger(18-27)database/models/epoch.go (2)
Epoch(17-27)Epoch(29-31)
⏰ 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). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
ledger/eras/eras.go (1)
61-70: LGTM! Clean era lookup implementation.The function correctly iterates through the Eras slice and returns a pointer to the matching EraDesc or nil if not found. The linear search is appropriate given the small number of eras (7), and returning a pointer to a slice element is safe here since Eras is a static, immutable slice.
ledger/state.go (4)
715-740: LGTM! Correct validation gating logic.The updated logic properly gates validation on both
ValidateHistoricalbeing enabled AND the block being within the k-slot stability window. This ensures historical blocks outside the rollback window are not validated, which aligns with the PR's objective to handle era transitions gracefully.
925-930: LGTM! Critical nil-guard fix.This change prevents nil protocol parameters from the database from overwriting valid
currentPParams. The guard ensures parameters are only updated when a valid value is found in the database or whencurrentPParamsis uninitialized. This is essential for handling empty-database scenarios and era transitions.
1079-1124: LGTM! Robust era initialization with genesis bootstrap.The changes properly handle era descriptor lookup with a nil-guard for unknown era IDs and implement genesis protocol parameter bootstrapping when database parameters are unavailable. This directly addresses the PR objective of fixing protocol parameter loading errors during genesis/preview network bootstrap. The fallback to
loadGenesisProtocolParametersensures the system can initialize even with an empty database or when loading from immutable storage without metadata.
1137-1137: LGTM! Correct initial era for genesis bootstrap.Initializing the current era to Byron (Eras[0]) when starting from genesis is correct, as Byron is the first era in the Cardano blockchain. The subsequent loop then transitions through each era up to the target era determined by the protocol version in the genesis file.
wolf31o2
force-pushed
the
fix/pparam-loading
branch
2 times, most recently
from
5bb60c3 to
f8d64da
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Add era-safe GetEraById() to prevent crashes from unknown era IDs - Implement genesis protocol parameter bootstrapping for empty database - Fix database error handling in GetPParams - Support complete era transition chain (Byron -> Conway) Signed-off-by: GitHub Copilot <noreply@github.com> Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
fix/pparam-loading
branch
from
f8d64da to
198cc81
Compare
…1006) - Add era-safe GetEraById() to prevent crashes from unknown era IDs - Implement genesis protocol parameter bootstrapping for empty database - Fix database error handling in GetPParams - Support complete era transition chain (Byron -> Conway) Signed-off-by: GitHub Copilot <noreply@github.com> Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
…1006) - Add era-safe GetEraById() to prevent crashes from unknown era IDs - Implement genesis protocol parameter bootstrapping for empty database - Fix database error handling in GetPParams - Support complete era transition chain (Byron -> Conway) Signed-off-by: GitHub Copilot <noreply@github.com> Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
Fixes protocol parameter type mismatch errors that occurred during era transitions and database operations.
Changes
GetEraById()function to prevent crashes from unknown era IDsGetPParamswith correct error variable usageValidation
dingo loadanddingo serveCloses #999
Summary by CodeRabbit
Bug Fixes
New Features
Tests