Merge bitcoin/bitcoin#34635: rpc, index: txospenderindex improve formatting, docs and test coverage

15c4889497b96037e41019a8f43090af841b36ec index: document TxoSpenderIndex::FindSpender (furszy)
f8b9595aaa966c373b02e6227dc799fed6d038ba test: Add missing txospenderindex coverage in feature_init (Fabian Jahr)
a1074d852a7a46b746fb4ed90d94cb4cc346f9b3 index, rpc, test: Misc formatting fixes (Fabian Jahr)

Pull request description:

  This addresses my own comments in the last review of #24539: https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-3829110465

  The first commit fixes three small formatting errors.

  The second commit adds some missing coverage in `feature_init` and refactors the code a bit as well so these misses don't happen so easily in the future.

  The third commit is by furzy:

  > TxoSpenderIndex::FindSpender returns an Expected<optional<TxoSpender>> but
  the two levels of the return type were undocumented, making it unclear what a returned
  nullopt means. So added doc clarifying each return case.

ACKs for top commit:
  furszy:
    ACK 15c4889497b96037e41019a8f43090af841b36ec
  sedited:
    ACK 15c4889497b96037e41019a8f43090af841b36ec
  rkrux:
    crACK 15c4889497b96037e41019a8f43090af841b36ec

Tree-SHA512: 2e0f060a54b558d2967ebae0835cf81bd86c2d8d983d670a48d1bee7d347f186623e75db7ae311ca1566807f715c1b3fa67cf734c9467d35e13b84d082f28253

src/index/txospenderindex.h

@@ -1,4 +1,3 @@
-
 // Copyright (c) The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
@@ -56,6 +55,16 @@ protected:
 public:
     explicit TxoSpenderIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
 
+    /**
+     * Search the index for a transaction that spends the given outpoint.
+     *
+     * @param[in] txo  The outpoint to search for.
+     *
+     * @return  std::nullopt               if the outpoint has not been spent on-chain.
+     *          std::optional{TxoSpender}  if the output has been spent on-chain. Contains the spending transaction
+     *                                     and the block it was confirmed in.
+     *          util::Unexpected{error}    if something unexpected happened (i.e. disk or deserialization error).
+     */
     util::Expected<std::optional<TxoSpender>, std::string> FindSpender(const COutPoint& txo) const;
 };
 

src/rpc/mempool.cpp

@@ -940,7 +940,7 @@ static RPCHelpMan gettxspendingprevout()
             if (output_params.empty()) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, outputs are missing");
             }
-            const UniValue options{request.params[1].isNull() ? UniValue::VOBJ : request.params[1]};\
+            const UniValue options{request.params[1].isNull() ? UniValue::VOBJ : request.params[1]};
             RPCTypeCheckObj(options,
                             {
                                 {"mempool_only", UniValueType(UniValue::VBOOL)},

test/functional/feature_init.py

@@ -18,6 +18,12 @@ from test_framework.test_node import (
 )
 from test_framework.util import assert_equal
 
+ALL_INDEX_ARGS = [
+    '-txindex=1',
+    '-blockfilterindex=1',
+    '-coinstatsindex=1',
+    '-txospenderindex=1',
+]
 
 class InitTest(BitcoinTestFramework):
     """
@@ -74,6 +80,7 @@ class InitTest(BitcoinTestFramework):
             b'txindex thread start',
             b'block filter index thread start',
             b'coinstatsindex thread start',
+            b'txospenderindex thread start',
             b'msghand thread start',
             b'net thread start',
             b'addcon thread start',
@@ -81,22 +88,21 @@ class InitTest(BitcoinTestFramework):
         if self.is_wallet_compiled():
             lines_to_terminate_after.append(b'Verifying wallet')
 
-        args = ['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1', '-txospenderindex=1']
         for terminate_line in lines_to_terminate_after:
             self.log.info(f"Starting node and will terminate after line {terminate_line}")
             with node.busy_wait_for_debug_log([terminate_line]):
                 if platform.system() == 'Windows':
                     # CREATE_NEW_PROCESS_GROUP is required in order to be able
                     # to terminate the child without terminating the test.
-                    node.start(extra_args=args, creationflags=subprocess.CREATE_NEW_PROCESS_GROUP)
+                    node.start(extra_args=ALL_INDEX_ARGS, creationflags=subprocess.CREATE_NEW_PROCESS_GROUP)
                 else:
-                    node.start(extra_args=args)
+                    node.start(extra_args=ALL_INDEX_ARGS)
             self.log.debug("Terminating node after terminate line was found")
             sigterm_node()
 
         # Prior to deleting/perturbing index files, start node with all indexes enabled.
         # 'check_clean_start' will ensure indexes are synchronized (i.e., data exists to modify)
-        self.check_clean_start(node, args)
+        self.check_clean_start(node, ALL_INDEX_ARGS)
         self.stop_node(0)
 
     def init_stress_test_removals(self):
@@ -105,7 +111,6 @@ class InitTest(BitcoinTestFramework):
         """
         self.log.info("Test startup errors after removing certain essential files")
         node = self.nodes[0]
-        args = ['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1']
 
         def start_expecting_error(err_fragment, args):
             node.assert_start_raises_init_error(
@@ -209,7 +214,7 @@ class InitTest(BitcoinTestFramework):
                 self.log.debug(f"Restoring file from {bak_path} and restarting")
                 Path(bak_path).rename(target_file)
 
-            self.check_clean_start(node, args)
+            self.check_clean_start(node, ALL_INDEX_ARGS)
             self.stop_node(0)
 
         self.log.info("Test startup errors after perturbing certain essential files")
@@ -313,7 +318,7 @@ class InitTest(BitcoinTestFramework):
         self.log.info("Test that stopping and restarting a node that has done nothing is not causing a failure")
         options = [
             [],
-            ["-txindex=1", "-blockfilterindex=1", "-coinstatsindex=1"],
+            ALL_INDEX_ARGS,
         ]
         for option in options:
             self.restart_node(1, option)

test/functional/rpc_misc.py

@@ -111,7 +111,7 @@ class RpcMiscTest(BitcoinTestFramework):
                 "txindex": values,
                 "basic block filter index": values,
                 "coinstatsindex": values,
-                "txospenderindex": values
+                "txospenderindex": values,
             }
         )
         # Specifying an index by name returns only the status of that index