[Storehouse] Add CreateStorageSnapshot to Execution state #5031
Conversation
zhangchiqing
changed the base branch from
master
to
leo/storehouse-committer
| @@ -545,7 +545,7 @@ func (h *handler) GetAccountAtBlockID( | |||
|
|
|||
| value, err := h.engine.GetAccount(ctx, flowAddress, blockFlowID) | |||
| if err != nil { | |||
| if errors.Is(err, scripts.ErrStateCommitmentPruned) { | |||
| if errors.Is(err, state.ErrStateCommitmentPruned) { | |||
| return nil, status.Errorf(codes.OutOfRange, "state for block ID %s not available", blockFlowID) | |||
| } | |||
| if errors.Is(err, storage.ErrNotFound) { | |||
There was a problem hiding this comment.
I think this is the case that block is not found, rather than account not found, might need to update error message
engine/execution/rpc/engine.go
Outdated
| @@ -545,7 +545,7 @@ func (h *handler) GetAccountAtBlockID( | |||
|
|
|||
| value, err := h.engine.GetAccount(ctx, flowAddress, blockFlowID) | |||
| if err != nil { | |||
| if errors.Is(err, scripts.ErrStateCommitmentPruned) { | |||
| if errors.Is(err, state.ErrStateCommitmentPruned) { | |||
There was a problem hiding this comment.
I wonder if we should distinguish between block not executed error, so that we can remove L539, since GetAccount is already checking that.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5031 +/- ##
==========================================
- Coverage 56.24% 56.23% -0.01%
==========================================
Files 975 974 -1
Lines 90864 90802 -62
==========================================
- Hits 51110 51067 -43
+ Misses 35944 35925 -19
Partials 3810 3810
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-committer
branch
2 times, most recently
from
d94c72e
to
6e9e5df
Compare
zhangchiqing
force-pushed
the
leo/storehouse-execution-state
branch
from
85fa45b
to
01833b2
Compare
zhangchiqing
requested review from
koko1123,
janezpodhostnik,
sideninja and
peterargue
zhangchiqing
changed the title
| NewStorageSnapshot(commit flow.StateCommitment, blockID flow.Identifier, height uint64) snapshot.StorageSnapshot | ||
|
|
||
| // CreateStorageSnapshot creates a new ready-only view at the given block. | ||
| // It returns: | ||
| // - (nil, nil, storage.ErrNotFound) if block is unknown | ||
| // - (nil, nil, state.ErrNotExecuted) if block is not executed | ||
| // - (nil, nil, state.ErrExecutionStatePruned) if the execution state has been pruned | ||
| CreateStorageSnapshot(blockID flow.Identifier) (snapshot.StorageSnapshot, *flow.Header, error) |
There was a problem hiding this comment.
Do we still need the first method?
I wonder (not sure the impact of the change on the current usage of the method) if we could have one method that you could call with options, something like:
CreateStorageSnapshot(WithBlockID(id))
CreateStorageSnapshot(WithCommitment(commitment), WithBlockID(id), WithHeight(height))
You could then internally decide how to handle that. I don't have much context on the usage tho, so feel free to disregard
There was a problem hiding this comment.
There are a few different scenarios that still need the NewStorageSnapshot function. Let me explain why we need these two methods:
-
There are two storage engines, ledger-based storage (current) and storehouse-based storage (upgrading to). Ledger-based storage takes commitment to find register, whereas storehouse takes BlockID and Height. We would like to use a flag to control which storage engine to use in the execution state, and we don't want the caller to know this implementation detail. That's why we pass all three data to the NewStorageSnapshot method, so that the execution state can decide internally which data to use. If we make them as options like what you suggested with
WithCommitment/WithBlockID, then it means the caller still has to know the implementation detail to decide which option to use. -
Usually, we will lookup block first before querying the register, that's why I made this function
CreateStorageSnapshotthat takes a blockID, and lookup the commitment or block height and then create the storage snapshot. -
But then, why do we still need the NewStorageSnapshot, if we always need lookup commitment or block height ? Because there are one scenario where we don't need to lookup commitment or block height. That's when we are executing a block, the commitment and the block height is already available, see this change in ingestion engine. Using CreateStorageSnap there would make a unnecessary call.
zhangchiqing
force-pushed
the
leo/storehouse-execution-state
branch
from
01833b2
to
a5d8fcc
Compare
Working towards #4998
This PR:
NewStorageSnapshotto take blockID and height it's compatible with storehouse.CreateStorageSnapshotto ScriptExecutionState in order to handle looking up state commitment by block id, and then lookup ledger storage by state commitment. The CreateStorageSnapshot could allow us to have different implementation with storehouse storage.