Stateful block to vm #1744
Stateful block to vm #1744
Conversation
aaronbuchwald
added
the
DO NOT MERGE
vm/option.go
Outdated
| func WithTxRemovedSubscriptions(subscriptions ...event.SubscriptionFactory[TxRemovedEvent]) RegisterFunc { | ||
| return func(vm *VM) { | ||
| vm.txRemovedSubscriptionFactories = append(vm.txRemovedSubscriptionFactories, subscriptions...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can split this out into a separate PR. Given the limited cases where we can send a notification (only if auth fails, which will only happen for an incorrect/malicious client), clients cannot rely on it for any useful information so we may as well remove it entirely.
vm/vm.go
Outdated
| vm.chain, err = chain.NewChain(vm, vm.config.ExecutionConfig, vm) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Will make this pluggable so that we pass the chain implementation into the VM and the VM is not tied to the *chain.XXX block types
vm/vm.go
Outdated
| genesisExecutionBlk, err := chain.NewGenesisBlock(root) | ||
| if err != nil { | ||
| snowCtx.Log.Error("could not create genesis block", zap.Error(err)) | ||
| return err | ||
| } | ||
| genesisBlk, err := ParseStatefulBlock( | ||
| ctx, | ||
| chain.NewGenesisBlock(root), | ||
| nil, | ||
| genesisExecutionBlk, | ||
| true, | ||
| vm, | ||
| ) | ||
| if err != nil { | ||
| snowCtx.Log.Error("unable to init genesis block", zap.Error(err)) | ||
| return err | ||
| } | ||
| // Set executed block, since we will never execute the genesis block | ||
| genesisBlk.executedBlock = &chain.ExecutedBlock{ | ||
| BlockID: genesisBlk.ID(), | ||
| Block: genesisExecutionBlk.StatelessBlock, | ||
| } |
There was a problem hiding this comment.
TODO: clean up init genesis
| txRemovedSubscription := event.SubscriptionFuncFactory[vm.TxRemovedEvent]{ | ||
| AcceptF: func(event vm.TxRemovedEvent) error { | ||
| return server.RemoveTx(event.TxID, event.Err) | ||
| }, | ||
| } |
There was a problem hiding this comment.
This can be factored out to a separate PR if needed. Removed this as part of this change so that PreExecute can move into the chain/ package and would prefer not to force that function to differentiate between an error that can notify the caller the tx will never go through vs. may go through in the future. This is not currently providing any new information to the caller since it only sends this notification for an invalid signature, which the caller could always check itself.
This reverts commit f70b383.
aaronbuchwald
removed
the
DO NOT MERGE
chain/chain.go
Outdated
|
|
||
| func NewChain( | ||
| tracer trace.Tracer, | ||
| gatherer metrics.MultiGatherer, |
There was a problem hiding this comment.
Why are we using MultiGatherer? I previously thought that MultiGatherer was just exposed so you could register metrics into avalanchego, otherwise it seems like the prometheus.Registerer interface is just simpler to use. Wondering if we should pass in a prometheus registry here and move the setup code upstream into the VM.
There was a problem hiding this comment.
Added a note in #1765 to remove the special case AvalancheGo metric types throughout the rest of the repo
chain/metrics.go
Outdated
| r := prometheus.NewRegistry() | ||
|
|
||
| rootCalculated, err := metric.NewAverager( | ||
| "chain_root_calculated", |
There was a problem hiding this comment.
This avalanchego wrapper seems to have a subtle difference where the underlying metric does not have a defined namespace (whereas those created through the prometheus package in the rest of this function do). This might cause issues in datadog queries since these won't map to the same namespace.
There was a problem hiding this comment.
Created a separate issue since this PR is moving existing metrics #1765
There was a problem hiding this comment.
Removed from this PR, but leaving the issue to clean up everywhere else it's used
| block.sigJob = sigJob | ||
|
|
||
| // Setup signature verification job | ||
| _, sigVerifySpan := p.tracer.Start(ctx, "Chain.AsyncVerify.verifySignatures") //nolint:spancheck |
There was a problem hiding this comment.
I think we're doing this nolint override beacause this is async... would it make more sense for us to just have an AuthBatchVerifier that wraps the action-dev defined-one and adds tracing?
There was a problem hiding this comment.
Ya that's a good idea. Separate PR?
There was a problem hiding this comment.
Made an issue: #1766. I saw we're ignoring this linter on main already as well...
This PR moves
StatefulBlockfrom the chain package to the VM package.The
StatefulBlocktype handles the state sync invariants, which is implemented by the syncervm client/server in the VM package, so this should be moved so that the VM package is solely responsible for state sync.The
StatefulBlocktype also handles determining whether a block has been processed, when a block should be executed in verify vs deferred to handled on accept, etc.This PR separates all of that logic from the chain package, which is now solely responsible for handling the state transition function of a HyperVM (VM using actions/auth). This requires that it can:
The end goal for this refactor is to decouple the VM package as well from knowing the block/result/state types required by the chain package (#1278).