Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR enhances nonce verification by introducing Changes
Sequence DiagramsequenceDiagram
participant Client
participant CommandHandler as Command<br/>Handler
participant NonceTracker as NonceTracker
participant Cache as Local<br/>Cache
participant Ledger as Ledger/Chain
Client->>CommandHandler: Request (e.g., nextblock/leaderlog)
CommandHandler->>Cache: Check cached LeaderSchedule
alt Schedule exists in cache
CommandHandler->>NonceTracker: GetVerifiedNonceForEpoch(epoch)
NonceTracker->>Ledger: Compute nonce from chain data
NonceTracker->>Cache: Compare computed vs cached final_nonce
alt Nonces match
NonceTracker-->>CommandHandler: Return verified nonce
CommandHandler->>CommandHandler: scheduleNonceMatches = true
CommandHandler-->>Client: Use cached schedule
else Nonces mismatch (stale)
NonceTracker->>Cache: Update cache with correction
NonceTracker-->>CommandHandler: Return verified nonce
CommandHandler->>CommandHandler: scheduleNonceMatches = false
CommandHandler->>NonceTracker: Recompute schedule live
CommandHandler-->>Client: Use fresh schedule
end
else No cached schedule
CommandHandler->>NonceTracker: GetVerifiedNonceForEpoch(epoch)
NonceTracker-->>CommandHandler: Return verified nonce
CommandHandler->>NonceTracker: Compute schedule with verified nonce
CommandHandler-->>Client: Return computed schedule
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
1256-1263:⚠️ Potential issue | 🟡 MinorCanceled context passed to
scheduleNonceMatches.Line 1258 calls
cancel(), then line 1259 passes the now-canceledctxtoscheduleNonceMatches. Currently this is harmless becausescheduleNonceMatchesnever uses thectxparameter (it creates its own contexts internally), but it's fragile — if someone later adds a DB call usingctxinsidescheduleNonceMatches, it will silently fail.Suggested fix: defer the cancel or create a fresh context
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() existing, err := i.store.GetLeaderSchedule(ctx, nextEpoch) - cancel() if err == nil && existing != nil && i.scheduleNonceMatches(ctx, existing) {
🤖 Fix all issues with AI agents
In `@commands.go`:
- Around line 922-927: The ctxShort scope is canceled via cancelShort() before
being passed to i.scheduleNonceMatches, which is fragile; either postpone
calling cancelShort() until after i.scheduleNonceMatches(...) completes or call
i.scheduleNonceMatches with a fresh context (e.g., context.Background() or a
newly created short-lived context) so you never pass a canceled ctx to
scheduleNonceMatches; update the code around ctxShort/cancelShort,
i.store.GetLeaderSchedule(ctxShort, currentEpoch) and the subsequent call to
i.scheduleNonceMatches to use one of these two approaches.
In `@nonce.go`:
- Around line 319-356: GetVerifiedNonceForEpoch currently always recomputes the
epoch nonce in fullMode which can block hot paths like
scheduleNonceMatches/checkLeaderlogTrigger; add an in-memory cache on
NonceTracker (e.g., verifiedNonceCache map[int][]byte) protected by the existing
mutex so GetVerifiedNonceForEpoch first checks the cache and returns immediately
if present, otherwise proceeds to ComputeEpochNonce and then stores the verified
result into verifiedNonceCache before returning; ensure you update/evict entries
from verifiedNonceCache when final nonces are updated from SetFinalNonce or
other write paths so cache stays consistent.
🧹 Nitpick comments (2)
commands.go (2)
1070-1087:scheduleNonceMatchesacceptsctxbut never uses it.The
ctxparameter is unused —GetVerifiedNonceForEpochcreates its own internal contexts. This is confusing for callers (some of whom pass already-canceled contexts). Either remove the parameter or propagate it into the nonce verification call.Option A: drop the unused ctx parameter
-func (i *Indexer) scheduleNonceMatches(ctx context.Context, schedule *LeaderSchedule) bool { +func (i *Indexer) scheduleNonceMatches(schedule *LeaderSchedule) bool {Then update all call sites to remove the
ctxargument.
637-637:/noncecommand now returns verified nonce — note latency impact in full mode.For
/nonce next, this callsGetVerifiedNonceForEpoch(epoch+1)which in full mode triggersComputeEpochNonce(streaming all blocks from genesis). The user-facing Telegram command could take several minutes to respond. Consider showing a "computing..." message before the call, similar to the leaderlog commands.
| ctxShort, cancelShort := context.WithTimeout(context.Background(), 10*time.Second) | ||
| schedule, err := i.store.GetLeaderSchedule(ctxShort, currentEpoch) | ||
| cancelShort() | ||
| if err == nil && schedule != nil && !i.scheduleNonceMatches(ctxShort, schedule) { | ||
| schedule = nil | ||
| } |
There was a problem hiding this comment.
Same canceled-context pattern as checkLeaderlogTrigger.
ctxShort is canceled on line 924, then passed to scheduleNonceMatches on line 925. Currently safe because the ctx isn't used inside that function, but fragile.
Suggested fix
ctxShort, cancelShort := context.WithTimeout(context.Background(), 10*time.Second)
schedule, err := i.store.GetLeaderSchedule(ctxShort, currentEpoch)
- cancelShort()
- if err == nil && schedule != nil && !i.scheduleNonceMatches(ctxShort, schedule) {
+ if err == nil && schedule != nil && !i.scheduleNonceMatches(ctxShort, schedule) {
schedule = nil
}
+ cancelShort()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctxShort, cancelShort := context.WithTimeout(context.Background(), 10*time.Second) | |
| schedule, err := i.store.GetLeaderSchedule(ctxShort, currentEpoch) | |
| cancelShort() | |
| if err == nil && schedule != nil && !i.scheduleNonceMatches(ctxShort, schedule) { | |
| schedule = nil | |
| } | |
| ctxShort, cancelShort := context.WithTimeout(context.Background(), 10*time.Second) | |
| schedule, err := i.store.GetLeaderSchedule(ctxShort, currentEpoch) | |
| if err == nil && schedule != nil && !i.scheduleNonceMatches(ctxShort, schedule) { | |
| schedule = nil | |
| } | |
| cancelShort() |
🤖 Prompt for AI Agents
In `@commands.go` around lines 922 - 927, The ctxShort scope is canceled via
cancelShort() before being passed to i.scheduleNonceMatches, which is fragile;
either postpone calling cancelShort() until after i.scheduleNonceMatches(...)
completes or call i.scheduleNonceMatches with a fresh context (e.g.,
context.Background() or a newly created short-lived context) so you never pass a
canceled ctx to scheduleNonceMatches; update the code around
ctxShort/cancelShort, i.store.GetLeaderSchedule(ctxShort, currentEpoch) and the
subsequent call to i.scheduleNonceMatches to use one of these two approaches.
| // GetVerifiedNonceForEpoch returns a nonce that is verified against canonical | ||
| // data for that epoch, repairing stale DB cache entries if needed. | ||
| // | ||
| // Full mode: always recompute from local chain data and upsert DB cache. | ||
| // Lite mode: use existing lookup priority (DB -> Koios). | ||
| func (nt *NonceTracker) GetVerifiedNonceForEpoch(epoch int) ([]byte, error) { | ||
| if !nt.fullMode { | ||
| return nt.GetNonceForEpoch(epoch) | ||
| } | ||
|
|
||
| computeCtx, computeCancel := context.WithTimeout(context.Background(), 10*time.Minute) | ||
| defer computeCancel() | ||
| computed, err := nt.ComputeEpochNonce(computeCtx, epoch) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to verify nonce for epoch %d: %w", epoch, err) | ||
| } | ||
|
|
||
| checkCtx, checkCancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer checkCancel() | ||
| cached, cacheErr := nt.store.GetFinalNonce(checkCtx, epoch) | ||
| if cacheErr == nil && cached != nil && bytes.Equal(cached, computed) { | ||
| return cached, nil | ||
| } | ||
|
|
||
| source := "computed-verified" | ||
| if cacheErr == nil && cached != nil && !bytes.Equal(cached, computed) { | ||
| log.Printf("Correcting stale cached nonce for epoch %d: cached %x != computed %x", epoch, cached, computed) | ||
| source = "computed-correction" | ||
| } | ||
|
|
||
| storeCtx, storeCancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer storeCancel() | ||
| if err := nt.store.SetFinalNonce(storeCtx, epoch, computed, source); err != nil { | ||
| log.Printf("Failed to persist verified nonce for epoch %d: %v", epoch, err) | ||
| } | ||
|
|
||
| return computed, nil | ||
| } |
There was a problem hiding this comment.
GetVerifiedNonceForEpoch always recomputes from genesis in full mode — consider callers on hot paths.
In full mode this streams every block from Shelley genesis (up to 10 minutes). This is fine for on-demand commands (/leaderlog, /nonce) but problematic when called from scheduleNonceMatches inside checkLeaderlogTrigger, which runs synchronously on the block-processing goroutine. After a restart with an existing schedule in DB, the first block past the stability window will block the adder pipeline's event loop for the entire recomputation duration.
Consider caching the verified result per epoch in-memory (e.g., a map[int][]byte guarded by the existing mutex) so repeated calls for the same epoch return instantly after the first verification pass.
Sketch: add in-memory verified nonce cache
type NonceTracker struct {
mu sync.Mutex
store Store
koiosClient *koios.Client
evolvingNonce []byte
currentEpoch int
blockCount int
candidateFroze bool
networkMagic int
fullMode bool
+ verifiedNonces map[int][]byte // cache: epoch -> verified nonce
}Then in GetVerifiedNonceForEpoch:
func (nt *NonceTracker) GetVerifiedNonceForEpoch(epoch int) ([]byte, error) {
if !nt.fullMode {
return nt.GetNonceForEpoch(epoch)
}
+ nt.mu.Lock()
+ if cached, ok := nt.verifiedNonces[epoch]; ok {
+ nt.mu.Unlock()
+ return cached, nil
+ }
+ nt.mu.Unlock()
computeCtx, computeCancel := context.WithTimeout(context.Background(), 10*time.Minute)
defer computeCancel()
computed, err := nt.ComputeEpochNonce(computeCtx, epoch)
if err != nil {
return nil, fmt.Errorf("failed to verify nonce for epoch %d: %w", epoch, err)
}
+ nt.mu.Lock()
+ nt.verifiedNonces[epoch] = computed
+ nt.mu.Unlock()
// ... rest of DB cache check/repair🤖 Prompt for AI Agents
In `@nonce.go` around lines 319 - 356, GetVerifiedNonceForEpoch currently always
recomputes the epoch nonce in fullMode which can block hot paths like
scheduleNonceMatches/checkLeaderlogTrigger; add an in-memory cache on
NonceTracker (e.g., verifiedNonceCache map[int][]byte) protected by the existing
mutex so GetVerifiedNonceForEpoch first checks the cache and returns immediately
if present, otherwise proceeds to ComputeEpochNonce and then stores the verified
result into verifiedNonceCache before returning; ensure you update/evict entries
from verifiedNonceCache when final nonces are updated from SetFinalNonce or
other write paths so cache stays consistent.
1. Fix canceled context in /nextblock command: - Move cancelShort() after scheduleNonceMatches() call - Prevents passing canceled context to function 2. Add in-memory verified nonce cache: - Add verifiedNonces map[int][]byte to NonceTracker - Check cache before recomputing from genesis (10min operation) - Prevents blocking adder pipeline on hot paths - Cache guards with existing mutex Resolves CodeRabbit findings from PR #71. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(leaderlog): verify nonce from chain before serving schedules * fix(pr71): address CodeRabbit performance and context issues 1. Fix canceled context in /nextblock command: - Move cancelShort() after scheduleNonceMatches() call - Prevents passing canceled context to function 2. Add in-memory verified nonce cache: - Add verifiedNonces map[int][]byte to NonceTracker - Check cache before recomputing from genesis (10min operation) - Prevents blocking adder pipeline on hot paths - Cache guards with existing mutex Resolves CodeRabbit findings from PR #71. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1. Fix canceled context in /nextblock command: - Move cancelShort() after scheduleNonceMatches() call - Prevents passing canceled context to function 2. Add in-memory verified nonce cache: - Add verifiedNonces map[int][]byte to NonceTracker - Check cache before recomputing from genesis (10min operation) - Prevents blocking adder pipeline on hot paths - Cache guards with existing mutex Resolves CodeRabbit findings from PR #71. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix(leaderlog): verify nonce from chain before serving schedules * fix(pr71): address CodeRabbit performance and context issues 1. Fix canceled context in /nextblock command: - Move cancelShort() after scheduleNonceMatches() call - Prevents passing canceled context to function 2. Add in-memory verified nonce cache: - Add verifiedNonces map[int][]byte to NonceTracker - Check cache before recomputing from genesis (10min operation) - Prevents blocking adder pipeline on hot paths - Cache guards with existing mutex Resolves CodeRabbit findings from PR #71. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Motivation
final_noncevalues, causing cached schedules to be served with the wrong nonce.Description
GetVerifiedNonceForEpoch(epoch int)toNonceTracker, which in full mode recomputes the epoch nonce from local chain data (ComputeEpochNonce) and repairs the DB cache if it differs, while delegating to the existing path in lite mode./leaderlog,/nextblock, automaticcalculateAndPostLeaderlog, schedule cache validation (scheduleNonceMatches), and/nonceoutput.gofmt) and small struct/variable layout normalization.Testing
gofmt -w nonce.go commands.go main.goand the formatter completed successfully.go test ./... -run 'Test(Nonce|Store)' -count=1which completed and returned OK (no tests to run in this environment).go test ./...under a timeout, which completed successfully (exit 0).Summary by CodeRabbit