Skip to content
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

feat: asynchronous and configurable block sealing #392

Merged
merged 13 commits into from
Nov 27, 2024

Conversation

itegulov
Copy link
Contributor

@itegulov itegulov commented Nov 20, 2024

What 💻

This PR introduces several new components:

  • TxPool - a very very basic version for now, essentially a mempool where we treat all txs to be ready at all times
  • BlockSealer - component that takes txs from TxPool and decides whether it is time to seal the block (supports two modes of operation - immediate and intervaled)
  • BlockProducer - polls BlockSealer for new block that got sealed and actually applies them in the node

CLI supports --block-time now (matching anvil's behaviour) and max_transactions config param (also matching anvil).

One caveat here is that the new behaviour means that transactions are not included in a block immediately after submission which might be something people relied on. That being said this was never a guarantee and was pretty much an implementation detail.

Still need to add e2e tests for the new functionality (intervaled sealing) and cover components with unit tests, but opening a draft now to collect early feedback. (UPD: Unit tests are done, e2e tests are being discussed)

Closes #362

Why ✋

See #362 for motivation

src/config/cli.rs Show resolved Hide resolved
src/node/eth.rs Show resolved Hide resolved
src/node/in_memory.rs Show resolved Hide resolved
src/node/pool.rs Show resolved Hide resolved
@dutterbutter
Copy link
Collaborator

Looking good 👍 - Apologies on the conflicts 😅

src/config/cli.rs Outdated Show resolved Hide resolved
@itegulov itegulov marked this pull request as ready for review November 21, 2024 07:31
@itegulov itegulov requested a review from a team as a code owner November 21, 2024 07:31
@itegulov
Copy link
Contributor Author

Alright I have added unit tests for the new functionality introduced in this PR, so IMO it's ready for review.

@popzxc need your opinion on e2e tests for intervaled block sealing (i.e. --block-time 1). My first idea was to just make GHA run the entire e2e test suite twice: once for immediate sealing, once for intervaled sealing. Now, the problem is that most tests take ~20s with intervaled sealing as we have to wait up to 1s after every tx. Which means the entire suite takes >10 mins to finish which is far from ideal. An alternative would be to run a subsection of e2e tests just to make sure block sealing happens; main.test.ts seems like a good candidate for that.

Looking a bit ahead, the number of parameters will only continue to rise: we will be adding a couple more modes of block sealing, alternative modes of batch sealing, tx order etc. So it feels like there should be a separate mechanism for testing them - like a suite akin to spec-tests that can spin up a node and test its behaviour with dynamically provided CLI opts/config (as opposed to e2e-tests that expect the node to be set up externally). This would be a fairly heavy endeavour though. For now I am trying to design the new components in a decoupled and testable way so we at least have unit tests to ensure low-level functionality is there.

@dutterbutter
Copy link
Collaborator

dutterbutter commented Nov 25, 2024

Alright I have added unit tests for the new functionality introduced in this PR, so IMO it's ready for review.

@popzxc need your opinion on e2e tests for intervaled block sealing (i.e. --block-time 1). My first idea was to just make GHA run the entire e2e test suite twice: once for immediate sealing, once for intervaled sealing. Now, the problem is that most tests take ~20s with intervaled sealing as we have to wait up to 1s after every tx. Which means the entire suite takes >10 mins to finish which is far from ideal. An alternative would be to run a subsection of e2e tests just to make sure block sealing happens; main.test.ts seems like a good candidate for that.

Looking a bit ahead, the number of parameters will only continue to rise: we will be adding a couple more modes of block sealing, alternative modes of batch sealing, tx order etc. So it feels like there should be a separate mechanism for testing them - like a suite akin to spec-tests that can spin up a node and test its behaviour with dynamically provided CLI opts/config (as opposed to e2e-tests that expect the node to be set up externally). This would be a fairly heavy endeavour though. For now I am trying to design the new components in a decoupled and testable way so we at least have unit tests to ensure low-level functionality is there.

My 2 cents here, I think we should proceed with the initial thought of running a subsection of e2e tests in the meantime, and then plan for updating e2e tests as you've described. We likely will need to prioritize the e2e test refactor sooner rather then later but well worth it. For the refactor, can we make use of alloy-zksync node bindings here to spin up the node, and pass CLI params accordingly? We would have to build the binary, and then use at() and point to the binary but it should simplify the setup?

e.g.

let era_test_node = EraTestNode::at("/path/to/era_test_node")
        .block_time(1)
        .spawn();

Copy link
Collaborator

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the conflicts, otherwise LGTM. Let's merge this after the alpha.33 release?

@dutterbutter dutterbutter added the needs changes 🔧 Review requested changes on a PR label Nov 25, 2024
@popzxc
Copy link
Member

popzxc commented Nov 26, 2024

@itegulov I would probably go with module test approach. Somewhat similar to what @dutterbutter proposed -- use alloy-zksync to spawn the node, then write a few short Rust tests to check that functionality works as intended. No need to run the whole suite twice.

@itegulov itegulov force-pushed the daniyar/async-block-sealer branch from badc548 to 78d0c47 Compare November 27, 2024 01:34
@itegulov itegulov force-pushed the daniyar/async-block-sealer branch 2 times, most recently from f7f9e5f to 25375b2 Compare November 27, 2024 04:36
@itegulov itegulov force-pushed the daniyar/async-block-sealer branch from 25375b2 to 7900968 Compare November 27, 2024 04:40
@itegulov itegulov force-pushed the daniyar/async-block-sealer branch from 6ba9d7b to 767f5c5 Compare November 27, 2024 04:44
tokio = { version = "1", features = ["time", "rt", "process"] }

[dev-dependencies]
alloy-zksync = { git = "https://github.com/itegulov/alloy-zksync.git", rev = "b433d2d64e0f7d939891064b00d8a7991a427682" }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using my own branch for now as this needs popzxc/alloy-zksync#28 to be upstreamed

@itegulov
Copy link
Contributor Author

Merging as is for now, as there is a cyclic dependency between this PR and popzxc/alloy-zksync#28 😬

@itegulov itegulov merged commit 47b3bf2 into main Nov 27, 2024
13 checks passed
@itegulov itegulov deleted the daniyar/async-block-sealer branch November 27, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changes 🔧 Review requested changes on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple txs per block
3 participants