-
Notifications
You must be signed in to change notification settings - Fork 1.4k
polygon: Use BorSpansIndex to calculate span id at block number #16683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a fundamental change to how span IDs are calculated for block numbers in Polygon. Instead of using a simple arithmetic function, it implements a flexible BorSpansIndex MDBX table to handle overlapping spans that can occur during producer rotations in the upcoming VeBlop hardfork.
Key changes:
- Replaces hardcoded span length calculations with a dynamic range indexer system
- Implements support for overlapping spans where a new span can start before the previous one ends
- Adds comprehensive testing for the new span range index functionality
Reviewed Changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| erigon-lib/event/observers.go | Updates Observer interface to accept context parameter |
| polygon/heimdall/span_range_index.go | Introduces new span range indexer for flexible span ID lookups |
| polygon/heimdall/types.go | Updates snapshot extraction to use dynamic span indexing |
| polygon/heimdall/service.go | Modifies service to use database-backed span lookups |
| db/kv/tables.go | Adds new MDBX tables for span and producer selection indexing |
| eth/backend.go | Updates observer notifications to include context |
Comments suppressed due to low confidence (1)
polygon/heimdall/span_range_index.go:113
- The logic is inverted - when
Putreturns an error, the function returns(false, err)but the comment says "we can continue updating". WhenPutsucceeds (err == nil), it returns(true, nil)but says "we need to stop". The return values should be swapped to match the intended behavior.
if valuePair == nil {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mh0lt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you exposing the heimdallMdbxStore here - it feels like it should be accesses via the heimdall store by the components that need it rather than added into the back-end code. We're trying to remove these dependencies not all more.
taratorio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waiting on #16683 (comment)
This PR changes the way the span id at a given block number is calculated. Whereas before, the span id was calculated via the `SpanIdAt(blockNum)` function, which did a simple arithmetic calculation to get the span id, with the changes in this PR a `spanRangeIndex` `RangeIndexer` is used to calculate the span at given block via a `span.StartBlock -> (span.Id, span.EndBlock)` MDBX table (`kv.BorSpansIndex` for `Span`s, and `kv.BorProducerSelectionsIndex` for `BorProducerSelections` ). The span id for a given `blockNum` is now calculated as: `max span.Id such that blockNum >= span.StartBlock && blockNum <= span.EndBlock` The reasons for this change are 2: 1. In the upcoming VeBlop hardfork (https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-64.md) spans could overlap if there is a producer rotation before the current span completes. E.g. Span 0 is [0 ; 6399], but in block 5 there is a new span with a different producer (due to the block producer of span 0 going down), so Span 1 is [5; 12,799] . Therefore, the span at block 10 for example is no longer Span 0, but Span 1 instead. For this reason, a more flexible approach is needed to determine the span id for a given block. 2. In the old code a hardcoded span length of 6400 blocks was used. This worked fine for Bor mainnet and Amoy, but for Kurtosis testing, Polygon uses a configurable span length (default now is 128 blocks). This means that the Erigon node in the kurtosis setup was misbehaving. With this PR, the kurtosis Polygon tests can work no matter what the span length is, since the span id calculation is more flexible. The old `SpanIdAt()` function is deprecated and only kept in some obsolete tests that use `HeimdallSimulator`, which can be removed in the future. --------- Co-authored-by: antonis19 <[email protected]>
Follow-up to : #16683 The previous PR relied on the assumption that `span.StartBlock` is monotonically increasing, but it turns out that assumption is not true due to a future span being announced in advance from Heimdall and a span rotation taking place before the announced span. e.g. ```Go var spans = []Span{ Span{ Id: 0, StartBlock: 0, EndBlock: 999, }, Span{ Id: 1, // new span announced StartBlock: 1000, EndBlock: 1999, }, Span{ Id: 2, // span rotation StartBlock: 5, EndBlock: 1999, }, } ``` Therefore, this PR adapts the lookup algorithm by starting the search from `entry := cursor.Seek(blockNum)` and walking the `BorSpans` table backwards using `cursor.Prev()` while recording the highest span id which is in range, until an out-of-range span is reached, at which point we break and return the highest span id seen. This lookup algorithm relies on the assumption that there won't ever be more than 2 consecutive spans for which `span1.Id < span2.Id` and `span1.StartBlock > span2.StartBlock` . If this assumption were ever to be broken, we would have to fall back on brute-force linear search. Also, another detail to take into account is that due to the span rotations multiple spans with the same `StartBlock` could be produced, therefore overwriting the value of the index table at that block number. This is not a problem, because if this happens, then the previous span value will become entirely obsolete and no block number could have the old span associated with it. --------- Co-authored-by: antonis19 <[email protected]>
Follow-up to : #16683 The previous PR relied on the assumption that `span.StartBlock` is monotonically increasing, but it turns out that assumption is not true due to a future span being announced in advance from Heimdall and a span rotation taking place before the announced span. e.g. ```Go var spans = []Span{ Span{ Id: 0, StartBlock: 0, EndBlock: 999, }, Span{ Id: 1, // new span announced StartBlock: 1000, EndBlock: 1999, }, Span{ Id: 2, // span rotation StartBlock: 5, EndBlock: 1999, }, } ``` Therefore, this PR adapts the lookup algorithm by starting the search from `entry := cursor.Seek(blockNum)` and walking the `BorSpans` table backwards using `cursor.Prev()` while recording the highest span id which is in range, until an out-of-range span is reached, at which point we break and return the highest span id seen. This lookup algorithm relies on the assumption that there won't ever be more than 2 consecutive spans for which `span1.Id < span2.Id` and `span1.StartBlock > span2.StartBlock` . If this assumption were ever to be broken, we would have to fall back on brute-force linear search. Also, another detail to take into account is that due to the span rotations multiple spans with the same `StartBlock` could be produced, therefore overwriting the value of the index table at that block number. This is not a problem, because if this happens, then the previous span value will become entirely obsolete and no block number could have the old span associated with it. --------- Co-authored-by: antonis19 <[email protected]>
Cherry-picks for : - #16404 - #16683 - #16729 --------- Co-authored-by: antonis19 <[email protected]>
This PR changes the way the span id at a given block number is calculated. Whereas before, the span id was calculated via the `SpanIdAt(blockNum)` function, which did a simple arithmetic calculation to get the span id, with the changes in this PR a `spanRangeIndex` `RangeIndexer` is used to calculate the span at given block via a `span.StartBlock -> (span.Id, span.EndBlock)` MDBX table (`kv.BorSpansIndex` for `Span`s, and `kv.BorProducerSelectionsIndex` for `BorProducerSelections` ). The span id for a given `blockNum` is now calculated as: `max span.Id such that blockNum >= span.StartBlock && blockNum <= span.EndBlock` The reasons for this change are 2: 1. In the upcoming VeBlop hardfork (https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-64.md) spans could overlap if there is a producer rotation before the current span completes. E.g. Span 0 is [0 ; 6399], but in block 5 there is a new span with a different producer (due to the block producer of span 0 going down), so Span 1 is [5; 12,799] . Therefore, the span at block 10 for example is no longer Span 0, but Span 1 instead. For this reason, a more flexible approach is needed to determine the span id for a given block. 2. In the old code a hardcoded span length of 6400 blocks was used. This worked fine for Bor mainnet and Amoy, but for Kurtosis testing, Polygon uses a configurable span length (default now is 128 blocks). This means that the Erigon node in the kurtosis setup was misbehaving. With this PR, the kurtosis Polygon tests can work no matter what the span length is, since the span id calculation is more flexible. The old `SpanIdAt()` function is deprecated and only kept in some obsolete tests that use `HeimdallSimulator`, which can be removed in the future. --------- Co-authored-by: antonis19 <[email protected]>
Follow-up to : #16683 The previous PR relied on the assumption that `span.StartBlock` is monotonically increasing, but it turns out that assumption is not true due to a future span being announced in advance from Heimdall and a span rotation taking place before the announced span. e.g. ```Go var spans = []Span{ Span{ Id: 0, StartBlock: 0, EndBlock: 999, }, Span{ Id: 1, // new span announced StartBlock: 1000, EndBlock: 1999, }, Span{ Id: 2, // span rotation StartBlock: 5, EndBlock: 1999, }, } ``` Therefore, this PR adapts the lookup algorithm by starting the search from `entry := cursor.Seek(blockNum)` and walking the `BorSpans` table backwards using `cursor.Prev()` while recording the highest span id which is in range, until an out-of-range span is reached, at which point we break and return the highest span id seen. This lookup algorithm relies on the assumption that there won't ever be more than 2 consecutive spans for which `span1.Id < span2.Id` and `span1.StartBlock > span2.StartBlock` . If this assumption were ever to be broken, we would have to fall back on brute-force linear search. Also, another detail to take into account is that due to the span rotations multiple spans with the same `StartBlock` could be produced, therefore overwriting the value of the index table at that block number. This is not a problem, because if this happens, then the previous span value will become entirely obsolete and no block number could have the old span associated with it. --------- Co-authored-by: antonis19 <[email protected]>
…17095) Cherry-picks of : #16683 #16729 #16803 #16819 #16880 #16951 #17014 --------- Co-authored-by: antonis19 <[email protected]> Co-authored-by: Mark Holt <[email protected]>
This PR changes the way the span id at a given block number is calculated. Whereas before, the span id was calculated via the `SpanIdAt(blockNum)` function, which did a simple arithmetic calculation to get the span id, with the changes in this PR a `spanRangeIndex` `RangeIndexer` is used to calculate the span at given block via a `span.StartBlock -> (span.Id, span.EndBlock)` MDBX table (`kv.BorSpansIndex` for `Span`s, and `kv.BorProducerSelectionsIndex` for `BorProducerSelections` ). The span id for a given `blockNum` is now calculated as: `max span.Id such that blockNum >= span.StartBlock && blockNum <= span.EndBlock` The reasons for this change are 2: 1. In the upcoming VeBlop hardfork (https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-64.md) spans could overlap if there is a producer rotation before the current span completes. E.g. Span 0 is [0 ; 6399], but in block 5 there is a new span with a different producer (due to the block producer of span 0 going down), so Span 1 is [5; 12,799] . Therefore, the span at block 10 for example is no longer Span 0, but Span 1 instead. For this reason, a more flexible approach is needed to determine the span id for a given block. 2. In the old code a hardcoded span length of 6400 blocks was used. This worked fine for Bor mainnet and Amoy, but for Kurtosis testing, Polygon uses a configurable span length (default now is 128 blocks). This means that the Erigon node in the kurtosis setup was misbehaving. With this PR, the kurtosis Polygon tests can work no matter what the span length is, since the span id calculation is more flexible. The old `SpanIdAt()` function is deprecated and only kept in some obsolete tests that use `HeimdallSimulator`, which can be removed in the future. --------- Co-authored-by: antonis19 <[email protected]>
Follow-up to : #16683 The previous PR relied on the assumption that `span.StartBlock` is monotonically increasing, but it turns out that assumption is not true due to a future span being announced in advance from Heimdall and a span rotation taking place before the announced span. e.g. ```Go var spans = []Span{ Span{ Id: 0, StartBlock: 0, EndBlock: 999, }, Span{ Id: 1, // new span announced StartBlock: 1000, EndBlock: 1999, }, Span{ Id: 2, // span rotation StartBlock: 5, EndBlock: 1999, }, } ``` Therefore, this PR adapts the lookup algorithm by starting the search from `entry := cursor.Seek(blockNum)` and walking the `BorSpans` table backwards using `cursor.Prev()` while recording the highest span id which is in range, until an out-of-range span is reached, at which point we break and return the highest span id seen. This lookup algorithm relies on the assumption that there won't ever be more than 2 consecutive spans for which `span1.Id < span2.Id` and `span1.StartBlock > span2.StartBlock` . If this assumption were ever to be broken, we would have to fall back on brute-force linear search. Also, another detail to take into account is that due to the span rotations multiple spans with the same `StartBlock` could be produced, therefore overwriting the value of the index table at that block number. This is not a problem, because if this happens, then the previous span value will become entirely obsolete and no block number could have the old span associated with it. --------- Co-authored-by: antonis19 <[email protected]>
This PR changes the way the span id at a given block number is calculated. Whereas before, the span id was calculated via the
SpanIdAt(blockNum)function, which did a simple arithmetic calculation to get the span id, with the changes in this PR aspanRangeIndexRangeIndexeris used to calculate the span at given block via aspan.StartBlock -> (span.Id, span.EndBlock)MDBX table (kv.BorSpansIndexforSpans, andkv.BorProducerSelectionsIndexforBorProducerSelections).The span id for a given
blockNumis now calculated as:max span.Id such that blockNum >= span.StartBlock && blockNum <= span.EndBlockThe reasons for this change are 2:
In the upcoming VeBlop hardfork (https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-64.md) spans could overlap if there is a producer rotation before the current span completes. E.g. Span 0 is [0 ; 6399], but in block 5 there is a new span with a different producer (due to the block producer of span 0 going down), so Span 1 is [5; 12,799] . Therefore, the span at block 10 for example is no longer Span 0, but Span 1 instead. For this reason, a more flexible approach is needed to determine the span id for a given block.
In the old code a hardcoded span length of 6400 blocks was used. This worked fine for Bor mainnet and Amoy, but for Kurtosis testing, Polygon uses a configurable span length (default now is 128 blocks). This means that the Erigon node in the kurtosis setup was misbehaving. With this PR, the kurtosis Polygon tests can work no matter what the span length is, since the span id calculation is more flexible.
The old
SpanIdAt()function is deprecated and only kept in some obsolete tests that useHeimdallSimulator, which can be removed in the future.