Add 9 bytes of metadata to FlatKV store#3163
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3163 +/- ##
==========================================
- Coverage 59.02% 58.71% -0.31%
==========================================
Files 2065 2106 +41
Lines 169349 173549 +4200
==========================================
+ Hits 99960 101904 +1944
- Misses 60630 62530 +1900
- Partials 8759 9115 +356
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // Setup // | ||
| /////////// | ||
| s.phaseTimer.SetPhase("apply_change_sets_prepare") | ||
| s.pendingChangeSets = append(s.pendingChangeSets, changeSets...) |
There was a problem hiding this comment.
ApplyChangeSets now appends to pendingChangeSets before validation/processing succeeds, while Commit writes pendingChangeSets straight into the WAL. If one ApplyChangeSets fails and the caller later commits, the WAL can contain changes that were never reflected in the in-memory writes/LtHash, and catchup will replay bad data.
consider only commit to s.pendingChangeSets, s.accountWrites, etc. after the entire function succeeds
There was a problem hiding this comment.
Moved to end of function
|
|
||
| // GetBlockHeightModified returns the block height at which the key was last modified. | ||
| // If not found, returns (-1, false, nil). | ||
| func (s *CommitStore) GetBlockHeightModified(key []byte) (int64, bool, error) { |
There was a problem hiding this comment.
For an EOA (zero code-hash), Get(codeHashKey) returns (nil, false, nil), correctly reporting not found.
But GetBlockHeightModified(codeHashKey) returns (blockHeight, true, nil) because it doesn't check for zero code-hash.
Inconsistent API semantics.
There was a problem hiding this comment.
This actually already works the way you say it should work.
case evm.EVMKeyNonce, evm.EVMKeyCodeHash:
accountData, err := s.getAccountData(keyBytes)
if err != nil {
return -1, false, err
}
if accountData == nil || accountData.IsDelete() {
return -1, false, nil
}
func (a *AccountData) IsDelete() bool {
if a == nil {
return true
}
for i := accountBalanceStart; i < accountDataLength; i++ {
if a.data[i] != 0 {
return false
}
}
return true
}
The accountData.IsDelete() method returns true if the encoded value is all zeros, making the function as a whole return -1, false, nil. We can't check the byte array directly, since it will contain non-zero metadata even when the value is a deletion operation.
| } | ||
| return cd.GetBlockHeight(), true, nil | ||
| default: | ||
| return -1, false, fmt.Errorf("unsupported key type: %v", kind) |
There was a problem hiding this comment.
should we add EVMKeyLegacy switch support here? @yzang2019
There was a problem hiding this comment.
@yzang2019 told me we didn't need to store block height info for legacy data. This will mean we won't be able to form BUD state proofs on the legacy data, but maybe that's ok? I'm open to discussion on this if there isn't agreement.
There was a problem hiding this comment.
Yeah that's fine, BUD state proofs are only for EVM state, not for legacy state
|
file name spelling: |
| // Process incoming legacy changes into a form appropriate for hashing and insertion into the DB. | ||
| func processLegacyChanges( | ||
| rawChanges map[string][]byte, | ||
| blockHeight int64, |
There was a problem hiding this comment.
is the blockHeight param being used? and LegacyData has no block height field
There was a problem hiding this comment.
It was originally, but is no longer since we stopped tracking block height for legacy data. Removed.
There was a problem hiding this comment.
Yeah LegacyData doesn't need last modified height since they do not participate in BUD Membership Proof
sei-db/state_db/sc/flatkv/api.go
Outdated
| // Get returns the value for the x/evm memiavl key, or (nil, false) if not found. | ||
| Get(key []byte) ([]byte, bool) | ||
| // Get returns the value for the x/evm memiavl key. If not found, returns (nil, false, nil). | ||
| Get(key []byte) (value []byte, found bool, err error) |
There was a problem hiding this comment.
I'm not big fan of having to return an extra error for each get/has call in this layer, I think if we can avoid, we should not return any error. If there's a database internal error, we can choose to panic out.
There was a problem hiding this comment.
Change made.
I'd like to push back a bit on this style of using panic(), but I don't want to block this PR. We can keep the API the way it currently is, and discuss the coding style question asynchronously.
There was a problem hiding this comment.
No strong opinion on panic here, but I want to avoid critical errors being ignored by upper layer
| if !IsSupportedKeyType(kind) { | ||
| if s.config.StrictKeyTypeCheck { | ||
| return nil, false, fmt.Errorf("unsupported key type: %v", kind) | ||
| } | ||
| logger.Warn("unsupported key type in Get", "kind", kind) |
There was a problem hiding this comment.
I think we don't need this restriction any more. All known EVM keys should just goes to its corresponding EVM stores, and unknown key kind should just goes to legacyDB
There was a problem hiding this comment.
Basically FlatKV should support ALL keys, there's no concept of unsupported keys type
There was a problem hiding this comment.
It already works that way, since evm.ParseEVMKey returns EVMKeyLegacy for an unrecognized key type. The error checking is mostly just there in case we add a new key type but forget to update the code to handle that key type.
Since that is the case, I think it probably isn't necessary to have the switch that allows us to ignore unrecognized keys. We should always consider such a thing to be a bug, and should never ignore it. Updated this part of the code.
This behavior was not well documented. I've also updated documents/comments to call out this behavior.
// evm.ParseEVMKey() should return a valid key type 100% of the time, unless we add a new key but
// forget to update IsSupportedKeyType() and associated code. This is a sanity check.
if !IsSupportedKeyType(kind) {
return nil, fmt.Errorf("unsupported key type: %v", kind)
}
sei-db/state_db/sc/flatkv/config.go
Outdated
|
|
||
| // If true, FlatKV will return an error if it encounters an unsupported key type. Otherwise, | ||
| // it will log a warning and continue. | ||
| StrictKeyTypeCheck bool |
There was a problem hiding this comment.
Not really needed any more, see my other comment
| kind, keyBytes := evm.ParseEVMKey(key) | ||
| if kind == evm.EVMKeyUnknown { | ||
| return nil, false | ||
| if !IsSupportedKeyType(kind) { |
There was a problem hiding this comment.
We can move this check to the switch case under the default and then remove the function
| evm.EVMKeyNonce: {}, | ||
| evm.EVMKeyCodeHash: {}, | ||
| evm.EVMKeyCode: {}, | ||
| evm.EVMKeyLegacy: {}, |
There was a problem hiding this comment.
We can add a todo in evm/keys.go to rename the keys for EVMKeyLegacy
| // Returns (value, true) if found, (nil, false) if not found. | ||
| // Panics on I/O errors or unsupported key types. | ||
| func (s *CommitStore) Get(key []byte) ([]byte, bool) { | ||
| kind, keyBytes := evm.ParseEVMKey(key) |
There was a problem hiding this comment.
We should check EmptyKey separately to avoid panic on empty keys, it's safer to just return empty bytes and not found
| } | ||
| } | ||
|
|
||
| // Sort the change sets by type. This method only returns an error if |
Describe your changes and provide context
In order to support BUD proofs, we need to add "block last modified" metadata to each value. Since we are about to do a migration, it's lower effort in the long run to add this metadata now instead of later.
Testing performed to validate your change
Unit tests