Conversation
| /// - Append `[[domain = 3, slot_idx, num_changed_entries, 0], 0, 0, 0, 0]`. | ||
| /// - For partial state deltas, the map header must only be included if | ||
| /// `num_changed_entries` is not zero. | ||
| /// - For full state deltas, the map header must always be included. |
There was a problem hiding this comment.
I can't remember whether full state deltas ALWAYS implies the account is new? Or can we have full state delta for an existing account, too?
I'm bringing this up because there's a small inconsistency in the naming for this fix vs. for #2075.
Here, we make special adjustments "for full state deltas" (always include the map header, analogous to always including the value entry even if empty).
In #2075, we make special adjustments "for new accounts".
So if full state deltas =ALWAYS= new account, then maybe we could refer to new account vs. existing accounts here, too?
lmk if this makes sense or whether I missed the point completely :D
There was a problem hiding this comment.
Yes, full state delta always means new account - so, we should have probably used "new account" here as well. But I also think it is fine as is because we'll rework it relatively soon with #2078.
There was a problem hiding this comment.
I agree the terminology is inconsistent here, mostly a result of doing these PRs as "quick fixes" and in sequence instead of as one unified feature.
So if full state deltas =ALWAYS= new account, then maybe we could refer to new account vs. existing accounts here, too?
I added your comments to #2078 (comment), thanks for raising it! We could refer to new/existing accounts, but as mentioned in the issue comment, full state and partial state is more well-defined in the delta context than new/existing account, so this seems a bit better. Also open to a rename of the full / partial state notion, but can't think of anything better immediately.
| pub fn insert_empty_map_delta(&mut self, slot_index: u8) { | ||
| self.maps.entry(slot_index).or_default(); | ||
| } |
There was a problem hiding this comment.
Does the logic of this method align with its name?
If we want to insert an empty map, should we always use default(), rather than first trying to access by slot index?
3 participants
Fixes the representation of empty map slots in full state deltas (i.e. deltas representing new accounts).
This is done by:
StorageMapDeltainto theAccountStorageDeltato represent the presence of an empty map.num_changed_entries == 0Despite what the docs on
AccountDelta::to_commitmentsaid previously, I believe an entry withnum_changed_entries == 0should be fine. I updated the docs, primarily because I wanted to convince myself this was ok.This should work overall, but makes the
AccountDeltamore subtle to use, which is not great. Meaning, there is now an implicit expectation that anAccountDeltais correctly constructed, e.g. a full state delta should be constructed with an emptyStorageMapDeltato represent the presence of an empty map and it will be included in the commitment if it is present, not otherwise. In other words, constructing an account delta manually would currently likely result in some edge case being incorrect. So as long as the delta is taken fromExecutedTransactionorProvenTransaction, this should work out, but otherwise the delta has increased its complexity more than I had hoped for.I think we should revisit this fix more broadly when we implement general support for inserting or removing storage slots to an account, e.g. "account storage upgrade". The problem here really is representing a slot that is being newly inserted, while the delta was previously designed for representing only changes to existing slots, and the fix here is to make use of this existing design in subtle ways, which is not the best.
closes #2076