Merge bitcoin/bitcoin#34615: mempool: expose optimality of mempool to log / rpc

a9e59f7 rpc: add optimal result to getmempoolinfo (Greg Sanders)
a3fb3dd mempool: log if we detect a non-optimal mempool (Greg Sanders)

Pull request description:

  Post-SFL #34023 I don't think we expect the mempool to be unordered for long periods of time. If we consider it likely to be a serious regression in production, it would be useful to expose the fact  that the mempool is not known to be optimal.

  1. do a MEMPOOL log after any `DoWork()` returns false, meaning non-optimal
  2. expose it via getmempoolinfo, by calling `DoWork(0)`, which does nothing but return known-optimality

  I'm not wedded to either approach, I just think something is better than nothing for the next release.

ACKs for top commit:
  ajtowns:
    ACK a9e59f7
  ismaelsadeeq:
    reACK a9e59f7 [c89b93b95..a9e59f7](https://github.com/bitcoin/bitcoin/compare/c89b93b9584f09401f9740061fdb6e4036f6dbbf..a9e59f7d955f995078b3e0bf3b527c03c74fef8d) fixed typo, added more logging for block/reorg additions to mempool, and fixed brittle test case.
  sedited:
    ACK a9e59f7
  sipa:
    ACK a9e59f7

Tree-SHA512: 1560ad21cc1606df7279c102f35f61d4555c0ac920f02208b2a6eb89b14d7e22befb6d7f510a00a9074c2f9931f32e9af86bcea3a8dd9a1d947b0398c84666dd

Author: sedited

src/rpc/mempool.cpp

@@ -1050,6 +1050,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
     ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
     ret.pushKV("limitclustercount", pool.m_opts.limits.cluster_count);
     ret.pushKV("limitclustersize", pool.m_opts.limits.cluster_size_vbytes);
+    ret.pushKV("optimal", pool.m_txgraph->DoWork(0)); // 0 work is a quick check for known optimality
     return ret;
 }
 
@@ -1076,6 +1077,7 @@ static RPCHelpMan getmempoolinfo()
                 {RPCResult::Type::NUM, "maxdatacarriersize", "Maximum number of bytes that can be used by OP_RETURN outputs in the mempool"},
                 {RPCResult::Type::NUM, "limitclustercount", "Maximum number of transactions that can be in a cluster (configured by -limitclustercount)"},
                 {RPCResult::Type::NUM, "limitclustersize", "Maximum size of a cluster in virtual bytes (configured by -limitclustersize)"},
+                {RPCResult::Type::BOOL, "optimal", "If the mempool is in a known-optimal transaction ordering"},
             }},
         RPCExamples{
             HelpExampleCli("getmempoolinfo", "")

src/txmempool.cpp

@@ -221,7 +221,9 @@ void CTxMemPool::Apply(ChangeSet* changeset)
 
         addNewTransaction(it);
     }
-    m_txgraph->DoWork(POST_CHANGE_WORK);
+    if (!m_txgraph->DoWork(POST_CHANGE_WORK)) {
+        LogDebug(BCLog::MEMPOOL, "Mempool in non-optimal ordering after addition(s).");
+    }
 }
 
 void CTxMemPool::addNewTransaction(CTxMemPool::txiter newit)
@@ -378,7 +380,9 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check
     for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) {
         assert(TestLockPointValidity(chain, it->GetLockPoints()));
     }
-    m_txgraph->DoWork(POST_CHANGE_WORK);
+    if (!m_txgraph->DoWork(POST_CHANGE_WORK)) {
+        LogDebug(BCLog::MEMPOOL, "Mempool in non-optimal ordering after reorg.");
+    }
 }
 
 void CTxMemPool::removeConflicts(const CTransaction &tx)
@@ -421,7 +425,9 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
     }
     lastRollingFeeUpdate = GetTime();
     blockSinceLastRollingFeeBump = true;
-    m_txgraph->DoWork(POST_CHANGE_WORK);
+    if (!m_txgraph->DoWork(POST_CHANGE_WORK)) {
+        LogDebug(BCLog::MEMPOOL, "Mempool in non-optimal ordering after block.");
+    }
 }
 
 void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendheight) const

test/functional/mempool_cluster.py

@@ -306,6 +306,9 @@ def test_getmempoolcluster(self):
 
         assert_equal(node.getrawmempool(), [])
 
+        # Key should exist and be trivially optimal
+        assert node.getmempoolinfo()["optimal"]
+
         # Not in-mempool
         not_mempool_tx = self.wallet.create_self_transfer()
         assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolcluster, not_mempool_tx["txid"])
@@ -367,6 +370,9 @@ def test_getmempoolcluster(self):
         chunkfee = first_chunk_tx["fee"] + second_chunk_tx["fee"] + third_chunk_tx["fee"]
         assert_equal(first_chunk_info, {'clusterweight': first_chunkweight + second_chunkweight + third_chunkweight, 'txcount': 3, 'chunks': [{'chunkfee': first_chunk_tx["fee"], 'chunkweight': first_chunkweight, 'txs': [first_chunk_tx["txid"]]}, {'chunkfee': second_chunk_tx["fee"], 'chunkweight': second_chunkweight, 'txs': [second_chunk_tx["txid"]]}, {'chunkfee': third_chunk_tx["fee"], 'chunkweight': third_chunkweight, 'txs': [third_chunk_tx["txid"]]}]})
 
+        # We expect known optimality directly after txn submission
+        assert node.getmempoolinfo()["optimal"]
+
         # If we prioritise the last transaction it can join the second transaction's chunk.
         node.prioritisetransaction(third_chunk_tx["txid"], 0, int(third_chunk_tx["fee"]*COIN) + 1)
         first_chunk_info = node.getmempoolcluster(first_chunk_tx["txid"])