refactor: queue-based block production + better separation of node's mutable state #517

itegulov · 2025-01-08T08:27:01Z

What 💻

Closes #501

Apologies for an insane diff once again - one change required another so I ended up refactoring more stuff than originally intended. Vast part of the diff is just moving from std::sync::RwLock to tokio::sync::RwLock (was needed due to lifetime shenanigans) and hence adding async/await in a lot of places that did not require it before. I will try to give a brief overview of functional changes below:

Moved (most of) mutable node state into a separate module inner. Essentially trying to restrict writeable access to as little entrypoints as I could (see inner rustdoc for more info)
Introduced a new layer purely for blockchain state: inner/blockchain.rs that has reader/writer structs. BlockchainWriter is held exclusively by InMemoryNodeInner and is inaccessible from outside inner module. All API endpoints were refactored to rely on BlockchainReader for their queries, thus removing the need to lock the entirety of InMemoryNodeInner.
Refactored time module into TimeWriter and TimeReader. Former is owned by InMemoryNodeInner and is inaccessible from outside of inner module. Latter can still be used to read current time in API endpoints.
BlockProducer was renamed to NodeExecutor and is the sole place that can seal blocks. Works via mpsc command queue that ensures no contentions between time lock and block production. Note: some more improvements can still be made to relax its lock holding but this at least removes the requirement to hold time lock for the entire block production time. Can also seal multiple blocks "atomically" (term used pretty loosely here but hopefully conveys the meaning).
BlockSealer was adapted accordingly and is now a separate background process that pushes commands to BlockProducer

Why ✋

Hopefully better code quality + less lock contention across the board

# Conflicts: # crates/cli/src/cli.rs # crates/cli/src/main.rs # crates/core/src/node/eth.rs # crates/core/src/node/in_memory.rs # crates/core/src/node/in_memory_ext.rs # crates/core/src/node/zks.rs # crates/core/src/system_contracts.rs

popzxc

Overall I like what I see, but tbh it feels much more complicated and low-level than it intuitively has to be.

Left a few preliminary comments.

popzxc · 2025-01-08T09:48:52Z

crates/api_server/src/impls/net.rs

+        let chain_id = tokio::runtime::Handle::current()
+            .block_on(async { self.node.get_chain_id().await.map_err(RpcError::from) })?;


Why not make this method async as well?

popzxc · 2025-01-08T09:49:28Z

crates/cli/src/bytecode_override.rs

@@ -19,7 +19,7 @@ struct Bytecode {

 // Loads a list of bytecodes and addresses from the directory and then inserts them directly
 // into the Node's storage.
-pub fn override_bytecodes(node: &InMemoryNode, bytecodes_dir: String) -> eyre::Result<()> {
+pub async fn override_bytecodes(node: &InMemoryNode, bytecodes_dir: String) -> eyre::Result<()> {
    for entry in fs::read_dir(bytecodes_dir)? {


Given that you made this function async, probably it makes sense to replace calls with non-blocking alternatives.

popzxc · 2025-01-08T09:54:19Z

crates/core/src/node/eth.rs

-            None => Ok(None),
-        }
+    ) -> anyhow::Result<Option<api::TransactionReceipt>> {
+        // TODO: Call fork if not found


Should we create a task for that?

popzxc · 2025-01-08T09:59:15Z

crates/core/src/node/eth.rs

+                .inner
+                .read()
+                .await
+                .fork_storage
+                .inner
+                .read()
+                .expect("failed reading fork storage")
+                .fork
+                .as_ref()
+                .and_then(|fork| {


Nit: here and in other places the composition looks super awkward. A good candidate for the structure being re-thought. Not suggesting to do in this PR obv.

popzxc · 2025-01-08T10:05:32Z

crates/core/src/node/inner/blockchain.rs

+/// A single-instance writer to blockchain state that is only available to [`super::InMemoryNodeInner`].
+pub(super) struct BlockchainWriter {
+    pub(super) inner: Arc<RwLock<Blockchain>>,
+}


Can't we express BlockchainReader and BlockchainWriter as two traits? Actually, probably BlockchainWriter is not even needed -- you can expose only Box<dyn BlockchainReader> outside of the crate, and rework Blockchain as pub(crate) struct Blockchain { inner: Arc<RwLock<Blockchain>> }.

With that, hopefully, you will be able to expose all the required mutability methods on the structure itself so that you don't have to leak guards in the interface.

popzxc · 2025-01-08T10:10:36Z

crates/core/src/node/inner/mod.rs

+use tokio::sync::RwLock;
+
+impl InMemoryNodeInner {
+    // TODO: Bake in Arc<RwLock<_>> into the struct itself


Most of structures already have rwlocks inside, no? This indeed feels a bit clunky

popzxc · 2025-01-08T10:13:12Z

crates/core/src/node/inner/node_executor.rs

+impl Future for NodeExecutor {
+    type Output = ();
+
+    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {


Maybe it's a stupid question, but why can't we just do a run method that would do something like this?
It feels a bit overengineered.

while let Some(command) = this.command_receiver.next().await { ... }

popzxc · 2025-01-08T10:14:46Z

crates/core/src/node/inner/node_executor.rs

+}
+
+#[derive(Debug)]
+pub enum Command {


Does it have to be public?

popzxc · 2025-01-08T10:19:29Z

crates/core/src/node/sealer.rs

+    }
+}
+
+impl Future for BlockSealer {


Similarly -- cannot we express this as a simple loop?

itegulov added 8 commits December 19, 2024 17:03

make block producer an actor

3438185

refactor inner node state into a separate module

8942a01

fix unit tests

8628850

fix block sealer tests

b179b4f

rename block producer to node executor

23aac2b

refactor time: remove traits, rename structs

f45ae07

relax trait bounds for ArcRLock's clone

1101bd5

itegulov requested review from Romsters, popzxc and dutterbutter January 8, 2025 08:27

itegulov requested a review from a team as a code owner January 8, 2025 08:27

clippy

514c871

popzxc reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: queue-based block production + better separation of node's mutable state #517

refactor: queue-based block production + better separation of node's mutable state #517

itegulov commented Jan 8, 2025 •

edited

Loading

popzxc left a comment

popzxc Jan 8, 2025

popzxc Jan 8, 2025

popzxc Jan 8, 2025

popzxc Jan 8, 2025

popzxc Jan 8, 2025

popzxc Jan 8, 2025

popzxc Jan 8, 2025

popzxc Jan 8, 2025

popzxc Jan 8, 2025

		let chain_id = tokio::runtime::Handle::current()
		.block_on(async { self.node.get_chain_id().await.map_err(RpcError::from) })?;

refactor: queue-based block production + better separation of node's mutable state #517

Are you sure you want to change the base?

refactor: queue-based block production + better separation of node's mutable state #517

Conversation

itegulov commented Jan 8, 2025 • edited Loading

What 💻

Why ✋

popzxc 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

itegulov commented Jan 8, 2025 •

edited

Loading