[Fix] deletion proof should be collected in log tracing #247
Conversation
|
L2geth running on this build pass the problemic block discovered from block 624236 while getting block trace. |
| if !bytes.Equal(oldValue.Bytes(), storageValue.Bytes()) && | ||
| bytes.Equal(storageValue.Bytes(), common.Hash{}.Bytes()) { | ||
|
|
||
| if delSibling, err := l.env.StateDB.GetDeletetionProof(contractAddress, storageKey); err != nil { |
There was a problem hiding this comment.
What's the expected performance impact of this?
There was a problem hiding this comment.
Now we need to do a proof on each deletion (SSTORE a non-zero value to 0). The cost depends while considering the storage trie usually is not very high and deletion is not so common I think the impact should be limited.
There was a problem hiding this comment.
Okay, sounds acceptable. We just need to think if this can be triggered maliciously somehow.
There was a problem hiding this comment.
You are right. I just recognized that current implement would bring an addition commition on the corresponding storage trie so it may bring significant cost by a series well designed malicious txs in one block.
Lucikly if we should have separated and constrainted the logtrace only work while obtaining block trace, such a attack is mitigated for the cost only raised in any following nodes used for providng block trace.
And I think we can further push the deletion generation process to commiting step, I would start this idea in another PR.
There was a problem hiding this comment.
Would trace this problem in #253
| for _, bt := range deletionProofs { | ||
| key := crypto.Keccak256Hash(bt) | ||
| env.sMu.Lock() | ||
| env.deletionProofAgg[key] = bt |
There was a problem hiding this comment.
Do we need the last proof for the same key? Not the first one?
There was a problem hiding this comment.
deletionProof is not a full proof but just the bytes of a single node (the sibling of deleted leaf node) so each item is unique. From logtrace we may collect the same node so here we use a map for deduplication.
There was a problem hiding this comment.
Bytes of a node's value? Or its key? If we delete, reinsert, then delete again, this will work?
There was a problem hiding this comment.
It is in fact bytes of 'the whole node', i.e. trie can read these bytes as a valid node and add it to the (temporary) database.
Such an entry of node in database is immutable and not affected by insert / updating or deletion. It would just lost any reference from trie when being deleted. So adding any node into database would never break the trie, it just take no use in the worst case.
|
With additional discussion. We plan to use another patch instead of current one and may be discussed in issue #253 first |
|
Now it is replaced by PR #263 |
Finding that the deletion proof can not be collected from the initialized trie state for each storage slot being touched in the executed txs. Consider following case:
Ain trie at the beginningAis deleted, as the result,Ais pushed up to a new position in trie and now it has another sibling node (call itU)Aitself is being deleted. For the witness generator side, now it need the information ofUto deduce the state root after this deletion. However, if we just collect all deletion proofs at the beginning of this tx. We would have no chance to seeUand record it.In this PR we add deletion proof collection step into log trace. For every
SSTOREset a storage slot to 0 (i.e. a deletion occur) we collect the sibling of deleted node right before this step as a deletion proof.