Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Tweak propagation of individual transactions in network #6601

Closed
wants to merge 2 commits into from

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Jul 8, 2020

No description provided.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jul 8, 2020
@NikVolf NikVolf added A1-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix B5-clientnoteworthy C3-medium PR touches the given topic and has a medium impact on builders. labels Jul 8, 2020
@bkchr
Copy link
Member

bkchr commented Jul 8, 2020

As already said in Riot, I'm against this solution. Why not create a proper implementation directly?

@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 8, 2020

As already said in Riot, I'm against this solution. Why not create a proper implementation directly?

Why do we need to complicate protocol if this will be enough?

It is used in Ethereum and other networks successfully

@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 8, 2020

Also, this is complementary

If we choose to complicate protocol, we still want to flood to sqrt(count) peers immediately to reduce latency

And announce new transactions hashes to everyone.

Like in this proposal:
ethereum/EIPs#2465

@bkchr
Copy link
Member

bkchr commented Jul 8, 2020

To cater for degenerate topologies, transactions cannot be broadcast logarithmically, rather they need to be transferred linearly to all peers. With N peers, each node will transfer the same transaction N times (counting both directions), whereas 1 would be enough in a perfect world. This is a significant waste.

It says that they don't use logarithmically propagation for transactions.

@NikVolf NikVolf added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. A1-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix labels Jul 8, 2020
@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 8, 2020

It says that they don't use logarithmically propagation for transactions.

They do with referenced eip (it's already live)

But yeah, probably this alone is not enough due to topology issues, I'll add announce/retrival here as well

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

It looks reasonable although I would say that the timer-based propagation should go to sqrt { min: 5 (or even 10) } and the single-transaction should be broadcast to everyone.

Also I think it would be good to implement pool reconciliation protocol at some point soon :)

client/network/src/protocol.rs Outdated Show resolved Hide resolved
@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 8, 2020

@tomusdrw we already have timer-based propagation for every peer on timer (once in 3s)

also see paritytech/polkadot-sdk#553

@tomusdrw
Copy link
Contributor

tomusdrw commented Jul 8, 2020

@tomusdrw we already have timer-based propagation for every peer on timer (once in 3s)

That's why I said I think it should be the other way around. The timer-based propagation (propagate_transactions/0) should be sqrt while newly imported transactions (propagate_transaction/1) can be sent to everyone (since the packets are small) - we might introduce some batching though.

@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 8, 2020

@tomusdrw we already have timer-based propagation for every peer on timer (once in 3s)

That's why I said I think it should be the other way around. The timer-based propagation (propagate_transactions/0) should be sqrt while newly imported transactions can be sent to everyone (since the packets are small) - we might introduce some batching though.

ah, right

@tomaka just mentioned that in new (current) protocol messages are batched automatically, so if you dispatch several messages for particular peer, they'll go batched on wire

@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 8, 2020

The timer-based propagation

@tomusdrw I think with paritytech/polkadot-sdk#553 resolved we can just drop timer-based propagation

FloodSettings::Sqrt { minimum } => (
self.context_data.peers
.iter()
.filter(|(_, peer)| peer.info.roles.is_full())
Copy link
Contributor

Choose a reason for hiding this comment

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

@arkpar was saying, that we have to include peers with authority role into the set for propagation anyway

@gnunicorn
Copy link
Contributor

closing because of inactivity.

@gnunicorn gnunicorn closed this Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. C3-medium PR touches the given topic and has a medium impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants