-
Notifications
You must be signed in to change notification settings - Fork 264
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: mempool v1 records which peer it has sent the tx #1089
Conversation
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
[question] do we want to do something similar for the CAT pool around here?
} else { | ||
// record that we have sent the peer the transaction | ||
// to avoid doing it a second time | ||
memTx.SetPeer(peerID) |
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.
lol what a simple change! makes sense to me.
the old mechanism seems so weird! Like, why even both setting the peer if they send you a transaction that you already have if they are setting the peer for original sender? Is the sole purpose to attempt to keep txs in the mempool for longer? are there other tradeoffs?
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 the original idea was to stop the transaction from being sent back to the peer who originally sent it
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 that part makes sense, but this one specifically
celestia-core/mempool/v1/mempool.go
Lines 206 to 212 in fdc1e51
// Check for the transaction in the cache. | |
if !txmp.cache.Push(tx) { | |
// If the cached transaction is also in the pool, record its sender. | |
if elt, ok := txmp.txByKey[txKey]; ok { | |
txmp.metrics.AlreadySeenTxs.Add(1) | |
w := elt.Value.(*WrappedTx) | |
w.SetPeer(txInfo.SenderID) |
why do this at all?
We do do it. See celestia-core/mempool/cat/reactor.go Line 289 in 86e67b7
|
This PR records that we have sent the peer the transaction to avoid doing it a second time
This PR records that we have sent the peer the transaction to avoid doing it a second time
This PR records that we have sent the peer the transaction to avoid doing it a second time