-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[factory] simplify interface for archive mode #4474
Conversation
fbadbad
to
b365980
Compare
action/protocol/managers.go
Outdated
ArchiveStateSimulator interface { | ||
StateReader | ||
SimulateExecution(context.Context, address.Address, action.Envelope, ...SimulateOption) ([]byte, *action.Receipt, error) | ||
ReadContractStorage(context.Context, address.Address, []byte) ([]byte, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 are needed for archive-node API
} | ||
return state.NewIterator(keys, values) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
included in WorkingSetAtHeight
funcs
state/factory/factory_withheight.go
Outdated
sf.mutex.RLock() | ||
defer sf.mutex.RUnlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it share the same mutex with factory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think reading on archive state can be lock-free
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in latest commit
state/factory/factory_withheight.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SimulateExecution
and ReadContractStorage
also need re-implement, they are not currently based on the specified height, but rather on the latest height
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok just added, was thinking to add it later in API PR
timer := sf.timerFactory.NewTimer("Commit") | ||
sf.mutex.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when reviewing the usage of sf.mutex
, found it's not really needed here
state/factory/factory_withheight.go
Outdated
cfg, err := processOptions(opts...) | ||
if err != nil { | ||
return 0, err | ||
} | ||
if cfg.Keys != nil { | ||
return 0, errors.Wrap(ErrNotSupported, "Read state with keys option has not been implemented yet") | ||
} | ||
if sf.height > sf.currentChainHeight { | ||
return 0, errors.Errorf("query height %d is higher than tip height %d", sf.height, sf.currentChainHeight) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the check to AtHeight()
state/factory/factory_withheight.go
Outdated
return sf.height, sf.stateAtHeight(sf.height, cfg.Namespace, cfg.Key, s) | ||
} | ||
|
||
func (sf *factoryWithHeight) stateAtHeight(height uint64, ns string, key []byte, s interface{}) error { | ||
if !sf.saveHistory { | ||
return ErrNoArchiveData | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check is already there in AtHeight()
state/factory/factory_withheight.go
Outdated
defer sf.mutex.RUnlock() | ||
if sf.height > sf.currentChainHeight { | ||
return 0, nil, errors.Errorf("query height %d is higher than tip height %d", sf.height, sf.currentChainHeight) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the check to AtHeight()
@@ -87,8 +87,6 @@ type ( | |||
NewBlockBuilder(context.Context, actpool.ActPool, func(action.Envelope) (*action.SealedEnvelope, error)) (*block.Builder, error) | |||
PutBlock(context.Context, *block.Block) error | |||
DeleteTipBlock(context.Context, *block.Block) error | |||
StateAtHeight(uint64, interface{}, ...protocol.StateOption) error | |||
StatesAtHeight(uint64, ...protocol.StateOption) (state.Iterator, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 are included in WorkingSetAtHeight
funcs
if err != nil { | ||
return nil, 0, err | ||
} | ||
d, h, err := p.ReadState(ctx, historySR, methodName, arguments...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can combine codes after getting the specified StateReader
state/factory/factory.go
Outdated
rootKey = fmt.Sprintf("%s-%d", ArchiveTrieRootKey, height) | ||
createTrie = false | ||
} | ||
store, err := newFactoryWorkingSetStore(sf.protocolView, flusher, rootKey, createTrie) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitting into the following two functions seems more clear:
newFactoryWorkingSetStore(view, flusher)
, which inner use ArchiveTrieRootKey and truenewFactoryWorkingSetStoreAt(view, flusher, height)
, which inner use ArchiveTrieRootKey-height and false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it should use ArchiveTrieRootKey or ArchiveTrieRootKey-height if height equals currentChainHeight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 cases for height equals currentChainHeight
- createGenesisStates calls
newWorkingSet(ctx, 0)
, which is write at current tip - archive-mode trying to read at current tip height > 0, at the same time
PutBlock
may still be in progress soArchiveTrieRootKey-height
may not exist at the moment
both cases should use ArchiveTrieRootKey
state/factory/workingset.go
Outdated
@@ -333,7 +333,7 @@ func (ws *workingSet) Commit(ctx context.Context) error { | |||
return err | |||
} | |||
ws.Reset() | |||
return nil | |||
return ws.store.Stop(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newWorkingSet() creates a factoryWorkingSetStore
instance and calls Start()
inside, but its Stop()
is never called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After checking, workingSetStore.Finalize()
calls store.tlt.RootHash()
, which calls tlt.flush()
, which is the effective Stop(), we may need another PR to clean-up this
@@ -343,6 +343,9 @@ func (ws *workingSet) State(s interface{}, opts ...protocol.StateOption) (uint64 | |||
if err != nil { | |||
return ws.height, err | |||
} | |||
if cfg.Keys != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's in the removed stateAtHeight()
func, it's a safety check, there's a similar check in States()
at L361
Quality Gate passedIssues Measures |
Description
make the Factory interface more natural to support archive mode
WorkingSetAtHeight()
interfacehistoryfactory.go
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: