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(network): PropagateTransactionsTo implementation #4772

Merged

Conversation

SleepingShell
Copy link
Contributor

@SleepingShell SleepingShell commented Sep 25, 2023

One of the requested implementations in #4691

Closes #4691

Unless I am wrong, the logic for both propagate_transactions and propagate_transactions_to_peer should be the same except for the number of peers propagated to. Therefore I moved the implementation to a separate perform_peer_propagation function.

Please let me know if this satisfies the requirement, and if I can continue on implementation for the other commands. I see that another user has implemented the PropagateHashTo command, I would like to implement the others if that has not been claimed.

I would also appreciate any pointers to tests I should be looking at related to this function and other TransactionCommands.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

thanks for this,

I'd like to have this similar to:

/// Propagate the transaction hashes to the given peer
///
/// Note: This will only send the hashes for transactions that exist in the pool.
fn propagate_hashes_to(&mut self, hashes: Vec<TxHash>, peer_id: PeerId) {

as separate function

@mattsse mattsse added C-enhancement New feature or request A-networking Related to networking in general labels Sep 26, 2023
@SleepingShell
Copy link
Contributor Author

thanks for this,

I'd like to have this similar to:

/// Propagate the transaction hashes to the given peer
///
/// Note: This will only send the hashes for transactions that exist in the pool.
fn propagate_hashes_to(&mut self, hashes: Vec<TxHash>, peer_id: PeerId) {

as separate function

Hi, could you provide some more specifics on what this commit is missing that you are looking for? There is a new function propagate_transactions_to_peer, and in addition I moved the logic out of propagate_transactions into a helper function perform_peer_propagation. This latter function does not necessarily need to be inside of the impl block

crates/net/network/src/cache.rs Outdated Show resolved Hide resolved
crates/net/network/src/cache.rs Outdated Show resolved Hide resolved
crates/net/network/src/transactions.rs Outdated Show resolved Hide resolved
crates/net/network/src/transactions.rs Outdated Show resolved Hide resolved
crates/net/network/src/transactions.rs Outdated Show resolved Hide resolved
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
crates/net/network/src/transactions.rs Outdated Show resolved Hide resolved
@mattsse mattsse force-pushed the transactionmanager-implementations branch from 21107f0 to 48bec70 Compare September 30, 2023 09:34
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

awesome tysm for this, I just put some finishing touches on it

@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

Merging #4772 (48bec70) into main (01b264a) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

Files Coverage Δ
crates/net/network/src/cache.rs 94.20% <0.00%> (-4.29%) ⬇️
crates/net/network/src/transactions.rs 52.73% <0.00%> (-4.04%) ⬇️

... and 6 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.51% <0.00%> (-0.02%) ⬇️
unit-tests 62.61% <0.00%> (-0.06%) ⬇️

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

Components Coverage Δ
reth binary 32.28% <ø> (ø)
blockchain tree 80.44% <ø> (ø)
pipeline 88.45% <ø> (ø)
storage (db) 73.31% <ø> (ø)
trie 94.48% <ø> (ø)
txpool 49.49% <ø> (-0.02%) ⬇️
networking 76.13% <0.00%> (-0.32%) ⬇️
rpc 57.80% <ø> (+<0.01%) ⬆️
consensus 61.06% <ø> (ø)
revm 28.53% <ø> (ø)
payload builder 8.16% <ø> (ø)
primitives 85.24% <ø> (-0.03%) ⬇️

@mattsse mattsse added this pull request to the merge queue Sep 30, 2023
Merged via the queue into paradigmxyz:main with commit 05198e9 Sep 30, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement missing logics in on_command function for TransactionsManager
2 participants