Merge #15531: Suggested interfaces::Chain cleanups from #15288

4d4e4c6 Suggested interfaces::Chain cleanups from #15288 (Russell Yanofsky)

Pull request description:

  Mostly documentation improvements requested in the last review of #15288 before it was merged (bitcoin/bitcoin#15288 (review))

Tree-SHA512: 64e912520bbec20a44032f265a8cf3f11ad7f5126c8626b5ad5e888227b1f92ecb321522fab4bbbd613230b55450abd6ace023631d0a4f357a780d65c5638bfe

Author: MarcoFalke

src/interfaces/README.md

@@ -2,9 +2,9 @@
 
 The following interfaces are defined here:
 
-* [`Chain`](chain.h) — used by wallet to access blockchain and mempool state. Added in [#10973](https://github.com/bitcoin/bitcoin/pull/10973).
+* [`Chain`](chain.h) — used by wallet to access blockchain and mempool state. Added in [#14437](https://github.com/bitcoin/bitcoin/pull/14437), [#14711](https://github.com/bitcoin/bitcoin/pull/14711), [#15288](https://github.com/bitcoin/bitcoin/pull/15288), and [#10973](https://github.com/bitcoin/bitcoin/pull/10973).
 
-* [`ChainClient`](chain.h) — used by node to start & stop `Chain` clients. Added in [#10973](https://github.com/bitcoin/bitcoin/pull/10973).
+* [`ChainClient`](chain.h) — used by node to start & stop `Chain` clients. Added in [#14437](https://github.com/bitcoin/bitcoin/pull/14437).
 
 * [`Node`](node.h) — used by GUI to start & stop bitcoin node. Added in [#10244](https://github.com/bitcoin/bitcoin/pull/10244).
 

src/interfaces/chain.cpp

@@ -148,7 +148,7 @@ class LockImpl : public Chain::Lock
         LockAnnotation lock(::cs_main);
         return CheckFinalTx(tx);
     }
-    bool submitToMemoryPool(CTransactionRef tx, CAmount absurd_fee, CValidationState& state) override
+    bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) override
     {
         LockAnnotation lock(::cs_main);
         return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */,
@@ -207,8 +207,8 @@ class ChainImpl : public Chain
     bool hasDescendantsInMempool(const uint256& txid) override
     {
         LOCK(::mempool.cs);
-        auto it_mp = ::mempool.mapTx.find(txid);
-        return it_mp != ::mempool.mapTx.end() && it_mp->GetCountWithDescendants() > 1;
+        auto it = ::mempool.GetIter(txid);
+        return it && (*it)->GetCountWithDescendants() > 1;
     }
     void relayTransaction(const uint256& txid) override
     {
@@ -219,7 +219,7 @@ class ChainImpl : public Chain
     {
         ::mempool.GetTransactionAncestry(txid, ancestors, descendants);
     }
-    bool checkChainLimits(CTransactionRef tx) override
+    bool checkChainLimits(const CTransactionRef& tx) override
     {
         LockPoints lp;
         CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp);

src/interfaces/chain.h

@@ -6,7 +6,6 @@
 #define BITCOIN_INTERFACES_CHAIN_H
 
 #include <optional.h>               // For Optional and nullopt
-#include <policy/rbf.h>             // For RBFTransactionState
 #include <primitives/transaction.h> // For CTransactionRef
 
 #include <memory>
@@ -16,17 +15,39 @@
 #include <vector>
 
 class CBlock;
+class CFeeRate;
 class CScheduler;
 class CValidationState;
 class uint256;
+enum class RBFTransactionState;
 struct CBlockLocator;
 struct FeeCalculation;
 
 namespace interfaces {
 
 class Wallet;
 
-//! Interface for giving wallet processes access to blockchain state.
+//! Interface giving clients (wallet processes, maybe other analysis tools in
+//! the future) ability to access to the chain state, receive notifications,
+//! estimate fees, and submit transactions.
+//!
+//! TODO: Current chain methods are too low level, exposing too much of the
+//! internal workings of the bitcoin node, and not being very convenient to use.
+//! Chain methods should be cleaned up and simplified over time. Examples:
+//!
+//! * The Chain::lock() method, which lets clients delay chain tip updates
+//!   should be removed when clients are able to respond to updates
+//!   asynchronously
+//!   (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269).
+//!
+//! * The relayTransactions() and submitToMemoryPool() methods could be replaced
+//!   with a higher-level broadcastTransaction method
+//!   (https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984).
+//!
+//! * The initMessages() and loadWallet() methods which the wallet uses to send
+//!   notifications to the GUI should go away when GUI and wallet can directly
+//!   communicate with each other without going through the node
+//!   (https://github.com/bitcoin/bitcoin/pull/15288#discussion_r253321096).
 class Chain
 {
 public:
@@ -114,8 +135,9 @@ class Chain
         virtual bool checkFinalTx(const CTransaction& tx) = 0;
 
         //! Add transaction to memory pool if the transaction fee is below the
-        //! amount specified by absurd_fee (as a safeguard). */
-        virtual bool submitToMemoryPool(CTransactionRef tx, CAmount absurd_fee, CValidationState& state) = 0;
+        //! amount specified by absurd_fee. Returns false if the transaction
+        //! could not be added due to the fee or for another reason.
+        virtual bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) = 0;
     };
 
     //! Return Lock interface. Chain is locked when this is called, and
@@ -154,19 +176,19 @@ class Chain
     //! Calculate mempool ancestor and descendant counts for the given transaction.
     virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) = 0;
 
-    //! Check chain limits.
-    virtual bool checkChainLimits(CTransactionRef tx) = 0;
+    //! Check if transaction will pass the mempool's chain limits.
+    virtual bool checkChainLimits(const CTransactionRef& tx) = 0;
 
     //! Estimate smart fee.
     virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc = nullptr) = 0;
 
     //! Fee estimator max target.
     virtual unsigned int estimateMaxBlocks() = 0;
 
-    //! Pool min fee.
+    //! Mempool minimum fee.
     virtual CFeeRate mempoolMinFee() = 0;
 
-    //! Get node max tx fee setting (-maxtxfee).
+    //! Node max tx fee setting (-maxtxfee).
     //! This could be replaced by a per-wallet max fee, as proposed at
     //! https://github.com/bitcoin/bitcoin/issues/15355
     //! But for the time being, wallets call this to access the node setting.