feat(engine): hooks #4582

shekhirin · 2023-09-13T15:41:52Z

This PR introduces the engine hooks (name is debatable, we can also call them triggers or actions). They are used to extend the functionality of consensus engine with custom logic provided by the caller (in src/node/mod.rs for now, and in CLI builder later).

The first instance of such functionality is the pruner, that needs to be run on every iteration of the main engine loop. Next will be snapshot hook (#4588).

Only the way pruner is called from the engine has changed in this PR, and no changes in engine or pruner behavior are expected.

codecov · 2023-09-13T18:26:55Z

Codecov Report

Merging #4582 (d5f2101) into main (b5905c4) will decrease coverage by 0.08%.
The diff coverage is 73.07%.

Files Changed	Coverage Δ
bin/reth/src/node/mod.rs	`26.11% <0.00%> (-0.08%)`	⬇️
crates/consensus/beacon/src/engine/error.rs	`33.33% <ø> (ø)`
crates/consensus/beacon/src/engine/metrics.rs	`100.00% <ø> (ø)`
crates/interfaces/src/sync.rs	`40.00% <0.00%> (ø)`
crates/consensus/beacon/src/engine/hooks/mod.rs	`65.21% <65.21%> (ø)`
...es/consensus/beacon/src/engine/hooks/controller.rs	`74.24% <74.24%> (ø)`
crates/consensus/beacon/src/engine/hooks/prune.rs	`86.25% <80.00%> (ø)`
crates/consensus/beacon/src/engine/mod.rs	`74.42% <80.64%> (-0.18%)`	⬇️
crates/consensus/beacon/src/engine/test_utils.rs	`68.21% <100.00%> (+0.31%)`	⬆️

... and 11 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.93% <0.00%> (-0.04%)`	⬇️
unit-tests	`63.17% <73.07%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`31.94% <0.00%> (-0.02%)`	⬇️
blockchain tree	`83.59% <ø> (ø)`
pipeline	`88.54% <ø> (ø)`
storage (db)	`73.47% <ø> (ø)`
trie	`94.70% <ø> (-0.04%)`	⬇️
txpool	`49.44% <ø> (-0.49%)`	⬇️
networking	`77.17% <ø> (-0.05%)`	⬇️
rpc	`57.31% <ø> (-0.01%)`	⬇️
consensus	`62.58% <76.43%> (+0.16%)`	⬆️
revm	`19.66% <ø> (ø)`
payload builder	`9.06% <ø> (ø)`
primitives	`86.43% <0.00%> (-0.04%)`	⬇️

mattsse

I like the direction,
we should think about the trait a bit more

mattsse · 2023-09-14T09:41:05Z

crates/consensus/beacon/src/engine/hooks/mod.rs

+    /// Returns an [action][`HookAction`], if any, according to the passed [event][`HookEvent`].
+    fn on_event(&mut self, event: HookEvent) -> Result<Option<HookAction>, HookError>;


this I don't understand after reading the event docs.

why do we feed the outcome of poll back into an event callback, this looks unnecessary because the hook already knows this.

we act on Started/Finished here:

reth/crates/consensus/beacon/src/engine/hooks/controller.rs

Lines 64 to 68 in 2a6fd5c

if !finished {

self.running_hook_with_db_write = Some((hook_idx, hook));

} else {

self.hooks[hook_idx] = Some(hook);

}

reth/crates/consensus/beacon/src/engine/hooks/controller.rs

Lines 123 to 127 in 2a6fd5c

if started && hook.dependencies().db_write {

self.running_hook_with_db_write = Some((hook_idx, hook));

} else {

self.hooks[hook_idx] = Some(hook);

}

also, there might be no action required (as will be for snapshots, for example), but the hook still need to signal as about its status.

We can combine it into one HookEvent::Finished(HookAction) though, and it will be returned on hook polling.

ok, I just made the Hook::poll return both event and an optional action, so we could:

Act on event and set/unset the running_hook_with_db_write

Pass the action further down

mattsse · 2023-09-14T09:45:10Z

crates/consensus/beacon/src/engine/hooks/mod.rs

+    /// Returns [dependencies][`HookDependencies`] for running this hook.
+    fn dependencies(&self) -> HookDependencies;


this implies the dependencies are required for the duration of the hook which might not be the case.

I think we can solve this a bit differently, for example with multiple poll functions:

like poll_start -> Poll<Deps>,
and poll_hook()

not following...

this implies the dependencies are required for the duration of the hook which might not be the case.

for DB write access, this is the case. Not sure what other dependencies we might have in the future though, so you might be right, but should we be ready for this now?

simplified according to #4582 (comment)

crates/consensus/beacon/src/engine/hooks/mod.rs

mattsse · 2023-09-14T09:46:49Z

crates/consensus/beacon/src/engine/hooks/controller.rs

+    hooks: Vec<Option<Box<dyn Hook>>>,
+    /// Next hook index to poll.
+    next_hook_idx: usize,
+    /// Currently running hook with DB write access, if any.
+    running_hook_with_db_write: Option<(usize, Box<dyn Hook>)>,


is this just a VecDeque?

thanks, fixed

rkrasiuk · 2023-09-15T06:49:32Z

crates/consensus/beacon/src/engine/mod.rs

+                match self.blockchain.restore_canonical_hashes() {
+                    Ok(()) => {}
+                    Err(error) => {
+                        error!(target: "consensus::engine", ?error, "Error restoring blockchain tree state");
+                        return Err(error.into())


Suggested change

match self.blockchain.restore_canonical_hashes() {

Ok(()) => {}

Err(error) => {

error!(target: "consensus::engine", ?error, "Error restoring blockchain tree state");

return Err(error.into())

if let Err(error) = self.blockchain.restore_canonical_hashes() {

error!(target: "consensus::engine", ?error, "Error restoring blockchain tree state");

return Err(error.into())

rkrasiuk · 2023-09-15T06:57:06Z

crates/consensus/beacon/src/engine/hooks/mod.rs

+/// Dependencies that [hook][`Hook`] require for execution.
+pub struct HookDependencies {
+    /// Hook needs DB write access. If `true`, then only one hook with DB write access can be run
+    /// at a time.
+    pub db_write: bool,
+}


shall we keep it simple for now and get rid of this generalization? dependencies naming is a bit confusing and db write access feels important enough to be extracted to fn db_access_level() -> HookDbAccess on the Hook trait

enum HookDbAccess { RO, RW }

or smth similar

agree, that would be enough for prune and snapshot hooks, and we can add more functionality on top later

it would also be nice to somehow restrict the database provider that custom user-provided hook has access to according to this level

crates/consensus/beacon/src/engine/mod.rs

joshieDo · 2023-09-15T11:50:10Z

crates/consensus/beacon/src/engine/mod.rs

+            if is_pending && !this.forkchoice_state_tracker.is_latest_invalid() {
+                if let Poll::Ready(result) = this.hooks.poll_next_hook(
+                    cx,
+                    HookArguments { tip_block_number: this.blockchain.canonical_tip().number },
+                    HookDependencies { db_write: this.sync.is_pipeline_active() },
+                ) {
+                    if let Err(err) = this.on_hook_action(result?) {
+                        return Poll::Ready(Err(err))
+                    }
                }
            }


So we are only polling one hook (maybe two if there was a previous write poll) per engine poll.

Is that intended? why not more? we don't want to potentially block the FCU handle?

random thought: is it possible to run them in parallel here (as long as they're read-only)?

we don't want to potentially block the FCU handle?

yes, that was the main reason, so we would return the control asap and process any new incoming messages, because they're higher priority than hooks

joshieDo · 2023-09-15T11:53:14Z

crates/consensus/beacon/src/engine/hooks/mod.rs

+pub enum HookAction {
+    /// Notify about a [SyncState] update.
+    UpdateSyncState(SyncState),
+    /// Read the last relevant canonical hashes from the database and update the block indices of
+    /// the blockchain tree.
+    RestoreCanonicalHashes,
+}


Given these variants... is the goal to move the engine fcu handling to an hook as well?

hmm yeah, that's a good idea, I think we can do that with both engine messages handling and pipeline. This would also breakdown the huge engine/mod.rs file and separate the engine polling, engine messages and pipeline control logics. @mattsse wdyt?

I don't want to hide the actual FCU inside a dyn Hook,
the entire purpose of the engine is to handle fcu and payload, this should be baked in directly

mattsse · 2023-09-15T12:55:38Z

crates/consensus/beacon/src/engine/hooks/mod.rs

+
+/// Hook that will be run during the main loop of
+/// [consensus engine][`crate::engine::BeaconConsensusEngine`].
+pub trait Hook: Send + Sync + 'static {


I wanna start bikeshedding the name,

how about EngineHook?

i like that

mattsse · 2023-09-15T12:58:26Z

crates/consensus/beacon/src/engine/hooks/mod.rs

+
+/// Arguments passed to the [hook polling function][`Hook::poll`].
+#[derive(Copy, Clone, Debug)]
+pub struct HookArguments {


I don't think Arguments is fitting here,
how about EngineContext?

mattsse

lgtm

rkrasiuk

lgtm

shekhirin added 27 commits September 12, 2023 12:46

feat(engine): hooks

b0be50c

hook arguments

3a2e623

running hook with db write

dcc04cd

debug logs

df182b7

helper enum fns

95a5297

fix vec init

27a14bc

fix poll logic

a783551

don't return pending

6ba1383

ughh ugly but works

ed43ac9

fix unwrap

5396010

hook controller

08810a4

simplify hooks idx access

250c889

move hook idx incr

6e0b236

change prune hook name

13a6bd2

reorg crates

8b5c1e9

hooks collections

ea4297d

improve hook event finished result

1a0d994

remove is_pipeline_active from args

9330962

move controller to a separate mod

0de8793

Merge remote-tracking branch 'origin/main' into alexey/engine-hooks

e5cc193

fix comments

477e9e3

simplify deps

73ee4ae

doc comments

7c2eaec

plural

1b5e4c9

fix lint

4708854

better hooks controller docs

e2b4a5b

fix test consensus engine init

2a6fd5c

shekhirin marked this pull request as ready for review September 13, 2023 18:29

shekhirin requested a review from rkrasiuk as a code owner September 13, 2023 18:29

shekhirin requested review from mattsse, Rjected, onbjerg and gakonst as code owners September 13, 2023 18:29

shekhirin requested a review from joshieDo September 13, 2023 18:32

rkrasiuk added C-enhancement New feature or request A-consensus Related to the consensus engine labels Sep 14, 2023

mattsse requested changes Sep 14, 2023

View reviewed changes

vecdeque, rework actions

f86f4da

shekhirin force-pushed the alexey/engine-hooks branch from 86fad45 to f86f4da Compare September 14, 2023 15:10

shekhirin requested a review from mattsse September 14, 2023 16:39

rkrasiuk reviewed Sep 15, 2023

View reviewed changes

joshieDo reviewed Sep 15, 2023

View reviewed changes

crates/consensus/beacon/src/engine/mod.rs Show resolved Hide resolved

joshieDo reviewed Sep 15, 2023

View reviewed changes

shekhirin force-pushed the alexey/engine-hooks branch from e552f18 to ba037ee Compare September 15, 2023 12:42

dependencies -> db access level

3b69c3b

shekhirin force-pushed the alexey/engine-hooks branch from ba037ee to 3b69c3b Compare September 15, 2023 12:45

mattsse reviewed Sep 15, 2023

View reviewed changes

engine hooks, engine context

7926ccf

shekhirin requested a review from mattsse September 15, 2023 14:11

mattsse approved these changes Sep 18, 2023

View reviewed changes

rkrasiuk approved these changes Sep 18, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into alexey/engine-hooks

d5f2101

shekhirin added this pull request to the merge queue Sep 18, 2023

Merged via the queue into main with commit 11f5f3f Sep 18, 2023
22 checks passed

shekhirin deleted the alexey/engine-hooks branch September 18, 2023 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(engine): hooks #4582

feat(engine): hooks #4582

shekhirin commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

mattsse left a comment

mattsse Sep 14, 2023

shekhirin Sep 14, 2023

shekhirin Sep 14, 2023

mattsse Sep 14, 2023

shekhirin Sep 14, 2023

shekhirin Sep 15, 2023

mattsse Sep 14, 2023

shekhirin Sep 14, 2023

rkrasiuk Sep 15, 2023

rkrasiuk Sep 15, 2023

shekhirin Sep 15, 2023

shekhirin Sep 15, 2023 •

edited

Loading

joshieDo Sep 15, 2023 •

edited

Loading

joshieDo Sep 15, 2023 •

edited

Loading

shekhirin Sep 15, 2023

joshieDo Sep 15, 2023

shekhirin Sep 15, 2023

mattsse Sep 15, 2023

mattsse Sep 15, 2023

shekhirin Sep 15, 2023

mattsse Sep 15, 2023

mattsse left a comment

rkrasiuk left a comment •

edited

Loading

		/// Returns an [action][`HookAction`], if any, according to the passed [event][`HookEvent`].
		fn on_event(&mut self, event: HookEvent) -> Result<Option<HookAction>, HookError>;

	if !finished {
	self.running_hook_with_db_write = Some((hook_idx, hook));
	} else {
	self.hooks[hook_idx] = Some(hook);
	}

	if started && hook.dependencies().db_write {
	self.running_hook_with_db_write = Some((hook_idx, hook));
	} else {
	self.hooks[hook_idx] = Some(hook);
	}

		/// Returns [dependencies][`HookDependencies`] for running this hook.
		fn dependencies(&self) -> HookDependencies;

feat(engine): hooks #4582

feat(engine): hooks #4582

Conversation

shekhirin commented Sep 13, 2023 • edited Loading

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shekhirin Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

joshieDo Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

joshieDo Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

rkrasiuk left a comment • edited Loading

Choose a reason for hiding this comment

shekhirin commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

shekhirin Sep 15, 2023 •

edited

Loading

joshieDo Sep 15, 2023 •

edited

Loading

joshieDo Sep 15, 2023 •

edited

Loading

rkrasiuk left a comment •

edited

Loading