-
Notifications
You must be signed in to change notification settings - Fork 762
Replace toEngine channel with a dedicated subscription API #3999
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
base: master
Are you sure you want to change the base?
Conversation
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.
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
toEngine
parameters from VM interfaces, chain creation, mocks, and tests - Introduce
SubscribeToEvents
on the VM interface and implementNotificationForwarder
to 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 inShutdown
to 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
uint65
is ambiguous; consider renaming it topChainHeight
to match the interface and improve clarity.
func subscription(ctx context.Context, uint65 uint64) (Message, uint64) {
d09f8a3
to
2614052
Compare
38b2f79
to
1097d82
Compare
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.
I was asked to review this as a codeowner of ./scripts, but the only script that appears is empty so I can't even comment directly on it.
my bad I accidentally added it to git. removed. |
…s an additional API: ``` SubscribeToEvents(ctx context.Context) Message) ``` 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 `SubscribeToEvents` will return that transactions are pending as long as the mem-pool is not empty and a block can be built, otherwise the calls block. The API change 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 curren toEngine pattern which notifies the consensus when new transactions arrive only once, and then block building may even fail. Having such an API on the VM side (and not implementing the logic in consensus) forces the VM to handle any side effects of block building (such as re-introducing transactions into the mempool). 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]>
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
toEngine
pattern 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 currenttoEngine
pattern 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
WaitForEvent
will 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?