Replace toEngine channel with a dedicated subscription API#3999
Merged
StephenButtolph merged 42 commits intoava-labs:masterfrom Jul 7, 2025
Merged
Replace toEngine channel with a dedicated subscription API#3999StephenButtolph merged 42 commits intoava-labs:masterfrom
StephenButtolph merged 42 commits intoava-labs:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
Replaces the toEngine channel pattern with a new SubscribeToEvents API across VMs and engine components, enabling on-demand VM event subscriptions for consensus.
- Remove
toEngineparameters from VM interfaces, chain creation, mocks, and tests - Introduce
SubscribeToEventson the VM interface and implementNotificationForwarderto bridge from VM to engine - Update gRPC messenger service to use bidirectional streaming
Notify(stream Event) returns (stream EventRequest)
Reviewed Changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vms/avm/block/builder/builder_test.go | Updated mempool.New calls to use callback instead of channel |
| snow/networking/{sender,router,handler}/*.go | Removed obsolete msgFromVMChan arguments and dispatch branches |
| snow/engine/snowman/{syncer,state_syncer.go} | Added NotificationForwarder field on state syncer |
| snow/engine/snowman/engine/engine.go | Added NotificationForwarder in engine setup |
| snow/engine/snowman/bootstrap/bootstrapper.go | Added NotificationForwarder in bootstrapper setup |
| snow/engine/snowman/block/blockmock/chain_vm.go | Regenerated mocks to drop toEngine args and include new method |
| snow/engine/enginetest/vm.go | Removed toEngine from test VM initializer |
| snow/engine/common/{vm.go,subscription*,notifier.go} | Dropped toEngine, added SubscribeToEvents and notification logic |
| snow/engine/avalanche/vertex/vm.go | Removed toEngine from vertex VM interface |
| proto/messenger/{messenger.proto,*.pb.go} | Changed Notify RPC to streaming events and updated generated |
| chains/{manager.go,linearizable_vm.go} | Removed toEngine wiring in chain manager and linearizable VM |
| go.mod | Bumped coreth dependency |
Comments suppressed due to low confidence (3)
snow/engine/snowman/engine/engine.go:156
- After constructing the NotificationForwarder in the Engine (
engine.nf), invokeengine.nf.Start()before returning so it begins forwarding VM events.
engine.nf = &common.NotificationForwarder{
snow/engine/snowman/engine/engine.go:441
- Ensure
e.nf.Close()is called inShutdownto stop the notification forwarder and clean up its goroutine.
func (e *Engine) Shutdown(ctx context.Context) error {
snow/engine/common/subscription_test.go:48
- [nitpick] The parameter name
uint65is ambiguous; consider renaming it topChainHeightto match the interface and improve clarity.
func subscription(ctx context.Context, uint65 uint64) (Message, uint64) {
yacovm
force-pushed
the
subscribeToEvents
branch
10 times, most recently
from
d09f8a3 to
2614052
Compare
yacovm
force-pushed
the
subscribeToEvents
branch
15 times, most recently
from
38b2f79 to
1097d82
Compare
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
And initialize notifier earlier Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
StephenButtolph
approved these changes
Jul 7, 2025
samliok
approved these changes
Jul 7, 2025
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
StephenButtolph
approved these changes
Jul 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this should be merged
The API change in this PR is needed to support consensus protocols which facilitate censorship resistance as part of their protocol and therefore need information about whether a block should be built or not on demand, in contrast to the current
toEnginepattern which notifies the consensus when new transactions arrive only once and is asynchronous to the operation of the consensus protocol. Having a dedicated API allows consensus to obtain information about whether a block should be built on demand and therefore correlated to a specific round, while using the currenttoEnginepattern makes it impossible to correlate with a specific round, and this makes it impossible to incorporate with consensus.How this works
This PR removes the toEngine channel pattern from all VMs and adds an additional API:
Which returns when an event (pending transactions or state sync done Message) is sent by the VM.
Unlike the toEngine pattern which only notifies the consumer a single time until a block is built, successive calls to
WaitForEventwill return that transactions are pending as long as the mem-pool is not empty and a block can be built, otherwise the calls block.How this was tested
Added unit tests, and CI passes.
Need to be documented in RELEASES.md?