limit the height range when querying getBlock in FVM #5607
Conversation
| @@ -142,6 +142,10 @@ func (info *blockInfo) GetBlockAtHeight( | |||
| return runtimeBlockFromHeader(info.blockHeader), true, nil | |||
| } | |||
|
|
|||
| if height+uint64(flow.DefaultTransactionExpiry) < info.blockHeader.Height { | |||
There was a problem hiding this comment.
When creating protocol snapshot, we are already adding X number of extra blocks below the root block to the protocol snapshot, where X is flow.DefaultTransactionExpiry, which currently is set to 600.
So all we need is to limit the API to not go beyond that limit, as long as the queried height is above RootFinalizedBlock.Height - DefaultTransactionExpiry, then the block must have been indexed already, and available to be queried.
There was a problem hiding this comment.
Should this use the actual transaction expiry instead of the default one?
There was a problem hiding this comment.
The actual transaction expiry would only be less than the default transaction expiry.
So it's safe and simple to use the default.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5607 +/- ##
==========================================
+ Coverage 55.63% 55.70% +0.07%
==========================================
Files 1040 1032 -8
Lines 101659 101254 -405
==========================================
- Hits 56558 56404 -154
+ Misses 40748 40506 -242
+ Partials 4353 4344 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| limitHeight := s.state.sporkRootBlockHeight | ||
| if head.Header.Height > s.state.sporkRootBlockHeight+flow.DefaultTransactionExpiry { | ||
| limitHeight = head.Header.Height - flow.DefaultTransactionExpiry | ||
| if blockSealedAtHead.Height > s.state.sporkRootBlockHeight+flow.DefaultTransactionExpiry { |
There was a problem hiding this comment.
Let me explain why changing this:
When using head.Header.Height, the number of blocks gets included in extra blocks is less than 600 (DefaultTransactionExpiry), why?
For instance, let's say block 800 seals block 792, if we create a protocol snapshot for block 800, then the SealingSegment.Blocks will include 8 blocks [block_792, block_800], and the ExtraBlocks field will include 592 blocks [block_200, block_791], since block_792 is the last sealed block, there are 592 blocks below the sealed block, which is not enough (we need 600). why?
Because, if an execution node is dynamically bootstrapped, it will re-execute from next unsealed block (block_793). Let's use the above example again, if we bootstrap an execution node with the protocol snapshot for block 800, then it will re-execute blocks [block_793, block_800], if a transaction in block_793 tries to query block at height 193 (793 - 600 = 193), then it will fail, because the protocol snapshot only includes block starting from height 200.
In order to prevent this error, we would have to take the 8 blocks [block_792, block_800]` into consideration, which is a bit complex.
A simpler approach is just to index a bit more blocks by indexing 600 blocks below the blockSealedAtHead, which means we will include exactly 600 blocks [block_192, block791] in the ExtraBlocks field to be indexed. And then, it will guarantee we always have 600 blocks below the root sealed block height indexed, and the block height limit we set on getBlock query should always work.
6e88d27 to
506657e
Compare
model/flow/sealing_segment.go
Outdated
| @@ -33,7 +33,8 @@ type SealingSegment struct { | |||
| // (see sealing_segment.md for details): | |||
| // (ii) All blocks that are sealed by `head`. This is relevant if `head` contains _multiple_ seals. | |||
| // (iii) The sealing segment holds the history of all non-expired collection guarantees, i.e. | |||
| // limitHeight := max(head.Height - flow.DefaultTransactionExpiry, SporkRootBlockHeight) | |||
| // limitHeight := max(blockSealedAtHead.Height - flow.DefaultTransactionExpiry, SporkRootBlockHeight) | |||
| // where blockSealedAtHead is the block sealed by `head` block. | |||
There was a problem hiding this comment.
mixed tabs and spaces
| @@ -142,6 +142,10 @@ func (info *blockInfo) GetBlockAtHeight( | |||
| return runtimeBlockFromHeader(info.blockHeader), true, nil | |||
| } | |||
|
|
|||
| if height+uint64(flow.DefaultTransactionExpiry) < info.blockHeader.Height { | |||
There was a problem hiding this comment.
Should this use the actual transaction expiry instead of the default one?
Close https://github.com/dapperlabs/flow-go/issues/6939