Implement BlockSnapshotProvider & BlockSnapshot interfaces
#632
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
m-Peter
changed the base branch from
main
to
mpeter/aggregate-evm-event-payloads
storage/pebble/blocks_provider.go
Outdated
| latestBlockPayload *events.BlockEventPayload | ||
| } | ||
|
|
||
| var _ evmTypes.BlockSnapshotProvider = (*BlocksProvider)(nil) |
There was a problem hiding this comment.
the existing Blocks in blocks.go could implement this
There was a problem hiding this comment.
Unfortunately that is not possible, in my opinion. Inside the processEvents of the Engine type, we want to first replay the received EVM block and all its transactions, and if no error or state changes occur, we'll then continue with updating the local state index, the blocks / transactions / receipts indices. Which means, that the Blocks index, does not yet have that current block which is still being processed and replayed.
That's why I have added the OnBlockReceived & OnBlockExecuted methods, to keep track of the latest block that we're processing in BlocksProvider. This will allow the GetSnapshotAt and BlockContext methods to work properly.
storage/pebble/blocks_provider.go
Outdated
| } | ||
|
|
||
| func (bp *BlocksProvider) OnBlockReceived(blockEvent *events.BlockEventPayload) error { | ||
| if bp.latestBlockPayload != nil { |
There was a problem hiding this comment.
instead of using latestBlockPayload evmTypes.BlockSnapshot should be implemented by a separate object.
something like
type blockSnapshot {
BlocksProvider
height uint64
}
the block context would then look like:
func (bs *blockSnapshot) BlockContext() (evmTypes.BlockContext, error) {
return bs.BlocksProvider.BlockContext(bs.height)
}
while GetSnapshotAt would look like:
func (bp *BlocksProvider) GetSnapshotAt(height uint64) (
evmTypes.BlockSnapshot,
error,
) {
return *blockSnapshot{
BlocksProvider: bp,
height: height
}, nil
}
There was a problem hiding this comment.
Good point 👍 The BlocksProvider is rather counter-intuitive, with implementing both interfaces, so I have moved the BlockSnapshot interface to a dedicated object. See: d9890bf#diff-8eafa2e76b08bd95e6922ee08584718a271ae9f9bf7d1d18bd6c959ee6edb7a3R23-R28
m-Peter
force-pushed
the
mpeter/implement-block-snapshot-provider
branch
from
2dc415a
to
db86472
Compare
m-Peter
requested review from
peterargue,
zhangchiqing and
ramtinms
as code owners
m-Peter
force-pushed
the
mpeter/implement-block-snapshot-provider
branch
2 times, most recently
from
2211feb
to
d9890bf
Compare
storage/pebble/blocks_provider.go
Outdated
| return fmt.Errorf("received nil block") | ||
| } | ||
|
|
||
| if bp.latestBlock != nil { |
There was a problem hiding this comment.
Is it possible to eliminate this case by initializing BlocksProvider with a block?
There was a problem hiding this comment.
I've removed this in 3cbf6ae#diff-b3cc424323a936972f841745ae20117fcd028cecb7305a8ee4054023ca021e63R78-R90.
Basically I was thinking of BlocksProvider as a long-lived object, that is wired in the event ingestion component.
Every time a new EVM block comes in (a.k.a latest block here, which is the latest received block from the Event Streaming API, that has not been replayed & indexed yet in EVM Gateway).
Right now, I only check that the received Block is sequential to the latest block, if there's any.
| b.storages.Blocks, | ||
| b.config.FlowNetworkID, | ||
| tracer, | ||
| ) |
There was a problem hiding this comment.
can we read the latest block and initialize the blocks provider with it?
There was a problem hiding this comment.
See #632 (comment).
The latestBlock field of BlocksProvider is not the latest indexed block on the EVM Gateway's DB.
It is the latest block the event ingestion engine has received, and the one that should be replayed. If no errors or state mismatches occur, this block will be saved in the EVM Gateway's DB.
| blocks := setupBlocksDB(t) | ||
| blocksProvider := NewBlocksProvider(blocks, flowGo.Emulator, nil) | ||
|
|
||
| err := blocksProvider.OnBlockReceived(nil) |
There was a problem hiding this comment.
Is this a valid case? I think it's the caller's responsibility to ensure the block is not nil.
There was a problem hiding this comment.
Good point. I've removed this in 3cbf6ae.
m-Peter
force-pushed
the
mpeter/implement-block-snapshot-provider
branch
from
d9890bf
to
3cbf6ae
Compare
| // If the given block height, is more than 256 blocks | ||
| // in the past, return an empty block hash. | ||
| if bs.block.Height-n > 256 { | ||
| return gethCommon.Hash{} | ||
| } |
There was a problem hiding this comment.
| // If the given block height, is more than 256 blocks | |
| // in the past, return an empty block hash. | |
| if bs.block.Height-n > 256 { | |
| return gethCommon.Hash{} | |
| } |
I think bs.blocks.GetByHeight(n) handles this case already
There was a problem hiding this comment.
If I remove this line, then the relevant test case fails with:
Not equal:
expected: 0x0000000000000000000000000000000000000000000000000000000000000000
actual : 0x5bc86e647028e549fd4e95bd845fee4ea79f4e32eef71ebe2d0aa5cf3dc64985That's because bs.blocks.GetByHeight(n) is using the Blocks DB of the EVM Gateway, which contains all blocks since genesis block. See: https://github.com/onflow/flow-evm-gateway/blob/main/storage/pebble/blocks.go#L124-L144.
We keep track of all indexed blocks in EVM Gateway, so we can return the proper response for endpoints such as:
eth_getBlockByNumbereth_getBlockByHasheth_getBlockReceiptsetc
An EVM transaction, however, when it is being replayed by the EVM Gateway, it should have access only to the last 256 blocks. Hence this condition in required in the GetHashFunc of BlockContext type.
Closes: #630
Closes: #631
Description
BlockSnapshotProvider&BlockSnapshotinterfacesBlocksProvidertype to the Event Ingestion Engine, with a defaultcallTracerEVMblocks that are receivedFor contributor use:
masterbranchFiles changedin the Github PR explorer