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

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

Merged
merged 5 commits into from
Oct 23, 2024

Conversation

romac
Copy link
Member

@romac romac commented Oct 22, 2024

Similar to what we did for consensus, this PR extracts all the logic from the BlockSync actor and moves it as a coroutine in the blocksync crate. This way the BlockSync logic can be re-usable even if one does not use our own node implementation.

@romac romac requested a review from ancazamfir October 22, 2024 14:39
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 62.56684% with 70 lines in your changes missing coverage. Please review.

Project coverage is 79.26%. Comparing base (c519274) to head (85526ce).
Report is 1 commits behind head on blocksync-mvp.

Files with missing lines Patch % Lines
code/crates/blocksync/src/handle.rs 57.14% 39 Missing ⚠️
code/crates/actors/src/block_sync.rs 60.00% 28 Missing ⚠️
code/crates/consensus/src/handle/driver.rs 0.00% 2 Missing ⚠️
code/crates/actors/src/gossip_consensus.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           blocksync-mvp     #487      +/-   ##
=================================================
- Coverage          80.71%   79.26%   -1.45%     
=================================================
  Files                121      124       +3     
  Lines               8476     8593     +117     
=================================================
- Hits                6841     6811      -30     
- Misses              1635     1782     +147     
Flag Coverage Δ
integration 79.37% <62.57%> (-1.46%) ⬇️
mbt 22.02% <ø> (ø)

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.

@romac romac changed the title chore(code): Extract BlockSync logic from actor and into blocksync crate chore(code): Extract logic from BlockSync actor and into blocksync crate Oct 22, 2024
@romac romac mentioned this pull request Oct 22, 2024
7 tasks
@romac romac force-pushed the romac/blocksync-coroutine branch from 0cdb0e1 to c54a7ff Compare October 23, 2024 08:46
@romac romac force-pushed the romac/blocksync-coroutine branch from ee52e79 to 85526ce Compare October 23, 2024 11:49
@romac romac merged commit 585bf5d into blocksync-mvp Oct 23, 2024
12 of 13 checks passed
@romac romac deleted the romac/blocksync-coroutine branch October 23, 2024 12:38
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.

1 participant