NONEVM-3054: load lastest processed masterchain block seqno #464
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements masterchain block sequence number tracking to enable service restart resumption from the last processed state, eliminating the need to reprocess blocks after restarts.
Key Changes:
- Added
GetLatestMasterBlockSeqnomethod to retrieve the highest masterchain block sequence number from stored logs - Updated
Txmodel to includeMasterBlockSeqnofield for tracking masterchain finalization of shard blocks - Modified
getLastProcessedBlockto prioritize stored state: in-memory → database → lookback window calculation
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/logpoller/store/postgres/logs.go | Implements database query to retrieve max master_block_seqno from logs table |
| pkg/logpoller/store/memory/logs.go | Adds cached maxMasterBlockSeqno field with O(1) lookup implementation |
| pkg/logpoller/store/memory/logs_test.go | Comprehensive test coverage for GetLatestMasterBlockSeqno with empty store and multiple logs scenarios |
| pkg/logpoller/interfaces.go | Defines GetLatestMasterBlockSeqno interface method with documentation |
| pkg/logpoller/observed_log_store.go | Adds observability wrapper with metrics for the new database query method |
| pkg/logpoller/models/models.go | Extends Tx model with MasterBlockSeqno field and updates Block field documentation |
| pkg/logpoller/parser.go | Updates parser to accept Tx model and populate MasterBlockSeqno in log entries |
| pkg/logpoller/parser_test.go | Updates test cases to use new Tx model structure |
| pkg/logpoller/block.go | Implements three-tier fallback logic in getLastProcessedBlock for state resumption |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/logpoller/block.go
Outdated
| lp.lggr.Debugw("Resuming from last processed", "seqNo", lastProcessed) | ||
| } | ||
| return lastProcessed, nil | ||
| blocksToProcess := currentBlock.SeqNo - lookbackSeqNo |
There was a problem hiding this comment.
The variable blocksToProcess is computed but never used. This appears to be dead code left over from refactoring. Consider removing this line if it serves no purpose, or use it in the logging statement below if the intent is to include it.
| func (s *pgLogStore) GetLatestMasterBlockSeqno(ctx context.Context) (uint32, error) { | ||
| var result *int64 | ||
|
|
||
| sql := `SELECT MAX(master_block_seqno) FROM ton.log_poller_logs WHERE chain_id = :chain_id` |
There was a problem hiding this comment.
The query scans the entire table for MAX(master_block_seqno). If this table grows large, consider adding an index on (chain_id, master_block_seqno DESC) to optimize this query, especially since it will be called on every service restart.
There was a problem hiding this comment.
Requires db schema update, NONEVM-3033 will handle with other optimizations in future sprint
* feat: resolve mc block seqno on load * chore: tests * chore: nix hash * chore: nix hash * chore: lint * fix: tests
jadepark-dev
dismissed stale reviews from huangzhen1997, ogtownsend, and patricios-space
via
6ab4a90
pkg/logpoller/store/memory/logs.go
Outdated
| // GetLatestMCBlockSeqno returns the highest masterchain block sequence number | ||
| // from stored logs. Returns (seqno, exists, err) where exists indicates whether any | ||
| // logs are stored. | ||
| func (s *inMemoryLogs) GetLatestMCBlockSeqno(_ context.Context) (uint32, bool, error) { |
There was a problem hiding this comment.
The Latest vs Max confuses me.
- We expect block sequence numbers to be growing, right? Then the expression
// update cached max
if log.MCBlockSeqno > s.maxMCBlockSeqno {
s.maxMCBlockSeqno = log.MCBlockSeqno
}
should always be true. Should we throw an error if this condition is not met?
- Do we expect sequence numbers to be incremental? In that case, we can also expect
log.MCBlockSeqno == s.maxMCBlockSeqno + 1
There was a problem hiding this comment.
My question arises from GetLatestMCBlockSeqno returning maxMCBlockSeqno. If we don't expect seq numbers to always be greater than the previous one, the latest to be process may not be maxMCBlockSeqno.
Maybe I am missing something
There was a problem hiding this comment.
Good questions, yeah the naming could be confusing.
- "highest seqno" - what the current implementation returns
- "chronologically last processed" - what you might think from the name
Should we throw an error if log.MCBlockSeqno > s.maxMCBlockSeqno is not met?
In our polling logic, multiple filters are processed concurrently, so logs arriving at the store may not be the "latest" in terms of masterchain block order. However, the function name was chosen from the perspective of the logpoller main loop: "what is the block number where processing was most recently completed?". I've renamed the function to GetHighestMCBlockSeqno to make this clearer.
Do we expect log.MCBlockSeqno == s.maxMCBlockSeqno + 1?
No. Sequence numbers are not strictly incremental for two reasons:
- Multiple logs can share the same
MCBlockSeqno(multiple txs in one block) - Blocks without matching transactions are skipped, creating gaps
pkg/logpoller/service.go
Outdated
| errsOut <- fmt.Errorf("failed to resolve masterchain block seqno: %w", err) | ||
| continue |
There was a problem hiding this comment.
will this cause any txs/events to be dropped permanently?
There was a problem hiding this comment.
Yeah, I revisited the logic and I would go soft with storing masterchain block number. I'll add retry logic, but allowing default masterchain block number 0 to avoid data loss. In worst case we'll have a few records with mcBlock=0, but it's used only for determining the highest block number processed, and it's safer than losing logs
pkg/logpoller/block.go
Outdated
| "err", err) | ||
| } else if exists { | ||
| if dbSeqno == 0 { | ||
| return 0, errors.New("database contains logs with master_block_seqno=0, which indicates data corruption") |
There was a problem hiding this comment.
are there any 0 seqNos in the current DB? will this cause failures during or right after a migration when these changes are deployed for the first time?
There was a problem hiding this comment.
Good catch, lowered it to warning
| return 0, nil | ||
| } | ||
|
|
||
| // Validate logs before expensive conversion to fail fast |
4 participants
Primary Changes
GetLatestMasterBlockSeqnomethod to retrieve the highest masterchain block sequence number from stored logs, enabling service restart resumptiongetLastProcessedBlockto check stored state before falling back to lookback window calculationOthers
GetLatestMasterBlockSeqnofunctionality