Proactively detect and report duplicate gossiped/malformed messages #3839
Conversation
tsachiherman
changed the title
tsachiherman
force-pushed
the
tsachi/add-duplicate-gossip-metrics
branch
from
efb20b5 to
6a2bf8e
Compare
tsachiherman
changed the title
vms/avm/network/gossip.go
Outdated
| @@ -119,7 +118,7 @@ func (g *gossipMempool) Add(tx *txs.Tx) error { | |||
| // Verify the tx at the currently preferred state | |||
| if err := g.txVerifier.VerifyTx(tx); err != nil { | |||
| g.Mempool.MarkDropped(txID, err) | |||
| return err | |||
| return fmt.Errorf("%w: transaction %s verification failed - %s", gossip.ErrGossipableMalformed, txID, err.Error()) | |||
There was a problem hiding this comment.
Wonder if we need a separate error type for malformed vs verification failing
| droppedLabel = "dropped" | ||
|
|
||
| droppedMalformed = "malformed" | ||
| DroppedDuplicate = "duplicate" |
There was a problem hiding this comment.
We don't have to export this label. Beside the code inside the gossip package, we use it in the coreth implementation along with the ObserveIncomingGossipable call. We can definitely define a separate label text ( doesn't have to be duplicate ). As a starting point, I think that keeping it the same is good enough. We can always change that later on if we see that it's unused/unjustified/etc.
network/p2p/gossip/mempool.go
Outdated
|
|
||
| "github.com/ava-labs/avalanchego/ids" | ||
|
|
||
| xmempool "github.com/ava-labs/avalanchego/vms/txs/mempool" |
There was a problem hiding this comment.
We should avoid having a dependency on vms... I think one way to avoid this is to just remove the error type we're importing but if we really wanted to keep an error we should just define it here instead.
There was a problem hiding this comment.
ok, I'll "port" the error type into here.
|
|
||
| // dropped indicate that the gossipable element was not added to the set due to some reason. | ||
| // for sent message, we'll use notReceive below. | ||
| droppedLabel = "dropped" |
There was a problem hiding this comment.
I find the naming for these to be extremely confusing because we use "dropped" even for outbound gossip, for which we can't ever actually drop it (and that's why we have the not_receive label). Without looking at the code I wouldn't understand how to interpret these metric names.
There was a problem hiding this comment.
The alternative is to have sentPushLabels and receivedPushLabels defined as a separate counter vector.
network/p2p/gossip/mempool.go
Outdated
| var ErrDuplicateTx = xmempool.ErrDuplicateTx | ||
|
|
||
| type Mempool[T Gossipable] interface { | ||
| Add(tx T) error |
There was a problem hiding this comment.
I don't mind us changing names to be tx instead of gossipable going forwards, but whatever we do we should be consistent in this diff (inconsistent w/ handleIncomingGossipables). I actually prefer tx naming, the only reason we use gossipables in the current code is because i thought we were going to use this for ip gossip, but we only ended up using this for txs.
There was a problem hiding this comment.
let me name the tx in the gossip package gossipable for now. We can make a change on the next PR. I want to keep the code consistent and make continues improvements.
Co-authored-by: Joshua Kim <[email protected]> Signed-off-by: Tsachi Herman <[email protected]>
Co-authored-by: Joshua Kim <[email protected]> Signed-off-by: Tsachi Herman <[email protected]>
Why this should be merged
This PR modify the way the existing gossip implementation reports dropped incoming transaction.
When reporting the
gossip_countandgossip_bytesfor received messages, an additional label was added :dropped. This label would specify the reason for a transaction to be dropped (malformed/duplicate/other) in case it was dropped. (nototherwise ).How this works
To support the existing avalanchego VMs, a simplified
mempoolwas created. This mempool maintain a map of all the pending transaction, and would label a duplicate transaction as such. The two VMs that are using the gossip,avmandplatformvmwere modified to use this as the backing mempool in addition to the existingvms/txs/mempool.Supporting the duplicate transaction on the coreth proven to be slightly more complicated, due to it's subpool architecture. In order to support both cases, the
gossip.Metricswas extended to allow labeling transaction during themempool.Addexecution.The existing, and updated metrics related logic was moved from
gossip.gointometrics.go. This cleaned up the former from metrics related logic.Related PRs ?
The coreth PR is ava-labs/coreth#935
Need to be documented in RELEASES.md?
No