Add preconfirmation gossip messages #2726
Conversation
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn run__gossip_message_from_p2p_service_is_broadcasted__tx_confirmations() { |
There was a problem hiding this comment.
This is the only new test in this file. The rest were just copied over during the refactor.
|
|
||
| #[tokio::test] | ||
| #[instrument] | ||
| async fn gossipsub_broadcast_tx_with_reject__tx_confirmations() { |
|
|
||
| #[tokio::test] | ||
| #[instrument] | ||
| async fn gossipsub_broadcast_tx_with_accept__tx_confirmations() { |
|
|
||
| // TODO: Move me before tests that use this function | ||
| /// Reusable helper function for Broadcasting Gossipsub requests | ||
| async fn gossipsub_broadcast( |
There was a problem hiding this comment.
This method was updated to accept the new GossipsubBroadcastRequest::Confirmations variant.
Introduced check_message_matches_request as part of that (before it was hardcoded to look for a specific value).
MitchTurner
changed the title
TxConfirmation gossip messages
MitchTurner
requested review from
acerone85,
rafal-ch,
rymnc and
AurelienFT
as code owners
| #[cfg(test)] | ||
| mod tests { | ||
| #![allow(non_snake_case)] | ||
| #![allow(clippy::cast_possible_truncation)] |
There was a problem hiding this comment.
Perhaps unrelated to this PR but why do we need this one?
There was a problem hiding this comment.
You're right, it was already here. In general, I am in favor of allowing panickable arithmetic and dangerous casts in tests, for the same reason I'm okay with unwrap in tests.
I'm not sure specifically what this is enabling in the tests, but I'd encourage this pattern.
| /// Seal | ||
| pub signature: Signature, |
There was a problem hiding this comment.
Nit: I'd suggest renaming the signature field to seal or renaming the type to Signed so the naming is more consistent. There's also a "todo" to do the same renaming in the sibling struct in the blockchain/consensus.rs module.
| /// Seal | |
| pub signature: Signature, | |
| /// Seal | |
| pub seal: Signature, |
There was a problem hiding this comment.
I prefer Signature. Seal is ambiguous IMO.
Linked Issues/PRs
Closes #2738
Description
This is part of the greater tx confirmation work. This will enable the block producer to gossip a batch of signed transactions around the network for the senders of those transactions to receive.
In an attempt to cleanup the P2P code, some of the tests were moved to their own files (this makes reviewing slightly more difficult, so I've marked the new tests with comments in those refactored test suites).
TODO:
TxConfirmationstypesignature?txs?Constrain peers for gossiping to minimize trafficAllow nodes to specify with whom they want to gossip Preconfirmations #2740Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]