trie: allow trie put/del ops to skip transforming key for snapsync #2950
Conversation
g11tech
added
PR state: WIP
package: client
package: mpt
target: master
g11tech
changed the title
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
g11tech
force-pushed
the
trie-ops-skipkeytransform
branch
from
84e9e00 to
f23310a
Compare
g11tech
force-pushed
the
trie-ops-skipkeytransform
branch
from
751eec4 to
f313f8a
Compare
|
coverage for snap related fetchers will increase as we do followup PRs for integration |
There was a problem hiding this comment.
This is looking great! One of the tests fail at the end, seemingly due to a missing account:
FAIL test/sim/snapsync.spec.ts > simple mainnet test run > should match entire state
TypeError: Cannot read properties of undefined (reading 'balance')
❯ test/sim/snapsync.spec.ts:201:17
199| console.log(addressString)
200| assert.equal(
201| account.balance,
| ^
202| BigInt(customGenesisState[addressString][0]),
203| `${addressString} balance should match`
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯
Test Files 1 failed (1)
Tests 1 failed | 3 passed | 1 skipped (5)
Start at 13:59:28
Duration 63.49s (transform 1.97s, setup 0ms, collect 43.23s, tests 19.60s, environment 0ms, prepare 213ms)
But since the roots match, I suspect it's not an issue with the fetchers. Does the test also fail for you when you run the sim test suit locally?
hey no, all tests pass for me... let me do some variations of the cmd, might be an edge case somewhere |
| @@ -376,7 +384,8 @@ export class AccountFetcher extends Fetcher<JobTask, AccountData[], AccountData> | |||
| const storageFetchRequests = new Set() | |||
| const byteCodeFetchRequests = new Set<Uint8Array>() | |||
| for (const account of result) { | |||
| await this.accountTrie.put(account.hash, accountBodyToRLP(account.body)) | |||
| // what we have is hashed account and not its pre-image, so we skipKeyTransform | |||
| await this.accountTrie.put(account.hash, accountBodyToRLP(account.body), true) | |||
There was a problem hiding this comment.
Does account.hash = keccak(accountBodyToRLP(account.body)) ?
If I understand this correctly, we would end up with a Trie where:
accountTrie.get(accountBodyToRLP(account.body)) = accountBodyToRLP(account.body)
There was a problem hiding this comment.
i think you got the keyvalue pair mixed up, first is key which is already hashed account lookup key second is value
There was a problem hiding this comment.
@ScottyPoi to my knowledge, the second one should be accountTrie.get(account.hash) = accountBodyToRLP(account.body) where account.hash is the keccak256 hash of the account address that is derived from the public key of the account's corresponding private key (and can be looked up by walking the account trie and concatenating the key nibbles). The account hash is not derived from the account body.
There was a problem hiding this comment.
sweet, that makes perfect sense. I was worried that we were mixing up the account address with the DB Key, that's all.
LGTM, 👍
packages/trie/src/trie.ts
Outdated
There was a problem hiding this comment.
So we're setting up a Trie where the key used for put and del is different than the key used for get?
There was a problem hiding this comment.
correct because in snap sync we already get hashed key in the AccountData for e.g. (account.hash for e.g.)
There was a problem hiding this comment.
We are putting by post-image (and skipping the hashing from pre to post-image) and reading by pre-image, which would be the account address.
i fixed the edge case, thanks for spotting it 🙂 |
Snap sync requires building the MPT from the peer tree where key is already hashed. This PR allows trie put/del opts to skip key transform even if trie is set with
useKeyHashing. This allows to correctly build the MPT and correctly map addresse lookups to the trieTODO: