-
Notifications
You must be signed in to change notification settings - Fork 671
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
Unify engines #3405
base: master
Are you sure you want to change the base?
Unify engines #3405
Conversation
788480d
to
9d6c351
Compare
92df2c8
to
fd73ebb
Compare
The lifecycle of an avalanchego node can be separated to three main stages: Stage I - If supported by the VM, then a state sync occurs. Otherwise, this stage is skipped. In the X-chain, this stage is entirely substituted by a custom bootstrapping phase that replicates and executes the X-chain DAG. Stage II - A stage called bootstrapping, which entails replicating the blocks from other avalanchego nodes, and executing them. Stage III - The final form of an avalanchego node, in which it runs the snowman consensus protocol. The phases are implemented by components called "engines": - avalanche bootstrapping - snowman - state syncer - snowman bootstrapper And the handler in snow/networking/handler/ is responsible to route messages from the network into the correct engine. Engines all implement the same common.Engine interface, but the interface consists of the union of all operations across all engines. Indeed, it is often the case that a message of type `m` dispatched by engine `e`, cannot be processed by a different engine `e'. For instance, a Chits message cannot be processed by any engine other than consensus, and a message about a query of state summary is only relevant to the state sync engine. To that end, each engine simply implements a no-op dispatcher for messages it does not care about. The biggest problem with the existing aforementioned structure, is that the lifecycle of the engines imposes a strict one way movement across the various stages, and there is no single component which consolidates the transition among the stages. The movement between the various stages takes place by a callback given to each engine at every stage but the final one (Stage I and Stage II). The structure therefore makes it difficult to introduce movement from snowman consensus back to bootstrapping / state sync, or to have better control over the message dispatch. This commit unifies all engines into a single one under snow/engine/unified As a result, the implementation of the handler in snow/networking/handler/handler.go is now simpler, as it only interacts with the unified engine, and it never needs to query the snow.EngineState. The state transition between the various stages is now taken care of by the unified engine, and since the code to dispatch messages to the right engine is now all in the unified engine, it's not only more testable but it lets us move among the stages in the same place where we consider the stage we're in to dispatch the message to the correct engine. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
This doesn't sound like state syncing. Why is this performed in Stage I instead of Stage II? |
Bootstrapper: bootstrapper, | ||
Consensus: engine, | ||
}, | ||
ctx.State.Set(snow.EngineState{ |
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 action required) Maybe determine the appropriate state (snow.StateSyncing
or snow.Bootstrapping
) and then perform Set
?
"context" | ||
|
||
"github.com/ava-labs/avalanchego/api/health" | ||
) | ||
|
||
type BootstrapableEngine interface { |
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 action required) What differentiates a BootstrapableEngine
from an AvalancheBootstrapableEngine
? Maybe use a docstring to document?
Engine | ||
AcceptedFrontierHandler | ||
AcceptedHandler | ||
AncestorsHandler |
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 action required) Would it make sense to embed the AvalancheBootstrapableEngine
interface since only the Accepted*
embeddings are unique to this interface?
"github.com/ava-labs/avalanchego/trace" | ||
) | ||
|
||
type Enabler interface { |
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 action required) Enabler
implies that it enables something, but the only method would appear to be a getter. Maybe EnabledChecker
?
|
||
type OnFinishedFunc func(ctx context.Context, lastReqID uint32) error | ||
|
||
type Factory interface { |
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 action required) Maybe move this to factory.go?
ctx.State.Set(testCase.state) | ||
|
||
var vm enginetest.VM | ||
configureVMRouting(&vm) |
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 this need to be called if routing isn't being checked?
configureVMRouting(&vm) | ||
|
||
engine, err := unified.EngineFromEngines(ctx, ef, &vm) | ||
require.NoError(t, err) |
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 action required) Maybe use require = require.New(t)
for a given scope rather than passing t
to every assertion?
return invokedTable | ||
} | ||
|
||
func createEngineFactory(t *testing.T, gs enginetest.Engine, as enginetest.Engine, sm enginetest.Engine, stateSyncer mockStateSyncer, avalancheSyncer mockStateSyncer, bootstrapper mockStateSyncer) *mock.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.
(No action required) Maybe refactor the code under test to enable validation without the use of a mock?
} | ||
|
||
func createEngineInvocationMap(t *testing.T) map[string]func(*unified.Engine) { | ||
m := make(map[string]func(*unified.Engine)) |
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 action required) Maybe use map[string]func(*unified.Engine) error
instead? That would avoid a dependency on t
(the error check could be performed by the test) and allow the map to be defined once rather than per sub-test.
smbootstrap "github.com/ava-labs/avalanchego/snow/engine/snowman/bootstrap" | ||
) | ||
|
||
func TestFactory(t *testing.T) { |
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 action required) Maybe skip checking trivial getters, and focus on specific methods whose correctness is difficult to reason about by observation?
Why this should be merged
The lifecycle of an avalanchego node can be separated to three main stages:
Stage I - If supported by the VM, then a state sync occurs. Otherwise, this stage is skipped.
In the X-chain, this stage is entirely substituted by a custom bootstrapping phase that replicates and executes the X-chain DAG.
Stage II - A stage called bootstrapping, which entails replicating the blocks from other avalanchego nodes, and executing them.
Stage III - The final form of an avalanchego node, in which it runs the snowman consensus protocol.
The phases are implemented by components called "engines":
And the handler in snow/networking/handler/ is responsible to route messages from the network into the correct engine.
Engines all implement the same common.Engine interface, but the interface consists of the union of all operations across all engines.
Indeed, it is often the case that a message of type m dispatched by engine e, cannot be processed by a different engine e'.
For instance, a Chits message cannot be processed by any engine other than consensus, and a message about a query of state summary is only relevant to the state sync engine.
To that end, each engine simply implements a no-op dispatcher for messages it does not care about.
The biggest problem with the existing aforementioned structure, is that the lifecycle of the engines imposes a strict one way movement across the various stages,
and there is no single component which consolidates the transition among the stages. The movement between the various stages takes place by a callback given to each
engine at every stage but the final one (Stage I and Stage II).
The structure therefore makes it difficult to introduce movement from snowman consensus back to bootstrapping / state sync, or to have better control over the message dispatch.
This commit unifies all engines into a single one under snow/engine/unified
As a result, the implementation of the handler in snow/networking/handler/handler.go is now simpler,
as it only interacts with the unified engine, and it never needs to query the snow.EngineState.
The state transition between the various stages is now taken care of by the unified engine, and since the code to dispatch messages to the right engine
is now all in the unified engine, it's not only more testable but it lets us move among the stages in the same place where we consider the stage we're in to dispatch
the message to the correct engine.
How this works
Just a refactoring: I shrinked the interfaces of the engines to what is absolutely required and now only the unified engine implements the common.Engine. Removed un-necessary code from the handler package and moved the logic into the unified engine.
How this was tested
CI.