Use transient store for EVM deferred info #1690
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| am.keeper.SetTxHashesOnHeight(ctx, ctx.BlockHeight(), utils.Filter(utils.Map(evmTxDeferredInfoList, func(i keeper.EvmTxDeferredInfo) common.Hash { return i.TxHash }), func(h common.Hash) bool { return h.Cmp(ethtypes.EmptyTxsHash) != 0 })) | ||
| am.keeper.SetBlockBloom(ctx, ctx.BlockHeight(), utils.Map(evmTxDeferredInfoList, func(i keeper.EvmTxDeferredInfo) ethtypes.Bloom { return i.TxBloom })) | ||
| am.keeper.DeleteAllAnteSurplus(ctx) | ||
| am.keeper.SetTxHashesOnHeight(ctx, ctx.BlockHeight(), utils.Filter(utils.Map(evmTxDeferredInfoList, func(i *types.DeferredInfo) common.Hash { return common.BytesToHash(i.TxHash) }), func(h common.Hash) bool { return h.Cmp(ethtypes.EmptyTxsHash) != 0 })) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| am.keeper.SetBlockBloom(ctx, ctx.BlockHeight(), utils.Map(evmTxDeferredInfoList, func(i keeper.EvmTxDeferredInfo) ethtypes.Bloom { return i.TxBloom })) | ||
| am.keeper.DeleteAllAnteSurplus(ctx) | ||
| am.keeper.SetTxHashesOnHeight(ctx, ctx.BlockHeight(), utils.Filter(utils.Map(evmTxDeferredInfoList, func(i *types.DeferredInfo) common.Hash { return common.BytesToHash(i.TxHash) }), func(h common.Hash) bool { return h.Cmp(ethtypes.EmptyTxsHash) != 0 })) | ||
| am.keeper.SetBlockBloom(ctx, ctx.BlockHeight(), utils.Map(evmTxDeferredInfoList, func(i *types.DeferredInfo) ethtypes.Bloom { return ethtypes.BytesToBloom(i.TxBloom) })) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
There was a problem hiding this comment.
the panic seems to be a sanity check in BytesToBloom which I think should be okay? (all bloom bytes are produced by geth code)
| if err := info.Unmarshal(val); err != nil { | ||
| // unable to unmarshal deferred info is serious, because it could cause | ||
| // balance surplus to be mishandled and thus affect total supply | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
codchen
force-pushed
the
use-transient-store-for-evm-deferred
branch
from
da6304b to
4ec6c09
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1690 +/- ##
==========================================
- Coverage 60.81% 60.76% -0.06%
==========================================
Files 373 373
Lines 27116 27129 +13
==========================================
- Hits 16491 16485 -6
- Misses 9518 9530 +12
- Partials 1107 1114 +7
|
codchen
force-pushed
the
use-transient-store-for-evm-deferred
branch
from
4ec6c09 to
2b8bcb7
Compare
codchen
force-pushed
the
use-transient-store-for-evm-deferred
branch
from
2b8bcb7 to
888db56
Compare
yzang2019
approved these changes
Jun 27, 2024
codchen
force-pushed
the
use-transient-store-for-evm-deferred
branch
from
888db56 to
9902e71
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes and provide context
Previously, we were using an adhoc
sync.Mapto keep track of deferred information that needs to be persisted from each transaction's individual processing toEndBlock(for purposes like surplus settlement, fee aggregation, etc.), where the map is key'ed on transaction index. The assumption of such usage is that the deferred info will be consistent across nodes by the end of the block because each TX index's entry will be overwritten by the final run of that transaction, under the context of OCC.However this assumption is not correct, because a later incarnation of a transaction does not immediately (or ever) terminates its previous incarnation's goroutine (because there is no way to do so in Golang), so it is possible that a former incarnation may finish (and write to deferred info map) later than the final incarnation, potentially causing the state to be inconsistent with other nodes (where the final incarnation does write the last).
This PR changes the store of such deferred info to a transient store that's governed by the multiversion store. Specifically we choose transient store instead of memstore because we don't need any of these deferred info to be carried over to the next block. Using a memstore means we'd need to delete every entry one-by-one at the start of each block, which can add a lot to latency, whereas with a transient store it will simply be replaced with a new store instance for every block. In doing so for deferred info, we also changed the store for ante surplus from memstore to transient store for the same reason.
Note that this change in theory would not affect app state (and thus not app hash breaking, because all relevant states are in-memory)
Testing performed to validate your change
unit test