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(code): Test implementations for mempool, tx batch streaming and value builder #191

Merged
merged 33 commits into from
Jun 4, 2024

Conversation

romac
Copy link
Member

@romac romac commented May 15, 2024

No description provided.

@romac
Copy link
Member Author

romac commented May 15, 2024

Looks like the driver_steps_not_proposer_other_height test is now failing, will look into it:

thread 'driver_steps_not_proposer_other_height' panicked at test/tests/driver.rs:1280:49:
execute succeeded: InvalidProposalHeight { proposal_height: Height(2), consensus_height: Height(1) }

@romac
Copy link
Member Author

romac commented May 15, 2024

Oh I guess it's because of the two new assertions (which I have turned into errors to make sure we never panic in the driver just in case). I'll update the test to account for that.

@romac
Copy link
Member Author

romac commented May 15, 2024

I think we should also convert the two new assertions in the consensus actor into proper errors instead of panicking in case they are not met. What do you think @ancazamfir?

Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 78.82273% with 313 lines in your changes missing coverage. Please review.

Project coverage is 83.83%. Comparing base (74dd726) to head (d4b9f35).

Files Patch % Lines
code/actors/src/mempool.rs 50.00% 52 Missing ⚠️
code/gossip-mempool/src/lib.rs 71.52% 43 Missing ⚠️
code/actors/src/consensus.rs 71.57% 29 Missing ⚠️
code/gossip-mempool/src/handle.rs 45.10% 28 Missing ⚠️
code/gossip/src/lib.rs 55.10% 22 Missing ⚠️
code/test/src/block_part.rs 86.75% 20 Missing ⚠️
code/actors/src/gossip_mempool.rs 79.78% 18 Missing ⚠️
code/actors/src/util/value_builder.rs 90.74% 15 Missing ⚠️
code/network-mempool/src/peer_id.rs 0.00% 13 Missing ⚠️
code/cli/src/cmd/start.rs 0.00% 11 Missing ⚠️
... and 14 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   86.21%   83.83%   -2.38%     
==========================================
  Files          56       67      +11     
  Lines        3720     4779    +1059     
==========================================
+ Hits         3207     4006     +799     
- Misses        513      773     +260     
Flag Coverage Δ
integration 83.83% <78.82%> (-2.44%) ⬇️
mbt 17.94% <0.00%> (-2.43%) ⬇️

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 requested a review from ancazamfir May 15, 2024 12:16
@romac romac removed the request for review from ancazamfir May 15, 2024 13:11
romac and others added 8 commits May 15, 2024 16:40
* Use random identity for mempool gossip

* Only accept messages from peers using the same protocol

* Formatting

* Enable logging in tests

* Increase timeout

* Another attempt to fix test

* Comment out dead code

* Revert timeout change

* Run tests on macOS as well

* Use Ubuntu 24

* Revert "Use Ubuntu 24"

This reverts commit f5d7f88.

* Use TCP transport instead of QUIC

* Send a Ready message to all peers, and only start consensus when we got enough of those

* Disable peer filtering based on protocol version

* Remove Ready message

* Fix bug in gossip mempool

* Disable peer filtering based on protocol version again

* Switch back to QUIC

* Cleanup

* No need for different identities if we do not blacklist peers

* Remove commented out code

* No need to act on expired mDNS entries

* Send PeerDisconnected event to handle when a peer unsubscribes

* No need to test on macOS anymore

* Cleanup

* Fix "API rate limit exceed" error when installing `protoc`
* Add gossip blockpart channel and BlockPart trait and test impl

* Sign and gossip block parts, receive them, validate, send to proposal builder

* Value builder now streams txes for a fraction of propose timeout

* Add constants for hardcoded values

* Fix proposal builder initialization and tweak some constants
@ancazamfir ancazamfir changed the title feat(code): Add initial implementation of gossip-based mempool feat(code): Test implementations for mempool, tx batch streaming and value builder May 28, 2024
ancazamfir and others added 7 commits May 29, 2024 00:03
Revert the consensus proposal value to be the block hash (or ValueId).
Multiplex proposal and last block part, pass valid from the latter.

Check if all parts are present when a proposal is received, otherwise
queue.

Implement a temporary store for block parts.
…salBuilder` actor (#199)

* Rename field for consistency

* Resolve cyclic dependency between Consensus and ProposalBuilder actor
romac and others added 3 commits May 31, 2024 15:45
…ctor (#200)

* Remove CAL actor and move `GetValidatorSet` into `ProposalBuilder`

* Rename `ProposalBuilder` actor to `Host`

* Make `PartStore` context-generic and move into its own module (#201)
* Wire up `persistent_peers` from config to both gossip layers

* simplified index command to only generate configs when init command is run

* Cleanup dialing of persistent peers

* Only add explicit peers when their protocol version matches

* Cleanup

* Fix args parsing test

* Use random base ports per test

* Formatting

---------

Co-authored-by: Greg Szabo <greg@philosobear.com>
* removed index parameter, added testnet command, simplified init command

* Update help message

* Remove `--config`, `--genesis` and `--private-key` options

* Unpretty private key bytes

* Log output in `testnet` command

* Use CometBFT-compatible format for `priv_validator_key.json` file

* Add test for `testnet` command

---------

Co-authored-by: Greg Szabo <greg@philosobear.com>
@romac romac marked this pull request as ready for review June 4, 2024 09:55
@romac romac merged commit 2ae7f78 into main Jun 4, 2024
12 of 13 checks passed
@romac romac deleted the anca/mempool_for_testing branch June 4, 2024 11:45
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.

3 participants