tx relay v2: more improvements

[j-berman]

Overview

We've observed a higher frequency of connection bans (and bad perf) stemming from some issues surrounding tx relay v2. See #373.

2 core issues:

1. The node can take an unexpectedly long time (>30s) to receive the tx from a peer from the point of time it starts the timer on the request.
2. Peers may have added the txs to the chain already (or otherwise don't have them), and so we assume the peer is just missing our requests.

Plus some other bugs contributing to the bans.

This PR aims to eliminate the most severe offending sections of code leading to these concerns.

It also introduces a new field in the p2p tx hash / tx notifier message that both request/response side would be expected to have.

Credit to @selsta and @Boog900 for collaborating on some of the items mentioned in this PR.

This PR's major changes

1. Remove unnecessary locking from the functions that strictly read the db for pool txs (tx_memory_pool::get_transaction and tx_memory_pool::have_tx).

   • This ensures we're not blocked waiting on block or tx verification when handling handle_notify_tx_pool_hash or handle_request_tx_pool_txs.
   • There is no benefit to locking in these functions, since the db could either have or not have them with every call.
   • If the caller needs lock protection, then the caller needs to acquire a lock. Nothing is gained by acquiring these locks in these functions.

2. Don't ban peers if they miss too many tx requests (drop connection, but don't ban).

   • It seems obviously ok to drop connections for very delayed responses, but we're currently banning what look like honest peers.
   • Most of this PR is aimed at reducing connection drops, but I think there is an argument to be made that tx relay v2 could end up causing some harm to honest nodes if we don't solve every issue and do end up banning.
   • There is a DoS concern here, but connection drops are there to mitigate that risk.
   • I'd argue for revisiting banning instead of just plain dropping if we see ~0 honest peers getting dropped because of missed tx requests under sustained stress.

3. Implement a nonce in the p2p tx hash / tx notify messages, so that we can track exact request -> response.

   • We use this to track when a peer doesn't have a tx that it told us it had (it's a more efficient alternative to tx relay v2: tell peer missed requested txs #378).
   • It also enables a much wider array of possibilities to extend tx relay v2 further in the future, and as such, is likely a field we want in the first release of the tx relay v2 protocol.
     • Example 1: keeping track of each peer's response time taking tx sizes into account, thereby enabling improved logic for requesting txs from the best peers.
     • Example 2: penalizing by missed number of requests in addition to, or in place of, penalizing by missed number of individual tx requests.

4. Remove the lock synchronizing handle_notify_new_transactions and the check tx request loop which runs in the idle loop.

   • The lock was a bad design decision on my part.
   • The goal of the lock was to make sure the check_tx_request_queue loop doesn't think a tx is stale while the node is actually still verifying it alongside a larger batch of txs it received, thus kicking a node that actually already responded with the tx.
   • The lock can stall the re-relay checker (and everything else in the idle loop) unnecessarily while handling incoming txs.
     • This has a double effect of potentially stalling re-relaying txs, potentially marginally exacerbating observed double spend errors.
   • The lock can prevent the node from beginning processing newly received txs from other peers, thus adding to the response time that can get the peers kicked.
   • The lock also prevents @jeffro256 's potential parallel processing new incoming txs (cryptonote_core: multi-threaded incoming pool tx handling [FCMP++ beta] #440).
   • We replace the need for a lock by immediately indicating the node is processing the tx request after parsing all the incoming tx blobs, and before handle_incoming_tx.
     • Then we don't consider any processing txs to be "stale" requests in the call to remove_stale_requests.
     • This introduces a somewhat inconvenient API that accepts a parsed tx in handle_incoming_tx which then gets expanded so it's non-const, but the benefit of not locking here seems well worth it. It also avoids locking when parsing, which does seem nice.

5. Request txs from peers as soon as our local capacity to accept more txs from peers becomes available, rather than just in the check_tx_request_queue loop.

   • See fly_available_requests_in_queue.
   • This should improve efficiency in filling our pool.
   • We call this after every call to remove_request.

6. Fix bugged logic in handle_notify_tx_pool_hash that starts the timer while still having more db reads.

   • We don't want to start the timer too early, otherwise it eats into the peer's allotted response time for the tx request.

7. Fix bugged logic in handle_notify_tx_pool_hash that could result in us attempting to request more than the allowed max, and thus not requesting some tx hashes after marking them in-flight.

   • This one could result in dropped connections because we could end up marking many txs as in-flight, then not requesting them at all, then timing out the requests in the check_tx_request_queue loop.

8. Use boost multi-index's .modify() to update elems in the request_manager, rather than updating the iterator in place.

   • This ensures changes propagate across all indexes.
   • The dont_rm_processing_txs_enqueue_overage unit test fails w/o this.
   • Also includes some defense protection in the wrapper call to modify() to handle unexpected failures, see the comment.

9. Align fluff timer flush_time for tx relay v2.

   • context.flush_time could get continuously pushed back even if there are v2 txs sitting in queue waiting to get flushed.

[j-berman]

Depends on monero-project#11065 for a correct max blob size check before parsing tx blobs.

[Boog900]

These changes look great and should help the issues with tx relay v2 a lot, especially removing the locks. I think removing the bans is not great but while tx relay v2 seems unstable it's not a bad idea.

The nonce idea is also a great and cheap way to track requests.

The code looks good.

[Boog900]

I think this needs the arg to ignore the limit that's on the PR here: monero-project#11065

[j-berman]

Yep yep, I'm just going to cherry-pick that change and build on top of it

[Boog900]

I have re-reviewed this PR and it all looks good, I have just a small comment.

Thank you @j-berman

[Boog900]

Do we need this fly_available_requests_in_queue here? AFAICT this will only do something if the m_request_manager.remove_request(tx_hash) above is hit but, as the comment says, that should never really be hit.