Bonsai accounts don't have equals() or hashCode() implementations #7845
Conversation
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
|
We might actually rely on this unintended behavior for linea-shomei zk-trielog plugin. The primary difference between the TrieLogFactoryImpl and ZkTrieLogFactory implementation is that we add block number and zero reads to the trielog. But the zk state needs to know about updates and reads. The value of having reads in the trielogs goes beyond just having a diff to apply, but rather is a log of all state access as well as modifications. Incidentally, this is useful for historical block tracing (very cheap historical/archive worldstate). I don't know that adding equals and hashCode implementations will break this. Before we merge we should probably either assert that either this doesn't break anything or make explicit provision for persisting reads (and zero reads) if we are relying on unintended consequences of not having these methods for the zk trielog factory impl |
| @@ -173,6 +173,22 @@ public String toString() { | |||
| + '}'; | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals(final Object o) { | |||
There was a problem hiding this comment.
We should move as much of this up into DiffBasedAccount as possible so that other implementations inherit this (like Verkle). perhaps implement equals in DiffBasedAccount and delegate to an abstract method that implementations are forced to provide.
|
Thanks for the comments @garyschulte I did wonder if the existing behaviour was in any way intentional. In the case I've given in the PR description, there isn't even a state access, just an event. I'm not sure how realistic that is in the real world but it does need considering in terms of whether we want try logs for those types of invocation. If the answer is "this is intentional/needed/useful" then I'm happy to close the PR with that explanation. I think I'll need to take your call on whether that's the case? |
|
It's something we saw while coding Verkle, but we decided not to include it for now until we have time to test that it won't have any side effects on the trielog shipping part. I think before we merge it, we will need to conduct some tests. We need to be sure about these kinds of things to avoid consensus problems as well. Because an account that seems identical, is it really? Since BonsaiAccount inherits from DiffBasedAccount, which has a list of updatedStorage. I don't think we update the stateroot directly when we update this map. So, two accounts can seem identical without actually being so. Maybe it’s fine, but I haven’t had time to verify. I think it will take time to ensure that this modification is safe. |
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
|
I've addressed the code comment re. moving most of the |
PR description
Unlike
StateTrieAccountValue,BonsaiAccountis missing overrides forequals()andhashCode(). This is causing unnecessary entries in the trie logs for accounts that have been loaded but which haven't changed.An example is invoking a method that only emits an event but which doesn't update storage. The account is loaded, the method is invoked, but nothing in the account has changed. It is then (unnecessarily) recorded in the trie-log for that block.