-
Notifications
You must be signed in to change notification settings - Fork 705
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
X-chain SDK gossip #2490
X-chain SDK gossip #2490
Conversation
Signed-off-by: Joshua Kim <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Stephen Buttolph <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
d974b7e
to
c58063d
Compare
Signed-off-by: Joshua Kim <[email protected]>
c58063d
to
ac685a0
Compare
} | ||
|
||
func (g *gossipMempool) AddVerified(tx *txs.Tx) error { | ||
if err := g.Mempool.Add(tx); err != nil { |
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.
Should we check if tx
is already in the mempool?
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.
AddVerified
is expected to error if the tx is already in the mempool (which is what happens here)
Co-authored-by: Dhruba Basu <[email protected]> Signed-off-by: Stephen Buttolph <[email protected]>
txID := tx.ID() | ||
n.txPushGossiper.Add(tx) | ||
if err := n.txPushGossiper.Gossip(ctx); err != nil { |
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.
Should we try to better batch these gossip invocations to reduce the number of messages we push (no more than one every x ms)? Or is that handled in the SDK?
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.
AFAICT we would batch if we didn't invoke manually:
avalanchego/network/p2p/gossip/gossip.go
Lines 331 to 347 in ee10054
// Every calls [Gossip] every [frequency] amount of time. | |
func Every(ctx context.Context, log logging.Logger, gossiper Gossiper, frequency time.Duration) { | |
ticker := time.NewTicker(frequency) | |
defer ticker.Stop() | |
for { | |
select { | |
case <-ticker.C: | |
if err := gossiper.Gossip(ctx); err != nil { | |
log.Warn("failed to gossip", zap.Error(err)) | |
} | |
case <-ctx.Done(): | |
log.Debug("shutting down gossip") | |
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.
Yeah - this is kind of an independent suggestion to this PR (as this is the same behavior as before)... But we could play around with batching
if err := n.issueTx(tx); err != nil { | ||
// IssueTx attempts to add a tx to the mempool, after verifying it. If the tx is | ||
// added to the mempool, it will attempt to push gossip the tx to random peers | ||
// in the network using both the legacy and p2p SDK. |
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.
Would it be better to just key off of support based on the handshake?
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.
I think we already get version
in the Connected
message, so it may not be that bad but could be wrong.
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.
We use AppGossip for this, so we don't have the ability to select who exactly we are sending the messages to for push gossip. We could filter the pull gossip to only send it to newer nodes if we cared to optimize this during the time between this release and the network upgrade.
Why this should be merged
This PR introduces p2p SDK based tx gossip to the X-chain. This includes pull and push gossip of transactions (under a new message format) while maintaining compatibility with the prior push gossip mechanism.
How this works
*txs.Tx
implement theGossipable
interfaceAppRequest
,AppResponse
, andAppRequestFailed
messages to the p2p SDK network implementation.AppGossip
messages that can't be parsed with the prior format to the p2p SDK network implementation.How this was tested