-
Notifications
You must be signed in to change notification settings - Fork 887
mempool/evmonlyapp: read nonce/balance through the pending overlay; fetch first-seen accounts outside the store lock #4271
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,7 +92,7 @@ func (e *Executor) executePreparedBlockWithStore(ctx context.Context, req Prepar | |
| snapshot: snapshot, | ||
| missingState: e.missingState, | ||
| } | ||
| source = newPendingOverlay(source, pending) | ||
| source = pending.overlay(source) | ||
|
|
||
| e.blockPhases.SetPhase("execute") | ||
| result, err := e.executePreparedBlock(ctx, req, source) | ||
|
|
@@ -201,12 +201,79 @@ func (e *Executor) AwaitCommits() error { | |
|
|
||
| // pipelinePending returns the changes of a block whose commit has not been waited on yet, or nil | ||
| // when the store is caught up. | ||
| func (e *Executor) pipelinePending() *StateChangeSet { | ||
| func (e *Executor) pipelinePending() *pendingChanges { | ||
| e.pipelineMu.Lock() | ||
| defer e.pipelineMu.Unlock() | ||
| return e.pipelineChanges | ||
| } | ||
|
|
||
| // LatestAccount is the balance and nonce of an account after the last block this executor ran. | ||
| type LatestAccount struct { | ||
| Balance *big.Int | ||
| Nonce uint64 | ||
| } | ||
|
|
||
| // ReadLatestAccount returns addr's balance and nonce after the last block this executor ran, | ||
| // without waiting for that block's commit to land. It reports the first failed commit instead of | ||
| // state that lacks the failed block. | ||
| func (e *Executor) ReadLatestAccount(addr common.Address) (LatestAccount, error) { | ||
| if e.stateStore == nil { | ||
| return LatestAccount{}, errMissingStateStore | ||
| } | ||
| for { | ||
| e.pipelineMu.Lock() | ||
| pending, generation, failure := e.pipelineChanges, e.pipelineGeneration, e.pipelineFailureLocked() | ||
| e.pipelineMu.Unlock() | ||
| if failure != nil { | ||
| return LatestAccount{}, failure | ||
| } | ||
| snapshot := e.stateStore.OpenView() | ||
| if snapshot == nil { | ||
| return LatestAccount{}, errors.New("giga store returned a nil snapshot") | ||
| } | ||
| account, ok := e.readLatestAccount(snapshot, pending, generation, addr) | ||
| snapshot.Close() | ||
| if ok { | ||
| return account, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // readLatestAccount reads addr through pending laid over snapshot. It reports false when another | ||
| // commit started after generation was read, since the view may then hold a later block's writes and | ||
| // pending would replay older values over them; the caller reads again. | ||
| func (e *Executor) readLatestAccount(snapshot gigatypes.EVMStateView, pending *pendingChanges, generation uint64, addr common.Address) (LatestAccount, bool) { | ||
| e.pipelineMu.Lock() | ||
| moved := e.pipelineGeneration != generation | ||
| e.pipelineMu.Unlock() | ||
| if moved { | ||
| return LatestAccount{}, false | ||
| } | ||
| reader := pending.overlay(gigaSnapshotStateReader{snapshot: snapshot, missingState: e.missingState}) | ||
| if rowReader, ok := reader.(accountSnapshotReader); ok { | ||
| if row, ok := rowReader.ReadAccount(addr); ok { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] For EOAs — the mempool hot path this PR targets — that costs nothing. But Since only balance and nonce are wanted here, reading
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deliberately not changed here: this stack re-opens the already-merged code byte-for-byte so the team can review what is actually running on giga-1 (the top of the stack equals current giga-1). Agreed this is a real improvement; tracking it as a follow-up to land on top once the stack has been reviewed, unless the reviewers prefer it folded in. |
||
| balance := row.Balance | ||
| if balance == nil { | ||
| balance = new(big.Int) | ||
| } | ||
| return LatestAccount{Balance: balance, Nonce: row.Nonce}, true | ||
| } | ||
| } | ||
| return LatestAccount{Balance: reader.GetBalance(addr), Nonce: reader.GetNonce(addr)}, true | ||
| } | ||
|
|
||
| // pipelineFailureLocked returns the first failed commit, whether or not a waiter has retired it yet. | ||
| // Callers hold pipelineMu. | ||
| func (e *Executor) pipelineFailureLocked() error { | ||
| if e.pipelineFailure != nil { | ||
| return e.pipelineFailure | ||
| } | ||
| if e.pipelineErr != nil { | ||
| return fmt.Errorf("commit state changes: %w", e.pipelineErr) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // awaitPipelineCommit blocks until the in-flight commit has landed, reporting the first commit that | ||
| // failed. After it returns the store holds every block this executor has run, so the next view | ||
| // opens on a known height and needs no overlay. | ||
|
|
@@ -248,14 +315,15 @@ func (e *Executor) awaitPipelineCommit() error { | |
| // Commits stay ordered because only one is ever in flight: awaitPipelineCommit lands the previous | ||
| // one before this is called. | ||
| func (e *Executor) startPipelineCommit(blockNumber int64, changesets []*proto.NamedChangeSet, changes *StateChangeSet) error { | ||
| pending := changes.clone() | ||
| pending := newPendingChanges(changes.clone()) | ||
| done := make(chan struct{}) | ||
| e.pipelineMu.Lock() | ||
| if failure := e.pipelineFailure; failure != nil { | ||
| e.pipelineMu.Unlock() | ||
| return failure | ||
| } | ||
| e.pipelineChanges = pending | ||
| e.pipelineGeneration++ | ||
| e.pipelineDone = done | ||
| e.pipelineErr = nil | ||
| e.pipelineMu.Unlock() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -470,20 +470,34 @@ func (a *evmOnlyApplication) callBlockContext() (evmonly.BlockContext, error) { | |
| panic("unreachable") | ||
| } | ||
|
|
||
| func (a *evmOnlyApplication) EvmNonce(address common.Address) uint64 { | ||
| // latestAccount returns address's balance and nonce after the last finalized block, read through | ||
| // the executor's in-flight commit rather than waiting for it. Before InitChain, or once a commit | ||
| // has failed, it reads the settled store instead. | ||
| func (a *evmOnlyApplication) latestAccount(address common.Address) evmonly.LatestAccount { | ||
| if executor, ok := a.settler.Load().Get(); ok { | ||
| if account, err := executor.ReadLatestAccount(address); err == nil { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] The error from The fallback is
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deliberately not changed here: this stack re-opens the already-merged code byte-for-byte so the team can review what is actually running on giga-1 (the top of the stack equals current giga-1). Agreed this is a real improvement; tracking it as a follow-up to land on top once the stack has been reviewed, unless the reviewers prefer it folded in. |
||
| return account | ||
| } | ||
| } | ||
| snapshot := a.openSettledView() | ||
| defer snapshot.Close() | ||
| return snapshot.GetNonce(evmOnlyStoreAddress(address)) | ||
| storeAddress := evmOnlyStoreAddress(address) | ||
| if !snapshot.AccountExists(storeAddress) { | ||
| return evmonly.LatestAccount{Balance: new(big.Int).Set(evmOnlyBaseBalance)} | ||
| } | ||
| balance := snapshot.GetBalance(storeAddress) | ||
| return evmonly.LatestAccount{ | ||
| Balance: new(big.Int).SetBytes(balance[:]), | ||
| Nonce: snapshot.GetNonce(storeAddress), | ||
| } | ||
| } | ||
|
|
||
| func (a *evmOnlyApplication) EvmNonce(address common.Address) uint64 { | ||
| return a.latestAccount(address).Nonce | ||
| } | ||
|
|
||
| func (a *evmOnlyApplication) EvmBalance(address common.Address, _ []byte) uint256.Int { | ||
| snapshot := a.openSettledView() | ||
| defer snapshot.Close() | ||
| if !snapshot.AccountExists(evmOnlyStoreAddress(address)) { | ||
| return *uint256.MustFromBig(evmOnlyBaseBalance) | ||
| } | ||
| balance := snapshot.GetBalance(evmOnlyStoreAddress(address)) | ||
| return *new(uint256.Int).SetBytes(balance[:]) | ||
| return *uint256.MustFromBig(a.latestAccount(address).Balance) | ||
| } | ||
|
|
||
| func (a *evmOnlyApplication) EvmChainID() uint64 { | ||
|
|
||
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.
is it possible that N's changes haven't finished committing when N+2 has begun execution? In that case missingState would carry changes from N+1 but not from N unless missingState itself is stacked
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.
No — at most one block is ever uncommitted.
executePreparedBlockWithStorecallsawaitPipelineCommit()(lands N) right beforestartPipelineCommitfor N+1, and it holdsstoreMufor the whole block, so N+2 cannot begin executing until N+1 has returned, i.e. until N is in the store. That is the invariant thependingChangesoverlay relies on: the view holds ≤ N,pendingis exactly N+1, nothing in between can be missing. ThepipelineGenerationrecheck inreadLatestAccountcovers the one race that remains — a commit for N+2 starting between the read ofpendingand the view being opened — by retrying rather than replaying N+1 over a view that already contains N+2.missingStateis a different thing: it is not a per-block layer but the fallbackStateReaderfor accounts the store has never seen (WithMissingAccountState, used for genesis-less funding in tests/loadtest), consulted only whensnapshot.AccountExists(addr)is false. It never carries block changes, so there is nothing to stack — the overlay sits above it and above the snapshot alike.