Closed itegulov closed 3 days ago
Looking good 👍 - Apologies on the conflicts 😅
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.
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 toe2e-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();
@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.
Merging as is for now, as there is a cyclic dependency between this PR and https://github.com/popzxc/alloy-zksync/pull/28 :grimacing:
What :computer:
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 timesBlockSealer
- 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 nodeCLI supports
--block-time
now (matching anvil's behaviour) andmax_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 :hand:
See #362 for motivation