[Storehouse] Update committer #5029
Conversation
zhangchiqing
changed the base branch from
master
to
leo/storehouse-extending-block-snapshot
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5029 +/- ##
==========================================
- Coverage 56.20% 56.20% -0.01%
==========================================
Files 975 975
Lines 90714 90754 +40
==========================================
+ Hits 50990 51006 +16
- Misses 35913 35937 +24
Partials 3811 3811
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
zhangchiqing
force-pushed
the
leo/storehouse-extending-block-snapshot
branch
from
27715b2
to
904bed0
Compare
zhangchiqing
force-pushed
the
leo/storehouse-committer
branch
from
556acad
to
d94c72e
Compare
zhangchiqing
force-pushed
the
leo/storehouse-committer
branch
from
d94c72e
to
6e9e5df
Compare
| require.Equal(t, []uint8(expectedProof), proof) | ||
| require.True(t, expectedTrieUpdate.Equals(trieUpdate)) | ||
|
|
||
| // TOOD(leo): verify ledger.Set and ledger.Prove are called and received expected arguments |
There was a problem hiding this comment.
You can put a function inside the mock like so:
l.On("Set", mock.Anything).
Return(func(/*args with types*/) /*return vals*/ {
// assert whatever you want
return ledger.State(endState), expectedTrieUpdate, nil
}).
Once()
zhangchiqing
requested review from
ramtinms and
janezpodhostnik
as code owners
zhangchiqing
requested review from
koko1123,
sideninja and
peterargue
and removed request for
ramtinms
| @@ -58,6 +58,10 @@ func (s *ExecutingBlockSnapshot) getFromUpdates(id flow.RegisterID) (flow.Regist | |||
| // Usually it's used to create a new storage snapshot at the next executed collection. | |||
| // The registerUpdates contains the register updates at the executed collection. | |||
| func (s *ExecutingBlockSnapshot) Extend(newCommit flow.StateCommitment, updates map[flow.RegisterID]flow.RegisterValue) execution.ExtendableStorageSnapshot { | |||
| if len(updates) == 0 { | |||
| return s | |||
There was a problem hiding this comment.
whats the reasoning here? I'm not sure I understand.
There was a problem hiding this comment.
I will add comments, the idea is that if there is. no updates, it is not necessary to wrap with a snapshot that has no update, instead, just return the original snapshot itself, since it is able to return the same register.
| UpdatedRegisters() flow.RegisterEntries | ||
| UpdatedRegisterSet() map[flow.RegisterID]flow.RegisterValue |
There was a problem hiding this comment.
The function below uses both of these. I think that is unnecessary. We could only have UpdatedRegisterSet() map[flow.RegisterID]flow.RegisterValue (which means we could just call it UpdatedRegisters() map[flow.RegisterID]flow.RegisterValue)
There was a problem hiding this comment.
Yeah, however it requires the caller to convert into a slice. And we probably don't want the caller to decide the order, the order of updates should be deterministic, that's UpdatedRegisters sort the slice internally before returning.
What about returning both map and slice, since both of them are used below?
It's possible, but I'm just afraid it's a bit confusing, returning the same data in two forms in one API. That's why I created a separate API for returning different form of the same data.
Thoughts?
There was a problem hiding this comment.
RegisterUpdatesHolder is only used here and the only use of UpdatedRegisters is then converted into a map anyway, so order is not conserved.
I'm just not sure the function below (CommitDelta) needs to use both these interface methods.
There was a problem hiding this comment.
No, the order is actually conserved by keys, values := RegisterEntriesToKeysValues(updatedRegisters)
The keys and values are ordered, which then be used to create trieUpdate by
newState, trieUpdate, err := ldg.Set(update)
The trieUpdate contains the ordered list of paths and payloads, the order has to be deterministic, otherwise it will produce different execution data id in the execution result.
Working towards #4998
This PR updates committer to use snapshot instead of commitment when commit delta