[KLC-2400] fix flaky configITO ShouldError test: slot-relative ITO windows#58
Conversation
…ndows The KAPP validates ITO windows against ctx.Block().GetTimestamp(), which in tests is genesis + slot*slotDuration — fully deterministic. The test was using time.Now().Add(4s) for StartTime, so on slow runners the configITO tx landed at a block whose timestamp had already passed StartTime and was rejected as invalid. Replace wall-clock offsets with slot-relative anchors so the window invariant blockTs(slot) < itoStart <= blockTs(slot+1) <= itoEnd < blockTs(slot+2) holds by construction. Verified: 20/20 passes in a -count=1 loop (previously ~40% failure on develop).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (Custom checks)
Files:
**/*_test.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-04-21T20:12:22.959ZApplied to files:
🔇 Additional comments (1)
WalkthroughThis PR updates TestTransaction_CreateConfigITO_ShouldError to compute all ITO and whitelist window timestamps from the proposer node's SlotManager (genesis timestamp + slot duration) and uses those deterministic values across all error and success test cases. ChangesITO time-window determinism
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes flakiness in the TestTransaction_CreateConfigITO_ShouldError integration test by eliminating wall-clock-based ITO window timestamps and instead anchoring ITO start/end times to deterministic, slot-relative timestamps derived from the test node’s SlotManager.
Changes:
- Replace
time.Now().Add(...)ITO/whitelist window timestamps with slot-relative timestamps (itoStart,itoEnd) based onSlotManager.Timestamp()andSlotManager.TimeDuration(). - Keep the two malformed-time cases by using deliberately inverted start/end timestamps computed from the same slot-relative anchors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integrationTest/transaction/configITO/configITO_test.go`:
- Line 208: itoEnd is set to itoStart+1 which assumes slotDuration>1s and can
make the "out-of-window at block(slot+2)" case hit the boundary when
slotDuration==1s; change the computation of itoEnd to derive it from the test's
slot boundaries (e.g., use slotDuration or the existing slotStart/slotEnd
variables) so itoEnd = itoStart + slotDuration (or itoEnd = slotEnd) instead of
a fixed +1, and update any assertions that rely on itoEnd to use the new
computed value (look for itoStart, itoEnd, slotDuration and any references to
slotStart/slotEnd in the test).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9dc3a59b-7acc-43e5-990a-fb6301a426eb
📒 Files selected for processing (1)
integrationTest/transaction/configITO/configITO_test.go
📜 Review details
⏰ 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: Agent
- GitHub Check: setup-and-lint / setup-and-lint
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
integrationTest/transaction/configITO/configITO_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
integrationTest/transaction/configITO/configITO_test.go
🧠 Learnings (1)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
integrationTest/transaction/configITO/configITO_test.go
🔇 Additional comments (1)
integrationTest/transaction/configITO/configITO_test.go (1)
198-207: LGTM!Also applies to: 209-212, 229-232, 251-254, 273-276, 295-298, 317-320, 339-342, 361-364, 383-386, 405-408, 427-430, 449-452, 471-474, 493-496, 515-518, 537-540
- Rename genesisTs → slot0Ts; the value is the proposer's slot-0 reference (SlotManagerMock captures time.Now() at node construction in block.go), not actual chain genesis. Tighten the explanatory comment to match. - Change itoEnd to slot0Ts + (slot+2)*slotDuration - 1 so the invariant itoEnd < blockTs(slot+2) holds regardless of slotDuration. With the previous +1, a 1s slotDuration would land itoEnd exactly at blockTs(slot+2) and the final "out of window" buy would silently pass the strict-less-than check at ito.go:336-337.
Summary
TestTransaction_CreateConfigITO_ShouldErrorwas flaky (~40% failure rate ondevelop) because it pinned ITOStartTimetotime.Now().Add(4s). The KAPP, however, validates ITO windows againstctx.Block().GetTimestamp()(core/kapp/ito/ito.go:660-666), and the test harness computes that asgenesis + slot * slotDuration(integrationTest/processorNode/block.go:72) — fully deterministic. When the runner was slow enough that wall-clock drifted past the 4-second cushion, the block-time checkStartTime <= block.Timestampfired and rejected the onlyconfigITOTxHashthe test expected to succeed.Fix
Replace wall-clock offsets with slot-relative anchors derived from the proposer's
SlotManager:This guarantees
blockTs(slot) < itoStart <= blockTs(slot+1) <= itoEnd < blockTs(slot+2), which is exactly the window the rest of the test (config submission atslot, in-window buys atslot+1, out-of-window buy atslot+2) needs — independent of wall-clock load. Two malformed-time test cases still use deliberately inverted values forstart > endvalidation.Sibling
triggerITO_test.gowas audited: it uses day-scaleAddDateoffsets and isn't affected.Test plan
go test ./integrationTest/transaction/configITO/ -run TestTransaction_CreateConfigITO_ShouldError -count=1— 20/20 pass in a loop (~270s total); previously ~40% failure rate.go test ./integrationTest/transaction/configITO/ -count=1package — pass.develop.Test-Only Reliability Fix (configITO_test.go)
Scope
Problem
Fix
Rationale / Invariants
Other edits
Blockchain-critical impact
Test results