-
Notifications
You must be signed in to change notification settings - Fork 20.5k
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
eth: propagate blocks and transactions async #16769
Conversation
// maxQueuedProps is the maximum number of block propagations to queue up before | ||
// dropping broadcasts. There's not much point in queueing stale blocks, so a few | ||
// that might cover uncles should be enough. | ||
maxQueuedProps = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might work for mainnet, but what will happen on e..g ropsten when there are large reorgs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are propagations. I.e. when a new block is announced. On Ropsten, if a huge reorg happens, the node will realize it's missing blocks and start a sync cycle.
// peer. If the peer's broadcast queue is full, the event is silently dropped. | ||
func (p *peer) AsyncSendTransactions(txs []*types.Transaction) { | ||
select { | ||
case p.queuedTxs <- txs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The limit of 128 transactions on the channel can get full pretty quickly, no? Won't that mean that we'll be stuck here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a default
clause that drops anything that doesn't fit.
|
||
case block := <-p.queuedAnns: | ||
if err := p.SendNewBlockHashes([]common.Hash{block.Hash()}, []uint64{block.NumberU64()}); err != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's an error somehwere here, we'll kill the broadcast loop, but will the peer correctly become unregistered? Otherwise, we'll just keep trying to feed stuff into channels that nobody is reading from.. (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's an error with sending, it means the connection dropped. Other parts of the system should detect it too and drop the peer eventually. In the short interim, the channels might buffer some messages, but they won't hang since overflows are silently dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Does it affect miner nodes e.g miner hashrate dropped? |
No, this does not affect miners. What this change does it drops transaction announcement to peers that are overloaded and cannot seem to process the network packets fast enough. |
Up until now, block propagations, block announcements and transaction broadcasts have been all done synchronously. This meant that a slow peer could starve fast ones, introduce blockages inside the event system and also could pile up goroutines causing memory issues.
This PR moves broadcasts into their separate goroutines (1 per peer), to ensure peers cannot interfere with each other, and also introduces a buffered channel to pile events up, which drop packages on overflow. Dropping announcements is fine here, since the goal is to get the "stuff" out into the network, not strict communication between two peers.