fix(taiko-client): check the shasta last seen proposal after reorg #20496
Conversation
YoGhurt111
requested review from
MatusKysel,
cyberhorsey and
davidtaikocha
as code owners
|
Claude finished @YoGhurt111's task —— View job PR Review CompleteReviewing PR #20496: "fix(taiko-client): check the shasta last seen proposal after reorg" Todo List:
SummaryThis PR addresses a critical issue with reorg handling for Shasta proposals by implementing comprehensive reorg detection and recovery logic. The changes span 4 files with significant enhancements to the reorg monitoring system. Code Quality & Best Practices✅ Strengths:
Potential Bugs & Issues🚨 Critical Issues:
Performance ConsiderationsConcerns:
Recommendations:
Security ConcernsLow Risk Issues:
Test CoverageMissing Tests:
Recommended Test Cases:
Specific Code Issuesserver.go:1177-1199// BUG: This assumes all proposals are Pacaya type
func (s *PreconfBlockAPIServer) recordLatestSeenProposal(proposal *encoding.LastSeenProposal) {
// ...
log.Info(
"Received latest batch seen in event",
"batchID", proposal.Pacaya().GetBatchID(), // 🚨 Will panic for Shasta proposalsFix: Add type checking: if proposal.IsPacaya() {
log.Info("Received latest batch seen in event", "batchID", proposal.Pacaya().GetBatchID())
} else {
log.Info("Received latest proposal seen in event", "proposalID", proposal.Shasta().GetProposal().Id)
}server.go:1134maxIterations := latestSeenProposalID.Int64() // 🚨 No bounds checkingFix: Add reasonable bounds: const maxReorgSearchDepth = 10000
maxIterations := latestSeenProposalID.Int64()
if maxIterations > maxReorgSearchDepth {
maxIterations = maxReorgSearchDepth
}Overall AssessmentThis PR addresses an important reorg handling issue but has several critical bugs that need fixing before merge. The architecture is sound, but implementation details need attention. Recommendation: Request Changes - The critical race condition and panic potential make this unsafe for production without fixes. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Find the last valid proposal by searching backwards | ||
| maxIterations := latestSeenProposalID.Int64() | ||
| for i := int64(1); i <= maxIterations; i++ { | ||
| currentProposalID := new(big.Int).Sub(latestSeenProposalID, big.NewInt(i)) | ||
|
|
||
| onChainHash, err := s.rpc.GetShastaProposalHash(&bind.CallOpts{Context: ctx}, currentProposalID) | ||
| if err != nil { | ||
| log.Error("Failed to filter batch proposed event", "err", err) | ||
| log.Error("Failed to get shasta proposal on chain", "proposalId", currentProposalID, "err", err) | ||
| return | ||
| } | ||
| defer iterPacaya.Close() | ||
|
|
||
| for iterPacaya.Next() { | ||
| if new(big.Int).SetUint64(iterPacaya.Event.Meta.BatchId).Cmp(s.latestSeenProposal.Pacaya().GetBatchID()) < 0 { | ||
| s.recordLatestSeenProposal(&encoding.LastSeenProposal{ | ||
| TaikoProposalMetaData: metadata.NewTaikoDataBlockMetadataPacaya(iterPacaya.Event), | ||
| PreconfChainReorged: true, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if err := iterPacaya.Error(); err != nil { | ||
| log.Error("Failed to iterate batch proposed events", "err", err) | ||
| } | ||
| } else { | ||
| latestSeenProposalID := proposal.Shasta().GetProposal().Id | ||
| shastaProposal := s.shastaIndexer.GetLastProposal() | ||
| if latestSeenProposalID.Cmp(shastaProposal.Proposal.Id) <= 0 { | ||
| recordedProposal, err := s.shastaIndexer.GetProposalByID(currentProposalID.Uint64()) | ||
| if err != nil { | ||
| log.Error("Proposal not found in cache", "proposalId", currentProposalID, "err", err) | ||
| return |
There was a problem hiding this comment.
The new handleShastaProposalReorg walks backwards through proposal IDs using s.shastaIndexer.GetProposalByID, but it returns immediately when a cached proposal is not found (log.Error("Proposal not found in cache"…) followed by return). Because the indexer intentionally prunes old entries (cleanupLegacyProposals) the cache can legitimately miss IDs after a deep reorg or on startup. The previous implementation fetched the needed proposal data directly from the chain via FilterProposed, so it could recover regardless of what remained in the cache. With this change, any reorg deeper than the retained buffer leaves latestSeenProposal pointing at an invalid proposal and the monitor never rewinds to a canonical state, repeatedly logging errors instead of recovering. Consider continuing the search or fetching the proposal from the RPC instead of aborting when the cache is empty.
Useful? React with 👍 / 👎.
| GetBlobTimestamp(int) uint64 | ||
| GetRawBlockHeight() *big.Int | ||
| GetRawBlockHash() common.Hash | ||
| GetLog() types.Log |
There was a problem hiding this comment.
| GetLog() types.Log | |
| GetLog() *types.Log |
Let's only return a reference here?
| Derivation *shastaBindings.IInboxDerivation | ||
| RawBlockHash common.Hash | ||
| RawBlockHeight *big.Int | ||
| Log types.Log |
2 participants
No description provided.