Cache EVM block proposal and defer storage writes to commit#8491
Cache EVM block proposal and defer storage writes to commit#8491
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnvironment now requires EVM block-store capabilities. Block-store implementation and lifecycle (stage/flush/commit/reset) moved into fvm/environment; backends and handlers updated to delegate block operations to the embedded EVMBlockStore. Tests and mocks updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant EVM_Handler as EVM Handler
participant Env as Environment / EVMBlockStore
participant Store as ValueStore (persistent)
end
EVM_Handler->>Env: StageBlockProposal(bp) -- stage in-memory
note right of Env: cached proposal set (no write)
EVM_Handler->>Env: StageBlockProposal(bp2)
note right of Env: cached proposal updated
EVM_Handler->>Env: FlushBlockProposal()
Env->>Store: persist proposal bytes (updateBlockProposal)
Store-->>Env: ack
Env-->>EVM_Handler: flush result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
holyfuchs
force-pushed
the
holyfuchs/6958-evm-optimize-block-formation
branch
from
2f6e450 to
35ac8b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
fvm/evm/backends/wrappedEnv.go (1)
155-158: Consider wrapping error withhandleEnvironmentErrorfor consistency.Other error-returning methods in this file (e.g.,
GetCurrentBlockHeight,ReadRandom,GenerateUUID) wrap their errors withhandleEnvironmentError. This method returns the error directly without wrapping. While the underlying flusher may already return appropriately typed errors, inconsistent error handling patterns could lead to unexpected error propagation behavior.♻️ Proposed fix for error handling consistency
// FlushBlockProposal calls the registered flusher to persist the cached proposal. func (we *WrappedEnvironment) FlushBlockProposal() error { - return we.env.FlushBlockProposal() + return handleEnvironmentError(we.env.FlushBlockProposal()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/backends/wrappedEnv.go` around lines 155 - 158, The FlushBlockProposal method returns the underlying error directly; update it to wrap the returned error using handleEnvironmentError for consistency with other methods. Locate WrappedEnvironment.FlushBlockProposal (currently returning we.env.FlushBlockProposal()) and change it to capture the error from we.env.FlushBlockProposal(), and if non-nil pass it into handleEnvironmentError with the same identifying context used elsewhere (e.g., the method name "FlushBlockProposal") before returning; follow the exact wrapping pattern used by GetCurrentBlockHeight/ReadRandom/GenerateUUID to keep behavior consistent.fvm/evm/handler/blockstore.go (1)
122-127: Flusher is re-registered on everyStageBlockProposalcall.
SetBlockProposalFlusheris called each timeStageBlockProposalis invoked. While functionally correct (the cache simply overwrites the previous flusher), this is redundant after the first call within a transaction. Consider tracking whether the flusher has already been registered to avoid unnecessary closure allocations.That said, this is a minor optimization and may not be worth the added complexity if
StageBlockProposalis called infrequently per transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/handler/blockstore.go` around lines 122 - 127, StageBlockProposal currently calls bs.backend.SetBlockProposalFlusher on every invocation, causing redundant flusher re-registration and closure allocations; add a boolean field on BlockStore (e.g., blockProposalFlusherRegistered) and change StageBlockProposal to only call SetBlockProposalFlusher(bs.FlushBlockProposal) when that flag is false, then set it true; ensure the flag is cleared at the end of the transaction by resetting it inside FlushBlockProposal (or a backend-callback invoked when the flusher runs) so future transactions can re-register the flusher.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/environment/facade_env.go`:
- Line 53: The Reset() method on facadeEnvironment must also clear or
reinitialize the BlockProposalCache to avoid carrying stale proposals across
transactions; update the Reset() implementation (which currently resets
ContractUpdater, EventEmitter, and Programs) to either set
facadeEnvironment.BlockProposalCache = NewBlockProposalCache(...) or call its
Clear/Reset method so the cache is empty after Reset(), and make the same change
wherever the same reset logic is duplicated (the other Reset/initialization call
that touches ContractUpdater/EventEmitter/Programs).
In `@fvm/evm/testutils/backend.go`:
- Around line 241-254: The TestBackend currently disables the deferred-write
path by making SetBlockProposalFlusher and FlushBlockProposal no-ops, so tests
calling FlushPendingUpdates can't detect stale-cache or flush-order bugs; modify
TestBackend to store the flusher passed into SetBlockProposalFlusher (e.g., save
it to a new field like blockProposalFlusher) and implement FlushBlockProposal to
invoke that stored function and return its error (or nil if no flusher is set);
keep CachedBlockProposal and CacheBlockProposal as-is so staged proposals are
retained and flushed via the stored flusher when FlushBlockProposal is called.
---
Nitpick comments:
In `@fvm/evm/backends/wrappedEnv.go`:
- Around line 155-158: The FlushBlockProposal method returns the underlying
error directly; update it to wrap the returned error using
handleEnvironmentError for consistency with other methods. Locate
WrappedEnvironment.FlushBlockProposal (currently returning
we.env.FlushBlockProposal()) and change it to capture the error from
we.env.FlushBlockProposal(), and if non-nil pass it into handleEnvironmentError
with the same identifying context used elsewhere (e.g., the method name
"FlushBlockProposal") before returning; follow the exact wrapping pattern used
by GetCurrentBlockHeight/ReadRandom/GenerateUUID to keep behavior consistent.
In `@fvm/evm/handler/blockstore.go`:
- Around line 122-127: StageBlockProposal currently calls
bs.backend.SetBlockProposalFlusher on every invocation, causing redundant
flusher re-registration and closure allocations; add a boolean field on
BlockStore (e.g., blockProposalFlusherRegistered) and change StageBlockProposal
to only call SetBlockProposalFlusher(bs.FlushBlockProposal) when that flag is
false, then set it true; ensure the flag is cleared at the end of the
transaction by resetting it inside FlushBlockProposal (or a backend-callback
invoked when the flusher runs) so future transactions can re-register the
flusher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce6e3945-12a0-4abf-83dd-c1a4458a1d7f
📒 Files selected for processing (14)
fvm/environment/block_proposal_cache.gofvm/environment/env.gofvm/environment/facade_env.gofvm/environment/mock/block_proposal_cache.gofvm/environment/mock/environment.gofvm/evm/backends/wrappedEnv.gofvm/evm/handler/blockstore.gofvm/evm/handler/blockstore_benchmark_test.gofvm/evm/handler/blockstore_test.gofvm/evm/handler/handler.gofvm/evm/testutils/backend.gofvm/evm/types/backend.gofvm/evm/types/handler.gomodule/state_synchronization/indexer/extended/mock/index_processor.go
| if ok && cached != nil { | ||
| return cached, nil | ||
| } |
There was a problem hiding this comment.
if ok == false, then it must be a bug, and we could just return an error.
| if ok && cached != nil { | |
| return cached, nil | |
| } | |
| if !ok { | |
| return nil, fmt.Errorf("cached block proposal should be *BlockProposal, but got %T", ...) | |
| } | |
| if cached != nil { | |
| return cached, nil | |
| } |
| // facadeEnvironment.FlushPendingUpdates at the end of the Cadence transaction. | ||
| func (bs *BlockStore) FlushBlockProposal() error { | ||
| cached, ok := bs.backend.CachedBlockProposal().(*types.BlockProposal) | ||
| if !ok || cached == nil { |
There was a problem hiding this comment.
same here, ok == false should be an error
| // StageBlockProposal updates the in-memory block proposal cache and registers the | ||
| // persistence flusher on the backend so it is called at the end of the Cadence transaction. | ||
| func (bs *BlockStore) StageBlockProposal(bp *types.BlockProposal) { | ||
| bs.backend.SetBlockProposalFlusher(bs.FlushBlockProposal) |
There was a problem hiding this comment.
This seems to be a redundant call. What about moving it to NewBlockStore? so that it's setup just once. If CacheBlockProposal was never called, FlushBlockProposal will return nil anyway.
There was a problem hiding this comment.
reusableCadenceRuntime is constructed with an empty SwappableEnvironment{} (no inner env), and immediately calls declareStandardLibraryFunctions -> EVMInternalEVMContractValue -> NewBlockStore.
The real environment is only swapped in later when a transaction executes.
NewBlockStore would need to either lazily register the flusher (e.g. on first StageBlockProposal call, which is what it already does), or declareStandardLibraryFunctions would need to be deferred until after the environment swap. The latter would require changes to the Cadence runtime reuse/pooling architecture.
|
This is very nice, but wouldn't it be better to handle this at FVM level ? I have a feeling state writes can also benefit from this. ( when bundling multiple transactions ) |
My understanding is that this optimization is really about metering rather than execution time / cpu cost. We already maintain a cache for the state, so repeated reads/writes within a block are handled there. bundling multiple transactionsI assume you mean multiple transaction in the same block. It wouldn’t really make sense for transactions to get cheaper just because a later transaction touches the same storage slot. |
|
thanks @holyfuchs this makes sense.
Technically I agree, but practically, as we will have some overhead there, not 100% sure. I was thinking more like intermittent read/writes we charge here ( for example: I send some FT to multiple addresses, like airdrop ) but not sure about internals there, @fxamacker may now more, maybe atree already handles those in map ) |
|
@bluesign the cache for state reads/writes on the FVM is already cached, and the cache is fairly close to where it is needed (in fvm transaction state). We could probably get some improvement if it would be cached even higher, but not sure how much that would improve stuff and it would definitely add complexity. It also wouldn't really matter for computation since we don't meter atree reads/writes during EVM execution since we rely on EVM gas metering to do the job there. I would like to look into this for additional performance improvements on the EVM side, but lets do that separately. We actually haven't looked that deeply yet when it comes to optimizing the EVM calls, so there is definitely work to be done there. The block proposal cache change this PR is addressing is "relatively" straightforward and gives a lot of reduction to computation used. (even if we added a cache for all EVM registers later I don't think this effort is wasted/overshadowed, since block proposal needs special handling anyway) Originally we thought that if people needed to run multiple EVM transactions in a cadence transaction, they would just use the batch operations, but it looks like in some cases that is not convenient or possible (when you want the transaction to do some EVM stuff then some cadence stuff and some EVM stuff again). |
@janezpodhostnik To add on top of that, in some cases the developers may not even have a choice here. By that I mean that when you interact with various Cadence contracts, to compose some logic in your own transaction, or even your own smart contract, the EVM transactions may come from contracts outside of the developers reach. So you can end up with 10-15 EVM transactions, that are impossible to glue together in a |
was there a change on this? I am not so up to date with flow-go code base anymore, but I think we were charging those to GW account.
yeah agreed here totally
I was thinking a bit this EVM register cache will cover this. I think this PR is totally fine. As you said more improvements can be another PR, this block proposal is alone a major gain. |
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
Overall I think this is on the right path. I think the FlushBlockProposal method is used in the correct place.
I think the main design issue to address before going into a more detailed review is the reason why the flusher registration is needed in the first place:
The BlockStore is on the EVM store, and has the same life-cycle as the ReusableCadenceRuntime which means it cannot keep its own state, so we need to keep some state for it in the BlockProposalCache. But looking at the BlockStore there is no reason its on the EVM side. The block store would have access to everything it needs on the fvm side as part of the fvm.Environment. Moving the BlockStore to the environment allows the block store to do its own caching and flushing (and reseting in case of erors) while only serving a couple of methods to the EVM side (to get/create the block proposal)
So I propose trying to move the BlockStore to fvm Environment (and call it EVMBlockStore). then handle caching/flushing/reseting on the EVMBlockStore itself.
If there are any dependencies issue we can pull some interfaces out to a separate package,
yeah. This was a fairly recent change that came with the fees re-calibration. If you look at flow-go/fvm/evm/handler/handler.go Line 427 in 46c2193 |
|
@janezpodhostnik oh this is cool, can't we just use blockProposal the same logic, something like: h.backend.RunWithMeteringDisabled(
func() {
bp, err = h.getBlockProposal() // same for updateBlockProposal
})
if err != nil {
return nil, err
}technically EVM is metering transaction overhead. |
|
Well that part isn't really accounted for with EVM gas (it costs 0 EVM gas to make a block proposal) so if it also costs 0 FVM computation, we risk it becoming an exploit. |
|
So this would further reduce computation cost by around 7.3%. |
|
This is pretty good result for this one, it is pretty cadence heavy. in normal EVM heavy transactions ( like https://www.flowscan.io/tx/2f52c22648e252b203134fa94729699d9f74f99aa723882f5dad37b0bf99a5f9 ) it would be like 40%-50% probably. |
Scenario 1: Real transactionSource: Flowscan Tx 53d2d816...
Scenario 2: Real transactionSource: Flowscan Tx 2f52c226...
Scenario 3: artifical a lot of EVM callsSource: Testnet Flowscan Tx c5a7c959...
|
holyfuchs
force-pushed
the
holyfuchs/6958-evm-optimize-block-formation
branch
from
35ac8b4 to
7675a17
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
fvm/environment/evm_block_store_benchmark_test.go (1)
21-28: Misleading error check placement.The
require.NoError(b, err)on line 27 checks the error frombs.BlockProposal()on line 22, but it appears afterStageBlockProposalwhich doesn't return an error. This is technically correct but confusing. Consider moving the check immediately after theBlockProposalcall for clarity.Proposed refactor for clarity
for i := 0; i < txCounts; i++ { bp, err := bs.BlockProposal() require.NoError(b, err) res := testutils.RandomResultFixture(b) bp.AppendTransaction(res) bs.StageBlockProposal(bp) - require.NoError(b, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/evm_block_store_benchmark_test.go` around lines 21 - 28, The require.NoError call is misplaced: it asserts the error returned by bs.BlockProposal() but sits after bs.StageBlockProposal(), which is confusing; move the require.NoError(b, err) to immediately after the bp, err := bs.BlockProposal() call (before using bp and before bp.AppendTransaction), and remove the trailing duplicate require.NoError(b, err) after bs.StageBlockProposal(); this keeps error checking adjacent to the failing call (bs.BlockProposal) and preserves use of bp.AppendTransaction and bs.StageBlockProposal as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/environment/evm_block_store_test.go`:
- Around line 55-63: The call to bs.FlushBlockProposal() in the test ignores its
returned error; update the test to capture that error and assert it's nil (e.g.,
using require.NoError(t, err)) immediately after calling bs.FlushBlockProposal()
so failures are detected; keep the rest of the sequence (bp.TotalSupply set,
bs.StageBlockProposal(bp), then err := bs.FlushBlockProposal();
require.NoError(t, err), then call bs.LatestBlock()).
In `@fvm/environment/evm_block_store.go`:
- Around line 251-254: StageBlockProposal currently ignores the error returned
by updateBlockProposal, so change BlockStore.StageBlockProposal to return an
error and propagate the result of bs.updateBlockProposal(bs.cached) (i.e.,
return the error instead of discarding it); update the EVMBlockStore interface
signature to reflect StageBlockProposal() error and modify
NoEVMBlockStore.StageBlockProposal to implement the new signature (returning nil
or the propagated error as appropriate) so callers can detect storage write
failures.
In `@fvm/evm/handler/blockstore_benchmark_test.go`:
- Around line 26-27: The require.NoError(b, err) after bs.StageBlockProposal(bp)
is asserting a stale error from the earlier BlockProposal call; update the code
to capture StageBlockProposal's return error and assert that instead (e.g., set
err = bs.StageBlockProposal(bp) and then call require.NoError(b, err)), or
remove the stale assertion if StageBlockProposal does not return an error;
ensure the assertion references the error value returned by StageBlockProposal
rather than the previous BlockProposal error.
---
Nitpick comments:
In `@fvm/environment/evm_block_store_benchmark_test.go`:
- Around line 21-28: The require.NoError call is misplaced: it asserts the error
returned by bs.BlockProposal() but sits after bs.StageBlockProposal(), which is
confusing; move the require.NoError(b, err) to immediately after the bp, err :=
bs.BlockProposal() call (before using bp and before bp.AppendTransaction), and
remove the trailing duplicate require.NoError(b, err) after
bs.StageBlockProposal(); this keeps error checking adjacent to the failing call
(bs.BlockProposal) and preserves use of bp.AppendTransaction and
bs.StageBlockProposal as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0098b387-ac76-422c-baca-bc8b4d0030fe
📒 Files selected for processing (30)
cmd/util/cmd/checkpoint-collect-stats/cmd.gofvm/environment/env.gofvm/environment/evm_block_hash_list.gofvm/environment/evm_block_hash_list_test.gofvm/environment/evm_block_store.gofvm/environment/evm_block_store_benchmark_test.gofvm/environment/evm_block_store_test.gofvm/environment/facade_env.gofvm/environment/mock/environment.gofvm/environment/mock/evm_block_store.gofvm/evm/backends/backend.gofvm/evm/backends/wrappedEnv.gofvm/evm/evm_test.gofvm/evm/handler/blockstore_benchmark_test.gofvm/evm/handler/blockstore_test.gofvm/evm/handler/handler.gofvm/evm/handler/handler_test.gofvm/evm/handler/precompiles.gofvm/evm/offchain/blocks/blocks.gofvm/evm/offchain/blocks/provider.gofvm/evm/offchain/storage/ephemeral.gofvm/evm/offchain/storage/readonly.gofvm/evm/offchain/sync/replay.gofvm/evm/testutils/backend.gofvm/evm/testutils/handler.gofvm/evm/testutils/offchain.gofvm/evm/types/handler.gofvm/fvm_test.gofvm/runtime/cadence_function_declarations.gomodule/state_synchronization/indexer/extended/mock/index_processor.go
💤 Files with no reviewable changes (2)
- fvm/evm/types/handler.go
- fvm/runtime/cadence_function_declarations.go
✅ Files skipped from review due to trivial changes (1)
- fvm/environment/mock/evm_block_store.go
🚧 Files skipped from review as they are similar to previous changes (2)
- fvm/evm/handler/blockstore_test.go
- fvm/evm/backends/wrappedEnv.go
holyfuchs
force-pushed
the
holyfuchs/6958-evm-optimize-block-formation
branch
from
7675a17 to
511c8ee
Compare
holyfuchs
force-pushed
the
holyfuchs/6958-evm-optimize-block-formation
branch
from
511c8ee to
baa4e25
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/environment/evm_block_store.go (1)
190-198:⚠️ Potential issue | 🟠 Major
CommitBlockProposalis still doing the final proposal write on the hot path.
fvm/environment/facade_env.go:350-370already callsFlushBlockProposal()during transaction flush. PersistingnewBPhere means every successful commit pays a secondSetValueforLatestBlockProposal, which gives back part of the metering win this change is trying to capture. KeepnewBPinbs.cachedhere and let the final flush own persistence.♻️ Proposed fix
newBP, err := bs.constructBlockProposal() if err != nil { return err } - err = bs.updateBlockProposal(newBP) - if err != nil { - return err - } bs.cached = newBP return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/evm_block_store.go` around lines 190 - 198, CommitBlockProposal currently persists the proposal on the hot path by calling updateBlockProposal(newBP) after constructBlockProposal; remove that persistence so CommitBlockProposal only assigns bs.cached = newBP and returns, letting FlushBlockProposal perform the final write later. Locate the constructBlockProposal / updateBlockProposal usage in CommitBlockProposal, delete the updateBlockProposal call (and its error handling), and ensure bs.cached is set to newBP and CommitBlockProposal returns nil; keep FlushBlockProposal as the sole writer of LatestBlockProposal.
🧹 Nitpick comments (1)
fvm/environment/evm_block_store.go (1)
224-240: Add caching forBlockHashListto avoid repeated storage reads per transaction.
BlockStorecachesBlockProposalbut creates a newBlockHashListon every call togetBlockHashList()(lines 224-225), which always invokesNewBlockHashList()→loadMetaData()→backend.GetValue(). BothBlockHash()(line 216) andCommitBlockProposal()(line 180) callgetBlockHashList(), and multipleBlockHash()calls during a single transaction will re-read the same metadata from storage repeatedly.Add a cached
*BlockHashListfield toBlockStore(similar to thecached *types.BlockProposalpattern) and initialize it once at transaction start, or refactorgetBlockHashList()to cache the instance withinBlockStoreand invalidate only onCommitBlockProposal().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/evm_block_store.go` around lines 224 - 240, BlockStore currently creates a new BlockHashList on every getBlockHashList() call causing repeated storage reads; add a cached field (e.g., cachedBlockHashList *BlockHashList) to the BlockStore struct and change getBlockHashList() to return the cached instance if present, otherwise create, cache, and return it; ensure CommitBlockProposal() (and any other mutation paths) clears/invalidate cachedBlockHashList (similar to the existing cached *types.BlockProposal pattern) so the cache stays consistent; update any constructors/tests as needed to handle the new field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@fvm/environment/evm_block_store.go`:
- Around line 190-198: CommitBlockProposal currently persists the proposal on
the hot path by calling updateBlockProposal(newBP) after constructBlockProposal;
remove that persistence so CommitBlockProposal only assigns bs.cached = newBP
and returns, letting FlushBlockProposal perform the final write later. Locate
the constructBlockProposal / updateBlockProposal usage in CommitBlockProposal,
delete the updateBlockProposal call (and its error handling), and ensure
bs.cached is set to newBP and CommitBlockProposal returns nil; keep
FlushBlockProposal as the sole writer of LatestBlockProposal.
---
Nitpick comments:
In `@fvm/environment/evm_block_store.go`:
- Around line 224-240: BlockStore currently creates a new BlockHashList on every
getBlockHashList() call causing repeated storage reads; add a cached field
(e.g., cachedBlockHashList *BlockHashList) to the BlockStore struct and change
getBlockHashList() to return the cached instance if present, otherwise create,
cache, and return it; ensure CommitBlockProposal() (and any other mutation
paths) clears/invalidate cachedBlockHashList (similar to the existing cached
*types.BlockProposal pattern) so the cache stays consistent; update any
constructors/tests as needed to handle the new field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5525c069-bc28-46d8-879b-dcef1cd588d7
📒 Files selected for processing (29)
cmd/util/cmd/checkpoint-collect-stats/cmd.gofvm/environment/env.gofvm/environment/evm_block_hash_list.gofvm/environment/evm_block_hash_list_test.gofvm/environment/evm_block_store.gofvm/environment/evm_block_store_benchmark_test.gofvm/environment/evm_block_store_test.gofvm/environment/facade_env.gofvm/environment/mock/environment.gofvm/environment/mock/evm_block_store.gofvm/evm/backends/backend.gofvm/evm/backends/wrappedEnv.gofvm/evm/evm_test.gofvm/evm/handler/blockstore_benchmark_test.gofvm/evm/handler/blockstore_test.gofvm/evm/handler/handler.gofvm/evm/handler/handler_test.gofvm/evm/handler/precompiles.gofvm/evm/offchain/blocks/blocks.gofvm/evm/offchain/blocks/provider.gofvm/evm/offchain/storage/ephemeral.gofvm/evm/offchain/storage/readonly.gofvm/evm/offchain/sync/replay.gofvm/evm/testutils/backend.gofvm/evm/testutils/handler.gofvm/evm/testutils/offchain.gofvm/evm/types/handler.gofvm/fvm_test.gofvm/runtime/cadence_function_declarations.go
💤 Files with no reviewable changes (2)
- fvm/runtime/cadence_function_declarations.go
- fvm/evm/types/handler.go
✅ Files skipped from review due to trivial changes (5)
- fvm/environment/evm_block_hash_list_test.go
- fvm/evm/offchain/storage/readonly.go
- cmd/util/cmd/checkpoint-collect-stats/cmd.go
- fvm/evm/handler/blockstore_benchmark_test.go
- fvm/evm/offchain/blocks/provider.go
🚧 Files skipped from review as they are similar to previous changes (11)
- fvm/environment/env.go
- fvm/evm/offchain/sync/replay.go
- fvm/evm/testutils/handler.go
- fvm/environment/evm_block_hash_list.go
- fvm/evm/offchain/blocks/blocks.go
- fvm/evm/handler/handler_test.go
- fvm/environment/evm_block_store_test.go
- fvm/environment/evm_block_store_benchmark_test.go
- fvm/environment/facade_env.go
- fvm/environment/mock/evm_block_store.go
- fvm/evm/handler/precompiles.go
holyfuchs
force-pushed
the
holyfuchs/6958-evm-optimize-block-formation
branch
from
baa4e25 to
5b7f841
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
fvm/environment/evm_block_store_test.go (1)
31-60: Add a read-only / reset regression here.This only exercises the happy-path
StageBlockProposal()→FlushBlockProposal()flow. The new contract also needs a case whereBlockProposal()is read without staging andFlushBlockProposal()is a no-op, plus a case whereResetBlockProposal()drops staged changes; otherwise the dry-run and failed-transaction paths can regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/evm_block_store_test.go` around lines 31 - 60, Add tests covering read-only and reset regression: after initial BlockProposal() call, call BlockProposal() again without StageBlockProposal and then call FlushBlockProposal() and assert no changes (FlushBlockProposal is a no-op and returned proposal equals original); also test ResetBlockProposal by StageBlockProposal() with modified fields (e.g., TotalGasUsed or TotalSupply), call ResetBlockProposal(), then call BlockProposal() and assert the returned proposal does not include staged changes (staged modifications were dropped). Reference BlockProposal, StageBlockProposal, FlushBlockProposal, and ResetBlockProposal when adding these assertions.fvm/environment/facade_env.go (1)
208-214: ConstructEVMBlockStoreafter parse guards are installed.
NewScriptEnv()does this in the safe order, butNewTransactionEnvironment()builds the block store first. SinceNewBlockStore(...)capturesenv.ValueStore,env.BlockInfo, andenv.RandomGenerator, the transaction path currently holds the unwrapped implementations and can bypass the parse-restricted guard layer.Suggested change
- env.EVMBlockStore = NewBlockStore( - params.Chain.ChainID(), - env.ValueStore, - env.BlockInfo, - env.RandomGenerator, - evm.StorageAccountAddress(params.Chain.ChainID()), - ) - env.addParseRestrictedChecks() + + env.EVMBlockStore = NewBlockStore( + params.Chain.ChainID(), + env.ValueStore, + env.BlockInfo, + env.RandomGenerator, + evm.StorageAccountAddress(params.Chain.ChainID()), + )Also applies to: 285-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/facade_env.go` around lines 208 - 214, The EVMBlockStore is constructed too early in NewTransactionEnvironment (via the NewBlockStore call that assigns env.EVMBlockStore), capturing raw env.ValueStore, env.BlockInfo, and env.RandomGenerator before parse guards are wrapped; move the NewBlockStore construction to after the parse-guard installation (mirror the safe ordering used in NewScriptEnv) so env.EVMBlockStore is created with the wrapped/guarded ValueStore, BlockInfo and RandomGenerator instances; update both occurrences (the one at the NewTransactionEnvironment block and the similar block around lines 285-291) to defer NewBlockStore until after the guard setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/environment/evm_block_store.go`:
- Around line 46-53: BlockStore currently only caches a BlockProposal (cached)
but not the reconstructed block-hash list, so getBlockHashList() rebuilds it
from storage on every BLOCKHASH access; add a cached block-hash list field
(e.g., cachedHashList) to BlockStore, modify getBlockHashList() to return the
cached list when present and populate it the first time it is built, and
update/invalidate that cache in CommitBlockProposal() and anywhere the cached
BlockProposal (cached) is set or cleared (so BlockHash(), CommitBlockProposal(),
and any setter that touches cached use the cachedHashList instead of always
reconstructing).
- Around line 75-98: BlockProposal currently sets bs.cached on every read which
makes reads stateful and causes subsequent FlushBlockProposal to write even when
nothing was staged; change the logic so BlockProposal only returns the existing
persisted proposal (using constructBlockProposal when absent) without mutating
bs.cached, introduce or use a separate staged field (e.g., bs.stagedProposal or
mark via StageBlockProposal) and make StageBlockProposal responsible for setting
that staged state, have FlushBlockProposal/CommitBlockProposal only persist when
the staged field is non-nil/dirty, and ensure CommitBlockProposal clears the
staged dirty state after a successful persist; update references in
BlockProposal, FlushBlockProposal, StageBlockProposal, CommitBlockProposal and
constructBlockProposal accordingly.
---
Nitpick comments:
In `@fvm/environment/evm_block_store_test.go`:
- Around line 31-60: Add tests covering read-only and reset regression: after
initial BlockProposal() call, call BlockProposal() again without
StageBlockProposal and then call FlushBlockProposal() and assert no changes
(FlushBlockProposal is a no-op and returned proposal equals original); also test
ResetBlockProposal by StageBlockProposal() with modified fields (e.g.,
TotalGasUsed or TotalSupply), call ResetBlockProposal(), then call
BlockProposal() and assert the returned proposal does not include staged changes
(staged modifications were dropped). Reference BlockProposal,
StageBlockProposal, FlushBlockProposal, and ResetBlockProposal when adding these
assertions.
In `@fvm/environment/facade_env.go`:
- Around line 208-214: The EVMBlockStore is constructed too early in
NewTransactionEnvironment (via the NewBlockStore call that assigns
env.EVMBlockStore), capturing raw env.ValueStore, env.BlockInfo, and
env.RandomGenerator before parse guards are wrapped; move the NewBlockStore
construction to after the parse-guard installation (mirror the safe ordering
used in NewScriptEnv) so env.EVMBlockStore is created with the wrapped/guarded
ValueStore, BlockInfo and RandomGenerator instances; update both occurrences
(the one at the NewTransactionEnvironment block and the similar block around
lines 285-291) to defer NewBlockStore until after the guard setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaa80248-e7e9-49c5-84cd-c74889a356c9
📒 Files selected for processing (27)
cmd/util/cmd/checkpoint-collect-stats/cmd.gofvm/environment/env.gofvm/environment/evm_block_hash_list.gofvm/environment/evm_block_hash_list_test.gofvm/environment/evm_block_store.gofvm/environment/evm_block_store_benchmark_test.gofvm/environment/evm_block_store_test.gofvm/environment/facade_env.gofvm/environment/mock/environment.gofvm/environment/mock/evm_block_store.gofvm/evm/backends/backend.gofvm/evm/backends/wrappedEnv.gofvm/evm/evm_test.gofvm/evm/handler/handler.gofvm/evm/handler/handler_test.gofvm/evm/handler/precompiles.gofvm/evm/offchain/blocks/blocks.gofvm/evm/offchain/blocks/provider.gofvm/evm/offchain/storage/ephemeral.gofvm/evm/offchain/storage/readonly.gofvm/evm/offchain/sync/replay.gofvm/evm/testutils/backend.gofvm/evm/testutils/handler.gofvm/evm/testutils/offchain.gofvm/evm/types/handler.gofvm/fvm_test.gofvm/runtime/cadence_function_declarations.go
💤 Files with no reviewable changes (2)
- fvm/runtime/cadence_function_declarations.go
- fvm/evm/types/handler.go
✅ Files skipped from review due to trivial changes (3)
- fvm/environment/evm_block_hash_list_test.go
- fvm/evm/offchain/blocks/provider.go
- fvm/environment/mock/evm_block_store.go
🚧 Files skipped from review as they are similar to previous changes (11)
- fvm/environment/env.go
- fvm/evm/offchain/storage/readonly.go
- fvm/evm/testutils/offchain.go
- fvm/evm/backends/backend.go
- fvm/evm/handler/handler_test.go
- fvm/fvm_test.go
- fvm/environment/evm_block_hash_list.go
- fvm/evm/handler/precompiles.go
- fvm/evm/testutils/handler.go
- fvm/environment/evm_block_store_benchmark_test.go
- fvm/evm/offchain/blocks/blocks.go
| type BlockStore struct { | ||
| chainID flow.ChainID | ||
| backend types.Backend | ||
| storage ValueStore | ||
| blockInfo BlockInfo | ||
| randGen RandomGenerator | ||
| rootAddress flow.Address | ||
| cached *types.BlockProposal | ||
| } |
There was a problem hiding this comment.
BlockHashList still is not cached per transaction.
BlockStore only keeps cached *types.BlockProposal; getBlockHashList() reconstructs the hash list from storage on every BlockHash() / CommitBlockProposal() path. Repeated BLOCKHASH access inside one Cadence transaction will therefore stay on the metered storage path, which leaves part of the targeted optimization on the table.
Possible direction
type BlockStore struct {
chainID flow.ChainID
storage ValueStore
blockInfo BlockInfo
randGen RandomGenerator
rootAddress flow.Address
cached *types.BlockProposal
+ cachedBlockHashList *BlockHashList
}
@@
func (bs *BlockStore) getBlockHashList() (*BlockHashList, error) {
+ if bs.cachedBlockHashList != nil {
+ return bs.cachedBlockHashList, nil
+ }
+
bhl, err := NewBlockHashList(bs.storage, bs.rootAddress, BlockHashListCapacity)
if err != nil {
return nil, err
}
@@
if bhl.IsEmpty() {
err = bhl.Push(
types.GenesisBlock(bs.chainID).Height,
types.GenesisBlockHash(bs.chainID),
)
if err != nil {
return nil, err
}
}
+ bs.cachedBlockHashList = bhl
return bhl, nil
}Also applies to: 224-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/environment/evm_block_store.go` around lines 46 - 53, BlockStore
currently only caches a BlockProposal (cached) but not the reconstructed
block-hash list, so getBlockHashList() rebuilds it from storage on every
BLOCKHASH access; add a cached block-hash list field (e.g., cachedHashList) to
BlockStore, modify getBlockHashList() to return the cached list when present and
populate it the first time it is built, and update/invalidate that cache in
CommitBlockProposal() and anywhere the cached BlockProposal (cached) is set or
cleared (so BlockHash(), CommitBlockProposal(), and any setter that touches
cached use the cachedHashList instead of always reconstructing).
There was a problem hiding this comment.
♻️ Duplicate comments (2)
fvm/environment/evm_block_store.go (2)
46-53:⚠️ Potential issue | 🟠 MajorCache
BlockHashListon the store too.Lines 46-53 only cache the proposal, but Lines 224-240 still rebuild
BlockHashListfrom storage on everyBlockHash()andCommitBlockProposal()call. RepeatedBLOCKHASHaccess inside one Cadence transaction therefore stays on the metered storage path, so the block-hash half of this optimization is still missing.Possible direction
type BlockStore struct { chainID flow.ChainID storage ValueStore blockInfo BlockInfo randGen RandomGenerator rootAddress flow.Address cached *types.BlockProposal + cachedBlockHashList *BlockHashList } @@ func (bs *BlockStore) getBlockHashList() (*BlockHashList, error) { + if bs.cachedBlockHashList != nil { + return bs.cachedBlockHashList, nil + } + bhl, err := NewBlockHashList(bs.storage, bs.rootAddress, BlockHashListCapacity) if err != nil { return nil, err } @@ if bhl.IsEmpty() { err = bhl.Push( types.GenesisBlock(bs.chainID).Height, types.GenesisBlockHash(bs.chainID), ) if err != nil { return nil, err } } + bs.cachedBlockHashList = bhl return bhl, nil }If the same
BlockStoreinstance can survive failed transactions, clear this cache on reset too. As per coding guidelines, "Use efficient data structures and algorithms as performance is critical in this blockchain protocol implementation."Also applies to: 224-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/evm_block_store.go` around lines 46 - 53, The BlockStore caches only cached *types.BlockProposal* but not the reconstructed BlockHashList, so BlockHash() and CommitBlockProposal() still rebuild the list from storage; extend BlockStore (the struct defined in BlockStore) with a cached BlockHashList field (e.g., blockHashListCache) and update the code paths in BlockHash() and CommitBlockProposal() to use and update that in-memory cache instead of reconstructing from storage; also ensure the existing reset/Reset method clears this new cache (same place that clears cached) so failed transactions don't leak stale data.
75-97:⚠️ Potential issue | 🔴 CriticalTrack dirty proposal state separately from the read cache.
Line 76 makes
BlockProposal()stateful, Line 198 leaves the freshly committed proposal in that same cache, and Lines 251-259 flush any non-nilcache.DryRun()still callsBlockProposal()infvm/evm/handler/handler.go:547-554, whilefvm/environment/facade_env.go:351-358flushes at transaction end. A non-mutating EVM read can therefore persist a new proposal, andCommitBlockProposal()will be written again on the final flush.Minimal fix
type BlockStore struct { chainID flow.ChainID storage ValueStore blockInfo BlockInfo randGen RandomGenerator rootAddress flow.Address cached *types.BlockProposal + dirty bool } @@ if len(data) != 0 { bp, err := types.NewBlockProposalFromBytes(data) if err != nil { return nil, err } bs.cached = bp + bs.dirty = false return bp, nil } @@ bs.cached = bp + bs.dirty = false return bp, nil } @@ err = bs.updateBlockProposal(newBP) if err != nil { return err } bs.cached = newBP + bs.dirty = false return nil } @@ func (bs *BlockStore) ResetBlockProposal() { bs.cached = nil + bs.dirty = false } func (bs *BlockStore) StageBlockProposal(bp *types.BlockProposal) { bs.cached = bp + bs.dirty = true } func (bs *BlockStore) FlushBlockProposal() error { - if bs.cached == nil { + if bs.cached == nil || !bs.dirty { return nil } err := bs.updateBlockProposal(bs.cached) if err != nil { return err } + bs.dirty = false return nil }Also applies to: 194-199, 243-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/evm_block_store.go` around lines 75 - 97, BlockProposal() currently reuses bs.cached for both a read cache and a mutated "dirty" proposal which lets non-mutating reads persist changes; introduce a separate dirty flag (e.g., bs.proposalDirty bool) on BlockStore, ensure BlockProposal() only populates bs.cached without setting bs.proposalDirty, have CommitBlockProposal() set bs.proposalDirty = true when it mutates the proposal, and change the flush/commit logic that currently checks bs.cached to only write to storage when bs.proposalDirty is true and then clear bs.proposalDirty (and keep bs.cached available for reads). Ensure all references to cached-flushing (the flush method and CommitBlockProposal/constructBlockProposal paths) follow this new dirty flag behavior.
🧹 Nitpick comments (1)
fvm/environment/facade_env.go (1)
208-214: Extract theNewBlockStore(...)wiring into one helper.These two constructors already drifted in ordering around
addParseRestrictedChecks(). Pulling the shared block-store setup behind one helper will keep the script and transaction environments from diverging further as this lifecycle grows.Also applies to: 285-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/environment/facade_env.go` around lines 208 - 214, The NewBlockStore wiring for env.EVMBlockStore should be extracted into a single helper to avoid drift between script and transaction environments; create a function (e.g., initBlockStore or setupBlockStore) that accepts the shared inputs (params.Chain.ChainID(), env.ValueStore, env.BlockInfo, env.RandomGenerator, evm.StorageAccountAddress(params.Chain.ChainID())) and returns the constructed BlockStore, then replace the two inline NewBlockStore(...) occurrences (the one assigning env.EVMBlockStore and the other at lines referenced in the comment) with calls to this helper so both environments use the same construction order and logic, and ensure any surrounding calls like addParseRestrictedChecks() remain in the same place relative to the helper call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fvm/environment/evm_block_store.go`:
- Around line 46-53: The BlockStore caches only cached *types.BlockProposal* but
not the reconstructed BlockHashList, so BlockHash() and CommitBlockProposal()
still rebuild the list from storage; extend BlockStore (the struct defined in
BlockStore) with a cached BlockHashList field (e.g., blockHashListCache) and
update the code paths in BlockHash() and CommitBlockProposal() to use and update
that in-memory cache instead of reconstructing from storage; also ensure the
existing reset/Reset method clears this new cache (same place that clears
cached) so failed transactions don't leak stale data.
- Around line 75-97: BlockProposal() currently reuses bs.cached for both a read
cache and a mutated "dirty" proposal which lets non-mutating reads persist
changes; introduce a separate dirty flag (e.g., bs.proposalDirty bool) on
BlockStore, ensure BlockProposal() only populates bs.cached without setting
bs.proposalDirty, have CommitBlockProposal() set bs.proposalDirty = true when it
mutates the proposal, and change the flush/commit logic that currently checks
bs.cached to only write to storage when bs.proposalDirty is true and then clear
bs.proposalDirty (and keep bs.cached available for reads). Ensure all references
to cached-flushing (the flush method and
CommitBlockProposal/constructBlockProposal paths) follow this new dirty flag
behavior.
---
Nitpick comments:
In `@fvm/environment/facade_env.go`:
- Around line 208-214: The NewBlockStore wiring for env.EVMBlockStore should be
extracted into a single helper to avoid drift between script and transaction
environments; create a function (e.g., initBlockStore or setupBlockStore) that
accepts the shared inputs (params.Chain.ChainID(), env.ValueStore,
env.BlockInfo, env.RandomGenerator,
evm.StorageAccountAddress(params.Chain.ChainID())) and returns the constructed
BlockStore, then replace the two inline NewBlockStore(...) occurrences (the one
assigning env.EVMBlockStore and the other at lines referenced in the comment)
with calls to this helper so both environments use the same construction order
and logic, and ensure any surrounding calls like addParseRestrictedChecks()
remain in the same place relative to the helper call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2eabf38b-9ad3-4f73-ba7b-7a461a2cbf28
📒 Files selected for processing (28)
cmd/util/cmd/checkpoint-collect-stats/cmd.gofvm/environment/env.gofvm/environment/evm_block_hash_list.gofvm/environment/evm_block_hash_list_test.gofvm/environment/evm_block_store.gofvm/environment/evm_block_store_benchmark_test.gofvm/environment/evm_block_store_test.gofvm/environment/facade_env.gofvm/environment/mock/environment.gofvm/environment/mock/evm_block_store.gofvm/environment/mock/reusable_cadence_runtime_interface.gofvm/evm/backends/backend.gofvm/evm/backends/wrappedEnv.gofvm/evm/evm_test.gofvm/evm/handler/handler.gofvm/evm/handler/handler_test.gofvm/evm/handler/precompiles.gofvm/evm/offchain/blocks/blocks.gofvm/evm/offchain/blocks/provider.gofvm/evm/offchain/storage/ephemeral.gofvm/evm/offchain/storage/readonly.gofvm/evm/offchain/sync/replay.gofvm/evm/testutils/backend.gofvm/evm/testutils/handler.gofvm/evm/testutils/offchain.gofvm/evm/types/handler.gofvm/fvm_test.gofvm/runtime/cadence_function_declarations.go
💤 Files with no reviewable changes (3)
- fvm/evm/types/handler.go
- fvm/environment/mock/reusable_cadence_runtime_interface.go
- fvm/runtime/cadence_function_declarations.go
✅ Files skipped from review due to trivial changes (6)
- fvm/evm/offchain/storage/readonly.go
- fvm/environment/evm_block_hash_list_test.go
- fvm/evm/offchain/blocks/provider.go
- fvm/evm/backends/wrappedEnv.go
- fvm/evm/testutils/backend.go
- fvm/environment/mock/evm_block_store.go
🚧 Files skipped from review as they are similar to previous changes (9)
- fvm/evm/testutils/offchain.go
- fvm/environment/env.go
- fvm/evm/backends/backend.go
- cmd/util/cmd/checkpoint-collect-stats/cmd.go
- fvm/environment/evm_block_store_benchmark_test.go
- fvm/environment/evm_block_hash_list.go
- fvm/environment/evm_block_store_test.go
- fvm/evm/evm_test.go
- fvm/evm/offchain/storage/ephemeral.go
I doubt it's gonna be called often and the list is much larger don't think it makes sense to cache it.
I don't think its an issue but will follow this up. |
|
Hey @holyfuchs I pushed a commit to reduce the interface size a little bit. Can you check it. |
| return err | ||
| } | ||
|
|
||
| bs.cached = newBP |
There was a problem hiding this comment.
I think we can clear the cache after the block proposal is committed, because it won't be used any more.
| bs.cached = newBP | |
| bs.cached = nil |
The CommitBlockProposal is used at the system tx, which is the last tx in a flow block, so there is no more tx to read this cache.
- Flow Block 100
|--cadence tx 1 (succeed)
|-- EVM Tx A
|-- StageBlockProposal() -> storage unchanged, create cache
|-- EVM Tx B
|-- FlushBlockProposal() -> write cache to storage (LatestBlockProposal), cache = nil
|--cadence tx 2 (failed)
|-- EVM Tx C
|-- StageBlockProposal() -> storage unchanged, create cache
|-- EVM Tx D (fail and revert)
|-- ResetBlockProposal() -> storage unchanged, cache = nil
|--system chunk tx (last tx)
|--heartbeat()
|--CommitBlockProposal() -> write cache to storage (LatestBlock, LatestBlockProposal), cache = nil
There are two keys in storage:
- LatestBlock
- LatestBlockProposal
The relationship is that LatestBlockProposal.Parent == LatestBlock, but when CommitBlockProposal updates the LatestBlock, this equation no longer holds, that's why we need to also call constructBlockProposal() to create a new empty block proposal in order to satisfy the equation (LatestBlockProposal.Parent == LatestBlock). However, we don't have to set cache = nil, because the cache won't be used anymore.
IMO, the CommitBlockProposal could just remove the LatestBlockProposal key after updating LatestBlock, because the bs.BlockProposal() method already handles the case where LatestBlockProposal is missing, we could just reuse the same code path. And then it would look more consistent when we remove both the LatestBlockProposal key and set cache = nil in CommitBlockProposal().
There was a problem hiding this comment.
⏺ Flow Block K Execution
├── Cadence tx 1 (succeed)
│ ├── EVM Tx A
│ │ ├── BlockProposal()
│ │ │ ├── cache miss
│ │ │ ├── read LatestBlockProposal from storage
│ │ │ │ └── (if empty) read LatestBlock from storage (for parent hash, height)
│ │ │ └── cache it
│ │ └── StageBlockProposal() → update cache
│ ├── EVM Tx B
│ │ ├── BlockProposal() → cache hit
│ │ └── StageBlockProposal() → update cache
│ └── [tx end]
│ └── FlushBlockProposal() → write LatestBlockProposal, cache = nil
│
├── Cadence tx 2 (failed)
│ ├── EVM Tx C
│ │ ├── BlockProposal()
│ │ │ ├── cache miss
│ │ │ └── read LatestBlockProposal from storage → cache it
│ │ └── StageBlockProposal() → update cache
│ ├── EVM Tx D
│ │ ├── BlockProposal() → cache hit
│ │ └── StageBlockProposal() → update cache
│ └── [tx fail/revert]
│ └── Reset() → cache = nil, storage unchanged
│
└── System chunk tx (last)
└── heartbeat()
└── CommitBlockProposal()
├── write LatestBlock
├── remove LatestBlockProposal (new proposal will be constructed lazily in next flow block)
└── cache = nil
There was a problem hiding this comment.
I made a PR for this change and added some test cases
https://github.com/onflow/flow-go/pull/8546/changes#diff-ff1a7eeefd68d12571331a6a3c5a285b71dba2614b194826a6541e251d1da957R230
| // Remove LatestBlockProposal key - the new proposal will be constructed lazily | ||
| // on the next BlockProposal() call by reading LatestBlock for parent hash and height. | ||
| err = bs.storage.SetValue( | ||
| bs.rootAddress[:], | ||
| []byte(BlockStoreLatestBlockProposalKey), | ||
| nil, // setting to nil removes the key | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This file was copied from evm/handler/blockstore.go, except here we are removing the last block proposal key instead of creating new one (the new one will be created lazily, see details in this PR)
7 participants
closes: #6958
Problem
Each EVM operation within a Cadence transaction (e.g.
EVM.run) was reading and writing the EVM block proposal to storage on every call. For a Cadence transaction executing N EVM operations, this resulted in N redundant storage round-trips for the same block proposal.Changes
The
BlockStorehas been moved from the EVM layer (fvm/evm/handler) into the FVM environment (fvm/environment) asEVMBlockStore. Since it now lives on the per-transactionenvironment.Environment, it can own its own caching, flushing, and resetting — no separateBlockProposalCacheindirection is needed.The block proposal is cached in-memory for the duration of a Cadence transaction and persisted exactly once at transaction end via
FlushBlockProposal. On transaction failure,ResetBlockProposaldiscards any staged but unflushed changes.BlockHashListwas moved tofvm/environmentas well for the same reasons.The EVM side accesses block proposal operations through the
backends.Backendinterface, which now composesEVMBlockStoredirectly.Benchmarks (Computation Cost)
Summary by CodeRabbit
Refactor
Tests