fix: use stability window for slot threshold #985
Conversation
📝 WalkthroughWalkthroughThe PR makes ledger logic era-aware for stability-window and slot-threshold calculations. It adds Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧠 Learnings (8)📚 Learning: 2025-10-22T21:28:48.396ZApplied to files:
📚 Learning: 2025-10-22T21:29:26.924ZApplied to files:
📚 Learning: 2025-10-22T20:19:25.277ZApplied to files:
📚 Learning: 2025-10-22T20:13:35.164ZApplied to files:
📚 Learning: 2025-10-22T20:13:35.972ZApplied to files:
📚 Learning: 2025-10-22T20:19:26.360ZApplied to files:
📚 Learning: 2025-10-22T21:28:48.396ZApplied to files:
📚 Learning: 2025-10-22T20:18:45.125ZApplied to files:
🧬 Code graph analysis (4)ledger/chainsync_test.go (6)
ledger/state_test.go (5)
ledger/chainsync.go (2)
ledger/state.go (3)
⏰ 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/chainsync.go (1)
274-314: Critical: Unreachable dead code and calculation issues.
Unreachable dead code: Lines 279-281 return early for Byron era (
Id == 0), making the Byron-specific branches at lines 300-301 and 307-308 unreachable. These conditions will never be true since we've already returned for Byron.Same calculation issues as handleEventChainsyncBlockHeader:
- Missing nil check for
ActiveSlotsCoeff.Rat(line 312)- Incorrect use of
.Num().Uint64()instead of proper integer division (lines 310-314)- Missing validation for negative
SecurityParamApply this diff:
// Calculate stability window byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if byronGenesis == nil || shelleyGenesis == nil { return nil, errors.New("could not get genesis config") } - var securityParam uint64 - // Use K parameter from Byron Genesis during Byron era, SecurityParam from Shelley Genesis for later eras - if ls.currentEra.Id == 0 { // Byron era ID is 0 - securityParam = uint64(byronGenesis.ProtocolConsts.K) - } else { - securityParam = uint64(shelleyGenesis.SecurityParam) - } - // Calculate stability window: 3k/f for Shelley+ eras, just k for Byron - var stabilityWindow uint64 - if ls.currentEra.Id == 0 { // Byron era - stabilityWindow = securityParam - } else { - stabilityWindow = new(big.Rat).Quo( - big.NewRat(int64(3*securityParam), 1), - shelleyGenesis.ActiveSlotsCoeff.Rat, - ).Num().Uint64() - } + // Note: We're already in Shelley+ era due to early return at line 279 + // Use SecurityParam from Shelley Genesis + k := shelleyGenesis.SecurityParam + if k < 0 { + return nil, fmt.Errorf("security param must be non-negative: %d", k) + } + securityParam := uint64(k) + + // Calculate stability window: 3k/f for Shelley+ eras + if shelleyGenesis.ActiveSlotsCoeff.Rat == nil { + return nil, errors.New("ActiveSlotsCoeff.Rat is nil") + } + result := new(big.Rat).Quo( + big.NewRat(int64(3*securityParam), 1), + shelleyGenesis.ActiveSlotsCoeff.Rat, + ) + // Proper integer division: divide numerator by denominator + stabilityWindow := new(big.Int).Div(result.Num(), result.Denom()).Uint64()
🧹 Nitpick comments (1)
ledger/chainsync.go (1)
143-171: Consider extracting stability window calculation to a helper function.The stability window calculation logic (era-aware security parameter retrieval + 3k/f formula for Shelley+) is duplicated between
handleEventChainsyncBlockHeaderandcalculateEpochNonce. This duplication increases maintenance burden and the risk of inconsistencies.Consider creating a helper method:
// calculateStabilityWindow returns the stability window based on the current era. // For Byron era, returns k. For Shelley+ eras, returns 3k/f. // Returns the default threshold if genesis data is unavailable or invalid. func (ls *LedgerState) calculateStabilityWindow() uint64 { byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if byronGenesis == nil || shelleyGenesis == nil { return blockfetchBatchSlotThresholdDefault } var securityParam uint64 if ls.currentEra.Id == 0 { // Byron era securityParam = uint64(byronGenesis.ProtocolConsts.K) return securityParam // Byron uses k directly } // Shelley+ eras k := shelleyGenesis.SecurityParam if k < 0 { ls.config.Logger.Warn("invalid negative security parameter", "k", k) return blockfetchBatchSlotThresholdDefault } securityParam = uint64(k) // Calculate 3k/f if shelleyGenesis.ActiveSlotsCoeff.Rat == nil { ls.config.Logger.Warn("ActiveSlotsCoeff.Rat is nil") return blockfetchBatchSlotThresholdDefault } result := new(big.Rat).Quo( big.NewRat(int64(3*securityParam), 1), shelleyGenesis.ActiveSlotsCoeff.Rat, ) return new(big.Int).Div(result.Num(), result.Denom()).Uint64() }Then use it in both locations:
slotThreshold := ls.calculateStabilityWindow()Also applies to: 298-314
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (1)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ 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: lint
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
🔇 Additional comments (2)
ledger/state.go (1)
597-623: LGTM! Era-aware security parameter retrieval is correctly implemented.The implementation properly handles era-specific security parameter sources with appropriate error handling and validation. The check for negative k before casting to uint64 (lines 616-621) is essential to prevent incorrect large values.
ledger/chainsync.go (1)
40-40: LGTM! Clear naming improvement.Renaming to
blockfetchBatchSlotThresholdDefaultbetter conveys that this is a fallback value when genesis data is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ledger/chainsync.go (1)
346-390: Same duplication and overflow issues as blockfetch threshold calculation.This code segment duplicates the stability window calculation from lines 148-213. Please see my earlier comment on those lines suggesting extraction into a shared helper function.
Additionally, line 379 has the same potential uint64 overflow issue as line 193: computing
3 * securityParambefore converting tobig.Intcould overflow ifsecurityParamis large enough.
🧹 Nitpick comments (2)
ledger/state.go (1)
597-632: Suggest extracting era-aware security parameter retrieval into a helper function.This security parameter retrieval logic (Byron K vs. Shelley SecurityParam with validation) is duplicated in
ledger/chainsync.goat lines 148-172 and 346-362. Consider extracting it into a helper method onLedgerState, such as:func (ls *LedgerState) getSecurityParam() (uint64, error) { if ls.currentEra.Id == 0 { // Byron era byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() if byronGenesis == nil { return 0, errors.New("failed to get Byron Genesis config") } k := byronGenesis.ProtocolConsts.K if k < 0 { return 0, fmt.Errorf("Byron K parameter must be non-negative: %d", k) } return uint64(k), nil // #nosec G115 } else { // Shelley and later eras shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if shelleyGenesis == nil { return 0, errors.New("failed to get Shelley Genesis config") } k := shelleyGenesis.SecurityParam if k < 0 { return 0, fmt.Errorf("security param must be non-negative: %d", k) } return uint64(k), nil // #nosec G115 } }This would improve maintainability and ensure consistency across the codebase.
ledger/chainsync.go (1)
193-193: Apply defensive big.Int arithmetic for the security parameter multiplication to prevent theoretical uint64 overflow.While Cardano protocol specifies no hardcoded maximum for the security parameter k and it's governance-changeable via on-chain updates, the current code multiplies
3 * securityParamin uint64 before converting to big.Int (ledger/chainsync.go lines 193 and 379). IfsecurityParamever exceeds ~6.1e18, this wraps silently and corrupts the stability window calculation—a bug the subsequentIsUint64()check won't catch.Though mainnet k=2160 makes overflow practically impossible today, use big.Int throughout for robustness:
-numerator := new(big.Int).SetUint64(3 * securityParam) +numerator := new(big.Int).SetUint64(securityParam) +numerator.Mul(numerator, big.NewInt(3))Apply this fix at both line 193 and line 379 in ledger/chainsync.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (1)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)
ledger/chainsync.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ 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: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
ledger/chainsync.go (2)
39-40: LGTM!The rename from
blockfetchBatchSlotThresholdtoblockfetchBatchSlotThresholdDefaultclearly indicates this is a fallback value, which aligns well with the dynamic calculation logic introduced below.
215-219: LGTM!The computed
slotThresholdis correctly applied in the conditional to gate early return when the node is far from the upstream tip.
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
6734d13 to
8a15b9c
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 (1)
ledger/state.go (1)
341-361: Avoid hard failure when Byron genesis is absent.
calculateEpochNoncealready returns early for Byron (Line 327), so deployments that start in Shelley legitimately provide no Byron genesis. The newif byronGenesis == nil || shelleyGenesis == nilcheck makes those setups bail out with “could not get genesis config”, breaking epoch rollover. Please only require the genesis file for the era you actually use here.Apply both hunks to keep compilation intact:
- byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() - shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() - if byronGenesis == nil || shelleyGenesis == nil { - return nil, errors.New("could not get genesis config") - } + shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() + if shelleyGenesis == nil { + return nil, errors.New("failed to get Shelley genesis config") + } @@ - if ls.currentEra.Id == 0 { // Byron era ID is 0 - k := byronGenesis.ProtocolConsts.K + if ls.currentEra.Id == 0 { // Byron era ID is 0 + byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() + if byronGenesis == nil { + return nil, errors.New("failed to get Byron genesis config") + } + k := byronGenesis.ProtocolConsts.K
🧹 Nitpick comments (1)
ledger/chainsync.go (1)
144-213: Factor stability-window calculation into a helper.This block duplicates the security-param extraction, ActiveSlotsCoeff validation, and overflow handling that
calculateEpochNonce(Lines 346-390) already performs. Consider extracting a sharedcalculateStabilityWindow()helper returning(uint64, error)so both call sites stay in sync—here you can warn and fall back on error, whilecalculateEpochNoncecan propagate it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (1)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)
ledger/chainsync.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
⏰ 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: nilaway
- GitHub Check: lint
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
ledger/state.go (1)
598-632: LGTM: era-aware k-window guard.The non-negative checks and era split keep the cutoff aligned with the right security parameter without risking underflow.
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
8a15b9c to
e3e3f6a
Compare
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 (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ 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). (5)
- GitHub Check: lint
- GitHub Check: nilaway
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: Analyze (go)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
e3e3f6a to
471d3d0
Compare
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 (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ 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)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
471d3d0 to
42827a4
Compare
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 (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
⏰ 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)
wolf31o2
changed the title
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
42827a4 to
a2f1e1f
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)
ledger/state.go (1)
312-323: Ceil the Shelley-era stability window in epoch nonce calculationThe same floor-based division shows up here, so the epoch-nonce stability window can undershoot the spec by one slot on networks where
3*k/fis non-integral. That can change which block we treat as “before the stability window.” Please switch to the ceiling form as in the ledger reference.- numerator := new(big.Int).SetUint64(3 * securityParam) - quotient := new(big.Int).Div( - new(big.Int).Mul(numerator, activeSlotsRat.Denom()), - activeSlotsRat.Num(), - ) + numerator := new(big.Int).SetUint64(securityParam) + numerator.Mul(numerator, big.NewInt(3)) + numerator.Mul(numerator, activeSlotsRat.Denom()) + denominator := new(big.Int).Set(activeSlotsRat.Num()) + quotient, remainder := new(big.Int).QuoRem(numerator, denominator, new(big.Int)) + if remainder.Sign() != 0 { + quotient.Add(quotient, big.NewInt(1)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ 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)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
a2f1e1f to
594a6f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ledger/chainsync.go (1)
272-323: Consider calling calculateStabilityWindow() to reduce duplication.The stability window calculation logic here (lines 272-323) closely mirrors the implementation in
calculateStabilityWindow()(state.go lines 409-495). The only difference is error handling: this function returns errors whilecalculateStabilityWindow()logs and falls back to the default.While the past review comment suggested extracting this into a shared helper, the current implementation keeps them separate due to different error-handling semantics. Consider whether you could call
calculateStabilityWindow()and convert a zero return value into an error, which would eliminate the duplication while preserving the error-propagation behavior needed here.For example:
stabilityWindow := ls.calculateStabilityWindow() if stabilityWindow == blockfetchBatchSlotThresholdDefault { return nil, errors.New("could not calculate stability window from genesis config") }However, this approach loses granular error messages. If you prefer to keep the detailed validation errors, the current duplication is acceptable for maintainability of distinct error paths.
ledger/state.go (1)
685-720: Consider extracting era-aware security parameter retrieval into a helper.The era-aware security parameter logic (lines 685-720) is duplicated in
calculateEpochNonce(chainsync.go lines 272-295). Both branches check for genesis availability, validate non-negative parameters, and select between Byron K and Shelley SecurityParam based on era.Extracting a helper like
getSecurityParam() (uint64, error)would eliminate this duplication and ensure consistent validation across both call sites.For example:
func (ls *LedgerState) getSecurityParam() (uint64, error) { if ls.currentEra.Id == 0 { // Byron era byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() if byronGenesis == nil { return 0, errors.New("failed to get Byron Genesis config") } k := byronGenesis.ProtocolConsts.K if k <= 0 { return 0, fmt.Errorf("k parameter must be positive: %d", k) } return uint64(k), nil // #nosec G115 } // Shelley+ eras shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if shelleyGenesis == nil { return 0, errors.New("failed to get Shelley Genesis config") } k := shelleyGenesis.SecurityParam if k <= 0 { return 0, fmt.Errorf("security param must be positive: %d", k) } return uint64(k), nil // #nosec G115 }This is optional since the duplication is limited and the code is clear as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ 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 (3)
ledger/chainsync.go (2)
39-40: LGTM: Constant rename clarifies fallback behavior.The rename from
blockfetchBatchSlotThresholdtoblockfetchBatchSlotThresholdDefaultaccurately reflects that this is now a fallback value when genesis data is unavailable or invalid.
143-146: LGTM: Dynamic stability window calculation properly integrated.The block header handling now uses the calculated stability window instead of a hard-coded value, achieving the PR's objective of protocol compliance and era awareness.
ledger/state.go (1)
409-495: LGTM: Comprehensive stability window calculation with proper safeguards.The
calculateStabilityWindow()method correctly implements:
- Era-aware parameter selection (Byron K vs. Shelley SecurityParam)
- Validation of genesis availability, non-negative/non-zero parameters, and ActiveSlotsCoeff
- Ceiling calculation for Shelley+ eras (3k/f rounded up) using proper big.Int arithmetic
- Overflow checks before uint64 conversion
- Fallback to default threshold on any validation failure
All issues raised in past review comments have been addressed.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Creating a local commit for the generated unit tests... The unit tests have been committed to the current branch. Commit ID: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ledger/chainsync.go (1)
296-323: Consolidate stability window calculation
calculateEpochNoncere-implements the stability-window math even thoughhandleEventChainsyncBlockHeadernow relies onls.calculateStabilityWindow(). Keeping two hand-written copies makes it easy for the formulas or validations to drift apart the next time we tweak era handling or overflow checks. Please fold both call sites onto the same helper (or extract a shared internal routine that returns(uint64, error)socalculateEpochNoncecan surface hard failures) and have the caller decide whether to log/fallback or bubble the error. That keeps the protocol constants in one place and prevents future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ledger/chainsync.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (1)
Rat(25-27)
⏰ 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)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
a43d73e to
3d97e2c
Compare
|
@coderabbitai the tests for this are failing |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12045 🏁 Script executed: Length of output: 4717 🏁 Script executed: Length of output: 889 🏁 Script executed: Length of output: 1649 🏁 Script executed: Length of output: 6322 🏁 Script executed: Length of output: 4114 🏁 Script executed: Length of output: 1216 🏁 Script executed: Length of output: 4951 @wolf31o2, I found the issue! The test The problem is that in Byron era (era ID 0), the The test expects an error but gets Here's the fix for the test: func TestCalculateEpochNonce_MissingByronGenesisInByronEra(t *testing.T) {
shelleyGenesisJSON := `{
"activeSlotsCoeff": 0.05,
"securityParam": 432,
"systemStart": "2022-10-25T00:00:00Z"
}`
cfg := &cardano.CardanoNodeConfig{
ShelleyGenesisHash: "363498d1024f84bb39d3fa9593ce391483cb40d479b87233f868d6e57c3a400d",
}
if err := cfg.LoadShelleyGenesisFromReader(strings.NewReader(shelleyGenesisJSON)); err != nil {
t.Fatalf("failed to load Shelley genesis: %v", err)
}
ls := &LedgerState{
currentEra: eras.ByronEraDesc,
currentEpoch: models.Epoch{
EpochId: 1,
StartSlot: 86400,
Nonce: []byte{0x01, 0x02, 0x03},
},
config: LedgerStateConfig{
CardanoNodeConfig: cfg,
Logger: slog.New(slog.NewJSONHandler(io.Discard, nil)),
},
db: &database.Database{},
}
// Byron era should return nil nonce with no error (Byron doesn't have epoch nonces)
// The function returns early before any genesis validation
nonce, err := ls.calculateEpochNonce(nil, 86400)
if err != nil {
t.Errorf("expected no error for Byron era, got: %v", err)
}
if nonce != nil {
t.Errorf("expected nil nonce for Byron era, got: %v", nonce)
}
}The fix changes the test to correctly verify that:
This aligns with the actual behavior of the code where Byron era returns immediately without needing any genesis data. 🧠 Learnings used |
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
3d97e2c to
ad2f39f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(3 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(2 hunks)ledger/state_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ledger/chainsync_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 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_test.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_test.go
📚 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_test.go
🧬 Code graph analysis (3)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/state_test.go (5)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
⏰ 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)
| t.Run("Missing Shelley genesis returns default", func(t *testing.T) { | ||
| cfg := &cardano.CardanoNodeConfig{} | ||
| byronGenesisJSON := `{ | ||
| "protocolConsts": { | ||
| "k": 432, | ||
| "protocolMagic": 2 | ||
| } | ||
| }` | ||
| if err := cfg.LoadByronGenesisFromReader(strings.NewReader(byronGenesisJSON)); err != nil { | ||
| t.Fatalf("failed to load Byron genesis: %v", err) | ||
| } | ||
|
|
||
| ls := &LedgerState{ | ||
| currentEra: eras.ByronEraDesc, | ||
| config: LedgerStateConfig{ | ||
| CardanoNodeConfig: cfg, | ||
| Logger: slog.New(slog.NewJSONHandler(io.Discard, nil)), | ||
| }, | ||
| } | ||
|
|
||
| result := ls.calculateStabilityWindow() | ||
| if result != blockfetchBatchSlotThresholdDefault { | ||
| t.Errorf("expected default threshold %d, got %d", blockfetchBatchSlotThresholdDefault, result) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix expected window when Shelley genesis is absent in Byron era
This test now asserts that we fall back to blockfetchBatchSlotThresholdDefault whenever the Shelley genesis is missing, even though the Byron calculation path doesn’t use it at all. Once we stop unconditionally requiring both genesis configs, the correct Byron-era behavior here is to still return k. Please update the test to expect the Byron security parameter (432 in this fixture) so we don’t bake in the regression.
🤖 Prompt for AI Agents
In ledger/state_test.go around lines 214-238, the test currently expects
blockfetchBatchSlotThresholdDefault when Shelley genesis is absent; update it to
expect the Byron security parameter instead (432 for the provided fixture) —
either assert the literal 432 or, better, read the value from the loaded Byron
genesis in cfg (the protocolConsts.k field) and compare that to the result so
the test reflects correct Byron-era behavior.
- Replace hard-coded blockfetchBatchSlotThreshold with dynamic calculation - Use correct security parameters based on current era: * Byron era: K parameter from ByronGenesis.ProtocolConsts.K * Shelley+ eras: SecurityParam from ShelleyGenesis.SecurityParam - Update stability window calculation in both chainsync and validation logic - Calculate stability window as 3k/f for Shelley+ eras, k for Byron era - Ensures blockfetch operations respect protocol-defined stability window Fixes the TODO to calculate slot threshold from protocol params Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
ad2f39f to
4a59bd9
Compare
Summary
This PR replaces the hard-coded slot threshold for bulk blockfetch operations with the protocol-defined stability window, addressing the TODO comment in the codebase.
Changes
Core Changes
Files Modified
Benefits
Testing
Technical Details
The stability window is calculated differently based on the era:
This ensures that bulk blockfetch operations stop at the appropriate distance from the chain tip, as defined by the protocol's stability guarantees.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Closes #591