Take ExecutionEngineSocket from executor instead of cloning it + Notifier refactor #13

bidzyyys · 2024-01-11T19:09:55Z

Closing Application: Take ExecutionEngineSocket from executor instead of cloning it

daltoncoder

Good start there is a couple changes i listed and we should sync on moving away from injection into the socket like i described below

daltoncoder · 2024-01-15T18:15:58Z

core/application/src/app.rs

 use crate::query_runner::QueryRunner;
 pub struct Application<C: Collection> {
-    update_socket: ExecutionEngineSocket,
+    update_socket: Arc<Mutex<Option<ExecutionEngineSocket>>>,


Suggested change

update_socket: Arc<Mutex<Option<ExecutionEngineSocket>>>,

update_socket: Mutex<Option<ExecutionEngineSocket>>,

No need for the Arc here

daltoncoder · 2024-01-15T18:16:25Z

core/application/src/app.rs

        Ok(Self {
            query_runner: env.query_runner(),
-            update_socket: TokioSpawn::spawn_async(UpdateWorker::<C>::new(env, blockstore)),
+            update_socket: Arc::new(Mutex::new(Some(TokioSpawn::spawn_async(


Suggested change

update_socket: Arc::new(Mutex::new(Some(TokioSpawn::spawn_async(

update_socket: Mutex::new(Some(TokioSpawn::spawn_async(

daltoncoder · 2024-01-15T18:17:58Z

core/application/src/app.rs

    /// See the safety document for the [`ExecutionEngineSocket`].
-    fn transaction_executor(&self) -> ExecutionEngineSocket {
-        self.update_socket.clone()
+    fn transaction_executor(&self) -> Option<ExecutionEngineSocket> {


Suggested change

fn transaction_executor(&self) -> Option<ExecutionEngineSocket> {

fn transaction_executor(&self) -> ExecutionEngineSocket> {

Lets keep the return on the interface the same as it was. We should unwrap here in this function before returning. The safety comments on this function in the interface should reflect that this is going to panic if a process besides consensus calls this

daltoncoder · 2024-01-15T18:36:28Z

core/interfaces/src/consensus.rs

+        executor.inject(move |res| {
+            if res.change_epoch {
+                epoch_change_notifier.epoch_changed();
+            }
+        });


Part of the reasoning we want to move to this new notifier flow is because this injection we do into the Affair socket is overhead that we really dont want. So we want to pass this notifier to consensus and be notifying in submit_batch when we know the epoch has changed. Ill sync with you and give you some more color on what im describing off of github.

yea, let's get rid of this flow here and instead trigger the event from the consensus.

Sure, I see your point!

qti3e

looks good overall, but let's not return an Option as dalton already pointed out and this way any changes to the test files should be not needed anymore.

qti3e · 2024-01-15T21:27:43Z

core/interfaces/src/consensus.rs

+        executor.inject(move |res| {
+            if res.change_epoch {
+                epoch_change_notifier.epoch_changed();
+            }
+        });


yea, let's get rid of this flow here and instead trigger the event from the consensus.

qti3e · 2024-01-15T21:28:59Z

core/notifier/src/lib.rs

+#[derive(Default, Clone)]
+pub struct EpochChangeNotificationsEmitter {
+    emitter: Arc<Notify>,
+}
+
+impl EpochNotifierEmitter for EpochChangeNotificationsEmitter {
+    fn epoch_changed(&self) {
+        self.emitter.notify_waiters()
+    }
+}


following the comment above about getting rid of the injection flow with affair, we should use this event emitter instead to perform the notification.

bidzyyys · 2024-01-16T10:54:02Z

Thanks @daltoncoder & @qti3e for pointing out fields for improvement!

qti3e · 2024-01-17T16:47:17Z

core/interfaces/src/notifier.rs

 #[infusion::service]
 pub trait NotifierInterface<C: Collection>: Sync + Send + Clone {
+    type EpochEmitter: EpochNotifierEmitter;
+    type BlockEmitter: BlockNotifierEmitter;
+
    fn _init(app: ::ApplicationInterface) {
        ok!(Self::init(app))
    }

    fn init(app: &c!(C::ApplicationInterface)) -> Self;

+    /// Returns a reference to the emitter end of this notifier. Should only be used if we are
+    /// interested (and responsible) for triggering a notification around new block.
+    fn new_block_emitter(&self) -> Self::BlockEmitter;
+
+    /// Returns a reference to the emitter end of this notifier. Should only be used if we are
+    /// interested (and responsible) for triggering a notification around new epoch.
+    fn new_epoch_emitter(&self) -> Self::EpochEmitter;
+
+    fn notify_on_new_block(&self, tx: mpsc::Sender<Notification>);
+
    fn notify_on_new_epoch(&self, tx: mpsc::Sender<Notification>);

    fn notify_before_epoch_change(&self, duration: Duration, tx: mpsc::Sender<Notification>);
 }
+
+#[infusion::blank]
+pub trait EpochNotifierEmitter: Clone + Send + Sync + 'static {
+    /// Notify the waiters about epoch change.
+    fn epoch_changed(&self);
+}
+
+#[infusion::blank]
+pub trait BlockNotifierEmitter: Clone + Send + Sync + 'static {
+    /// Notify the waiters about new block.
+    fn new_block(&self);
+}


hmm we should keep the emitters in the same trait and group them together:

Suggested change

#[infusion::service]

pub trait NotifierInterface<C: Collection>: Sync + Send + Clone {

type EpochEmitter: EpochNotifierEmitter;

type BlockEmitter: BlockNotifierEmitter;

fn _init(app: ::ApplicationInterface) {

ok!(Self::init(app))

}

fn init(app: &c!(C::ApplicationInterface)) -> Self;

/// Returns a reference to the emitter end of this notifier. Should only be used if we are

/// interested (and responsible) for triggering a notification around new block.

fn new_block_emitter(&self) -> Self::BlockEmitter;

/// Returns a reference to the emitter end of this notifier. Should only be used if we are

/// interested (and responsible) for triggering a notification around new epoch.

fn new_epoch_emitter(&self) -> Self::EpochEmitter;

fn notify_on_new_block(&self, tx: mpsc::Sender<Notification>);

fn notify_on_new_epoch(&self, tx: mpsc::Sender<Notification>);

fn notify_before_epoch_change(&self, duration: Duration, tx: mpsc::Sender<Notification>);

}

#[infusion::blank]

pub trait EpochNotifierEmitter: Clone + Send + Sync + 'static {

/// Notify the waiters about epoch change.

fn epoch_changed(&self);

}

#[infusion::blank]

pub trait BlockNotifierEmitter: Clone + Send + Sync + 'static {

/// Notify the waiters about new block.

fn new_block(&self);

}

#[infusion::service]

pub trait NotifierInterface<C: Collection>: Sync + Send + Clone {

type Emitter: Emitter;

fn _init(app: ::ApplicationInterface) {

ok!(Self::init(app))

}

fn init(app: &c!(C::ApplicationInterface)) -> Self;

/// Returns a reference to the emitter end of this notifier. Should only be used if we are

/// interested (and responsible) for triggering a notification around new epoch.

fn get_emitter(&self) -> Self::Emitter;

fn notify_on_new_block(&self, tx: mpsc::Sender<Notification>);

fn notify_on_new_epoch(&self, tx: mpsc::Sender<Notification>);

fn notify_before_epoch_change(&self, duration: Duration, tx: mpsc::Sender<Notification>);

}

#[infusion::blank]

pub trait Emitter: Clone + Send + Sync + 'static {

/// Notify the waiters about epoch change.

fn epoch_changed(&self);

/// Notify the waiters about new block.

fn new_block(&self);

}

…ffair socket

daltoncoder

LGTM

bidzyyys requested a review from daltoncoder January 11, 2024 19:10

bidzyyys force-pushed the notifier branch from 0ab5f81 to a841550 Compare January 12, 2024 10:09

bidzyyys marked this pull request as ready for review January 12, 2024 11:09

bidzyyys self-assigned this Jan 12, 2024

bidzyyys force-pushed the notifier branch from a841550 to cf91e9e Compare January 15, 2024 10:55

bidzyyys requested a review from qti3e January 15, 2024 10:56

daltoncoder suggested changes Jan 15, 2024

View reviewed changes

qti3e suggested changes Jan 15, 2024

View reviewed changes

bidzyyys requested review from daltoncoder and qti3e January 16, 2024 15:09

bidzyyys force-pushed the notifier branch from e81017e to a850872 Compare January 17, 2024 16:05

bidzyyys changed the title ~~Take ExecutionEngineSocket from executor instead of cloning it~~ Take ExecutionEngineSocket from executor instead of cloning it + Notifier refactor Jan 17, 2024

qti3e reviewed Jan 17, 2024

View reviewed changes

qti3e and others added 9 commits January 18, 2024 17:12

Add an emitter end to the notifier interface

8a264aa

chore(application): take ExecutionEngineSocket instead of cloning it

7631a7b

refactor(nofitifier): improve interface around New Epoch notification

8697067

refactor(application): apply changes from CR

c026ca0

refactor(consensus): get rid of injection of notifications into the A…

5449b09

…ffair socket

feat(notifier): interface around new block notification

160dd66

refactor: Notifier notifies about new block

9de86b8

test(notifier): add notifications tests

7155676

refactor(notifier): apply CR changes

8146e2a

bidzyyys force-pushed the notifier branch from c8b624f to 8146e2a Compare January 18, 2024 16:13

bidzyyys requested a review from qti3e January 19, 2024 11:14

daltoncoder approved these changes Jan 19, 2024

View reviewed changes

qti3e approved these changes Jan 19, 2024

View reviewed changes

qti3e merged commit 75a61ac into main Jan 19, 2024

qti3e deleted the notifier branch January 19, 2024 18:07

	update_socket: Arc<Mutex<Option<ExecutionEngineSocket>>>,
	update_socket: Mutex<Option<ExecutionEngineSocket>>,

	update_socket: Arc::new(Mutex::new(Some(TokioSpawn::spawn_async(
	update_socket: Mutex::new(Some(TokioSpawn::spawn_async(

	fn transaction_executor(&self) -> Option<ExecutionEngineSocket> {
	fn transaction_executor(&self) -> ExecutionEngineSocket> {

Take ExecutionEngineSocket from executor instead of cloning it + Notifier refactor #13

Take ExecutionEngineSocket from executor instead of cloning it + Notifier refactor #13

Uh oh!

Conversation

bidzyyys commented Jan 11, 2024

Uh oh!

daltoncoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qti3e left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bidzyyys commented Jan 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daltoncoder left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants