More intelligent transaction requests

[mrBliss]

UPDATE: new plan described in #1161 (comment)

Old plan:

---

Currently the txSubmissionInbound protocol handler does not look at the Mempool to see if txs are already there before requesting them. So in practice, it will needlessly download txs it already has. More confusingly, we can observe that node A sends a tx to B, which then sends it right back to A.

There are multiple options to improve this situation:

1. Look at the Mempool and don't request txs already in there.
2. Maintain a shared (across all txSubmissionInbound protocol handlers) set of TxIds that were recently in the Mempool. They should also expire from this set, so perhaps a PSQ would be a good fit. This can subsume (1).
3. Keep track of which TxIds we're asking from which peers and don't ask for the same one from multiple peers unless that would otherwise cause things to stall, in which case we should make another request for the same TxId.

Because of the more complex concurrency of (3), we'll keep that for later. Getting (1) working will be easy. Our current aim is to achieve (2).

[edsko]

Moving to 1.1 as it's inessential and we need to cut the release soon.

[intricate]

@dcoutts has also informed me that this PSQ should be shared between all the tx submission handlers, remote and local.

[mrBliss]

While working on #1514, @nfrisby discovered that unlike duplicate txs, duplicate votes are not rejected. Without the improvements described in this issue, the nodes end up resubmitting the votes to each other so that blocks end up containing the same votes many, many times. (I'll use tx to mean generalised transaction, which includes votes)

The approach taken in #1476 (option 2 from the description of this issue) would mitigate this, but not entirely. It helps avoid requesting duplicate txs from other nodes, but it doesn't avoid receiving duplicate txs from the (local) wallet.

A proper solution would be to track in the Mempool which txids it contains and reject incoming txs/votes that are already in the Mempool.

However, this would largely duplicate the txids cache of RecentTxIds.

Therefore, @edsko and I came up with a new approach:

• Remove RecentTxIds.
• Add MempoolSnapshot.hasTx :: GenTxId blk -> Bool, implement it by keeping a Set (GenTxId blk) in the Mempool. (Let it return Maybe (GenTx blk) and keep a Map (GenTxId blk) (GenTx blk)? Would that be useful? YAGNI?)
• Instead of using RecentTxIds to check which txs to request/acknowledge in TxSubmission.Inbound, use MempoolSnapshot.hasTx to do this.
• In Mempool.tryAddTxs, reject txs already in the Mempool. Use the following type for results:

  data AddTxResult blk
    = TxAdded
    | TxRejected (ApplyTxErr blk)
    | TxAlreadyInMempool

We believe this approach is simpler than the one described in (2) (reason about "recent" and time is more complicated), it fixes the duplicate votes problem @nfrisby discovered, it avoids complex interactions in #1496.

The only scenario we believe in which it is worse than option (2):

1. We receive or forge a block containing txs
2. We remove these txs from our mempool
3. Another node receives some of those txs
4. That node informs us about these txs
5. We request them, even though we have requested them in the past, which we could have avoided with RecentTxIds
6. We receive the txs, but the Mempool rejects them, as they are part of our ledger already (except for votes).

I have split off option (3) in #1743.

[mrBliss]

@dcoutts What do you think about the new direction (let's call it option 1+) for this issue and #1743?

[intricate]

It's worth noting that we could mitigate this duplicate votes issue using the RecentTxIds approach by performing a lookup before attempting to add a tx received by the local tx submission server to the mempool. However, I'd say that the new approach is worth pursuing for the reasons mentioned by @mrBliss:

We believe this approach is simpler than the one described in (2) (reason about "recent" and time is more complicated), it fixes the duplicate votes problem @nfrisby discovered, it avoids complex interactions in #1496.

[edsko]

Note that we have to be very cautious: the wallet will monitor the blockchain to verify whether or not transactions that it has submitted are eventually included, and if they aren't, resubmit them. We have to be sure that such resubmitted transactions aren't ignored.