Ledger based BAL tracker#4269
Conversation
|
|
||
| proc execPrecompile*(c: Computation, precompile: Precompiles) = | ||
| if c.balTrackerEnabled: | ||
| c.vmState.balTracker.trackAddressAccess(precompileAddrs[precompile]) |
There was a problem hiding this comment.
If removing this, how do precompile accesses get tracked?
There was a problem hiding this comment.
These two places ensure both top level frame and child frame record the access to precompile address.
There was a problem hiding this comment.
Actually it looks like there is a missing performance optimisation here. Inside the Ledger addBalance function if delta.isZero we always load the account to check if it is empty for EIP-161 but empty accounts are no longer possible so we could skip this check at some fork boundary (at the merge if I'm not mistaken).
If we apply this optimisation then we might need to track the precompile address separately but I'm pushing for removing precompiles that don't touch the state to be removed from the BAL spec.
There was a problem hiding this comment.
Even for delegate call we shouldn't need to load the code when we know the address is a precompile. This is another optimisation we can do.
There was a problem hiding this comment.
Probably better we still track the precompile addresses separately until/if it gets removed from the spec so that we can apply these optimisations in our EVM to reduce the amount of state accesses.
There was a problem hiding this comment.
I'm working on a few PRs to add in these optimisations.
| c.execCallOrCreate() | ||
| ledger.persist(clearEmptyAccount = true) | ||
| if vmState.balTrackerEnabled: | ||
| vmState.balLedger.writeToTxFrameAndBAL(ledger) |
There was a problem hiding this comment.
This means we now write to the BAL builder and check for diffs after every system call which might increase the overall work. Previously we only check if there is a diff to write into the BAL in commitCallFrame after processing all system calls. Will be interesting to see how this performs in benchmarks
There was a problem hiding this comment.
Actually this could lead to an incorrect BAL in theory if one system call updated state from A -> B then another system call updates state from B -> A. Might be unlikely to happen in practice but is possible depending on what state updates the system calls do. The diff checking needs to be done once after processing all pre execution system calls and then once after processing all post execution system calls.
There was a problem hiding this comment.
If the concerns is about different system call modifies the same account, the BAL builder will only record the last diff. As long as the balAccessIndex is the same, it only moves the recording process from BAL tracker to BAL builder.
There was a problem hiding this comment.
The problem is that the builder doesn't handle diff tracking and the diff check runs in writeToTxFrameAndBAL which runs after every system call. So if one pre execution system call (at BAL index 0) writes the state of account 1 from A->B then a B write is added into the builder and then if another system call writes the state of account 1 from B->A then another A write is added to the builder for the account. But in this case there should be no diff because account 1 ends up with the same state as before and so the BAL should have no write included. This would cause the balHash to change.
There was a problem hiding this comment.
I see, this will be a tough nut to crack in the ledger.
There was a problem hiding this comment.
You might be able to move the writeToTxFrameAndBAL/persist to the point where the previous commitCallFrame was and remove it from this location.
|
For historical context the BlockAccessListTracker was loosely based on the BAL tracker in the execution specs code see here: https://github.com/ethereum/execution-specs/pull/1338/changes#diff-bf1fcde011d1f672973f27cd5a023f0921d07b186d5742e76e8d8e77c04b639e Looks like that part is now moved here: https://github.com/ethereum/execution-specs/pull/1719/changes?mode=single#diff-743bc9d8436e6ac24492b77b87fd11f64bdc40dd8d568e4a449f21c4df198261 Other reasons for implementing the tracker separate from the ledger initially:
|
|
The downside of using the ledger for the tracking is that the BAL must match exactly the state read and written by the ledger. Basically the state reading in the ledger and the BAL tracking are now coupled together in a way that could in theory create problems in the future if we ever run into a scenario where our state reads/writes need to deviate from what is tracked in the BAL. |
|
You are correct about the downside, but it only affecting state reads. Ephemeral account access related to EIP-6780 will not leak to BAL. Other state writes should appear in the BAL. Currently, there is only one place where we use state reads that is not recorded to bal: read nonce in the txpool and remove transaction with invalid nonce. Beside the downside, there is also the upside of this design. The code can clearly detect missing test case, and I have found 2 so far, and both of them are being included in the next test release. Using a separate BAL tracker will be very difficult to know what missing test case there are. But of course we have to benchmark both of them before we decide which one to use. |
Yeah good points. I support this change overall since it cleans up the EVM code somewhat. Probably the main deciding factor should be the benchmarks. |
| vmState.balLedger.setBlockAccessIndex(pst.packedTxs.len() + 1) | ||
|
|
||
| # Find out what to do next: accepting this tx or trying the next account | ||
| let rc = processTransaction(vmState, item.tx, item.sender, rollbackReads = true) |
There was a problem hiding this comment.
How does this function correctly without rolling back the reads? I don't see this rollbackReads functionality implemented in the new ledger bal tracking.
If I remember correctly when executing a transaction here if it fails we don't want the reads because the transaction won't be included in the block.
There was a problem hiding this comment.
BAL only record the root ledger.savePoint, ledger.rollback will prevent the accessed accounts appear in the root savePoint.
There was a problem hiding this comment.
And we use separate ledger for txPool and block executor. So no need for rollbackReads.
|
|
||
| if ledger.enableBalTracker: | ||
| if address notin ledger.accountRead: | ||
| ledger.accountRead[address] = HashSet[UInt256]() |
There was a problem hiding this comment.
The BAL tracking here is now dependent on the caching. If the ledger.cache already contains the account then we don't track the read. If we were to change the lifetime of the ledger cache in the future the BAL tracking might break.
In my opinion it would be better if the caching does not effect the BAL tracking.
There was a problem hiding this comment.
This is added to track account accessed in EVM child frame, but the account itself is removed.
Another approach is to keep the zombified accounts around and accountRead table can be removed.
There was a problem hiding this comment.
I think recording the accountReads like this is fine, I just meant that perhaps the read should be tracked before checking the cache so that the read is still tracked on a cache hit.
Uh oh!
There was an error while loading. Please reload this page.