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

test(code/blocksync): Introduce small DSL for expressing test scenarios #496

Merged
merged 14 commits into from
Oct 28, 2024

Conversation

romac
Copy link
Member

@romac romac commented Oct 25, 2024

With this DSL, we can express more interesting test scenarios than previously.

Here is an example of a test that relies on BlockSync to make a node catch up to the others after crashing:

#[tokio::test]
pub async fn crash_restart() {
    const HEIGHT: u64 = 10;

    // Node 1 starts with 10 voting power.
    let n1 = TestNode::new(1)
        .vp(10)
        .start()
        // Wait until it reaches height 10
        .wait_until(HEIGHT)
        // Record a successful test for this node
        .success();

    // Node 2 starts with 10 voting power, in parallel with node 1 and with the same behaviour
    let n2 = TestNode::new(2).vp(10).start().wait_until(HEIGHT).success();

    // Node 3 starts with 5 voting power, in parallel with node 1 and 2.
    let n3 = TestNode::new(3)
        .vp(5)
        .start()
        // Then the test runner waits until it reaches height 2...
        .wait_until(2)
        // ...and kills the node!
        .crash()
        // After that, it waits 5 seconds before restarting the node
        .restart_after(Duration::from_secs(5))
        // Wait until the node reached the expected height
        .wait_until(HEIGHT)
        // Record a successful test for this node
        .success();

    Test::new([n1, n2, n3])
        .run_with_custom_config(
            App::Starknet,           // Application to run
            Duration::from_secs(30), // Timeout for the whole test
            TestParams {
                enable_blocksync: true, // Enable BlockSync
                ..Default::default()
            },
        )
        .await
}

The test runner is still very rudimentary, and lacks the ability to finely control
What each node is doing but we can hopefully improve that at some point, maybe by introducing a coordinator of some kind within the node for intercepting, injecting and routing messages between actors.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 89.14286% with 19 lines in your changes missing coverage. Please review.

Project coverage is 82.57%. Comparing base (13b9722) to head (70bd58b).
Report is 1 commits behind head on blocksync-mvp.

Files with missing lines Patch % Lines
code/crates/starknet/test/src/lib.rs 88.62% 19 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           blocksync-mvp     #496      +/-   ##
=================================================
+ Coverage          77.45%   82.57%   +5.12%     
=================================================
  Files                125      125              
  Lines               8835     8849      +14     
=================================================
+ Hits                6843     7307     +464     
+ Misses              1992     1542     -450     
Flag Coverage Δ
integration 82.61% <89.14%> (+5.07%) ⬆️
mbt 21.70% <ø> (-0.17%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Can be overriden locally with `MALACHITE_TRANSPORT=quic`
@romac romac force-pushed the romac/blocksync-test-dsl branch from 1f2781d to 8ee0e15 Compare October 25, 2024 14:00
@romac romac force-pushed the romac/blocksync-test-dsl branch from 8ee0e15 to fa0c118 Compare October 25, 2024 14:05
@romac romac requested a review from ancazamfir October 28, 2024 13:14
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

🚀 looks great!!

@romac romac merged commit 11a525c into blocksync-mvp Oct 28, 2024
14 checks passed
@romac romac deleted the romac/blocksync-test-dsl branch October 28, 2024 14:18
romac added a commit that referenced this pull request Oct 30, 2024
* feat(code): Add dummy `BlockSync` actor

* Add `/block_sync` pubsub channel

* Rename `broadcast` to `publish`

* Use `eyre` instead of boxed `Error`

* Have peers publish their status every time they move to a new height/round

* Fix bug where `StartRound` effect was performed twice

* Move status handling into BlockSync actor

* Less noise

* Move status update logic onto BlockSync actor

* Decouple GossipConsensus actor from Consensus and BlockSync actors

* Push latest hight from consensus to blocksync

* Revert "Decouple GossipConsensus actor from Consensus and BlockSync actors"

This reverts commit 0a59ada.

* feat(code): BlockSync RPC (#457)

* Define RPC behaviour for BlockSync

* Hook BlockSync behaviour into networking layer

* Revert to encoding-agnostic networking layer

* Hook BlockSync actor to BlockSync behaviour

* Fix unwraps

* Remove round from status

* code(feat): Add block store (#458)

* Add block store for blocksync testing

* Sync helper get stored block at height from host

* Add block store pruning

* Update Cargo.lock

* Add blocksync protocol initial implementation, wip

* Add config flag for maximum number of blocks in store

* Ask for the block only from peer at lower height.

* Fix mistake in host, should return ReceivedProposedValue and not
ProposedValue.

* When processing the synced block use the on_ handlers instead of
apply_driver_input.

* When storing the decision look for full proposal at proposal round and
NOT consensus round

* Change max_retain_blocks in config.toml to match the default.

* Set voting power for all vals to 1 for easier debugging.

* Use on_vote instead of apply_driver_input so our own commit is stored
and used on decided operations.

* Fix spelling.

* Remove lower number of peers (was used for testing).

* Store the commits in a set to avoid duplicates.

* Return if proposal from non-proposer.

* Add TODO for potential min block height in Status

* Change max_retain_blocks default to 100.

* Small cleanup

* Work around libp2p bug where a node cannot publish to a topic if restarted too quickly

See libp2p/rust-libp2p#5097

* Cleanup

* Ensure validator set is always sorted properly, even after being deserialized from genesis

* Update spawn.fish

* Move BlockSyncState to blocksync crate

* Add batched request for blocks

* Fix bug where only a single block would be sent back

* Revert block batching

* Cleanup

* Remove outdated comment

* Post-merge fixes

* Post-merge fix

* Cleanup

* chore(code): Extract logic from `BlockSync` actor and into `blocksync` crate (#487)

* chore(code): Extract BlockSync logic from actor and into `blocksync` crate

* Cleanup

* Fix doc

* Fix logs

* Use custom timeout per test

* Add timeouts to BlockSync requests (#490)

* feat(code/blocksync): Add timeouts to BlockSync requests

* Fix parsing of blocksync config

* Randomize choice of peer to sync from (#492)

* Add basic integration test (#493)

* Re-enable mempool in integration test

* Add config option `blocksync.enabled`

* Send back actual block bytes instead of just the block hash

* Fix example config file

* test(code/blocksync): Introduce small DSL for expressing test scenarios (#496)

* Introduce small DSL for expressing test scenarios

* Add basic integration test for BlockSync

* Update fish script

* Allow overriding the transport used in tests via a `MALACHITE_TRANSPORT` env variable

* Update test

* Add doc comments

* Use TCP by default in tests

Can be overriden locally with `MALACHITE_TRANSPORT=quic`

* Run integration tests sequentially

* Run integration tests in release mode

* Fix coverage job

* Try again

* Do not panic when tracing subscribers are set twice

* Enable basic optimizations in debug mode

* Update MSRV to 1.82

* feat(code/blocksync): Only request sync from peers which have the requested block in their store (#495)

* feat(code/blocksync): Only request sync from peers which have the requested block in their store

For this, we extend the status with the earliest block height available
in the store, to ensure we only request a block from peers which have
told us they have it.

* Remove `blocksync::State::random_peer_at_or_above()` method

* Update clippy msrv

* Add metrics

* Ensure Consensus and BlockSync actors never fail

---------

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants