Skip to content

Conversation

@sstone
Copy link
Contributor

@sstone sstone commented Mar 11, 2022

This PR adds a new "tx output spender" index, which allows users to query which tx spent a given outpoint with the gettxspendingprevout RPC call that was added by #24408.

Such an index would be extremely useful for Lightning, and probably for most layer-2 protocols that rely on chains of unpublished transactions.

UPDATE: this PR is ready for review and issues have been addressed:

We use a composite key with 2 parts (suggested by @romanz): hash(spent outpoint) and tx position, with an empty value. Average composite key size is 15 bytes.

The spending tx can optionally be returned by gettxspendingprevout (even it -txindex is not set).

@sstone sstone force-pushed the add-txospender-index branch from 3f7cdb6 to aa9f963 Compare March 11, 2022 21:08
@ryanofsky
Copy link
Contributor

Concept ACK. It would be good know more about specific use-cases for this, if you can provide some links or more description. But the implementation seems very simple, so I don't think adding this would be a maintenance burden.

@maflcko
Copy link
Member

maflcko commented Mar 12, 2022

@ ryan, I think the idea is that there is one coin that is owned by more than one entity. There may be several (conflicting) pre-signed txs spending it and as soon as it is spent, all entities with ownership in it want to check which pre-signed tx spent the coin.
If they use Bitcoin Core to monitor the coin, they may use:

  • A watch-only wallet (which requires a file on disk and BDB/Sqlite)
  • Walk the raw chain and raw mempool themselves (According to rpc: add rpc to get mempool txs spending specific prevouts #24408 (comment) this is doable for the chain, but may not be for the mempool)
  • Use an index. A blockfilterindex might speed up scanning the chain, whereas this index directly gives the answer.

@sstone
Copy link
Contributor Author

sstone commented Mar 12, 2022

Lightning channels are basically trees of unpublished transactions:

  • a commit tx that spends a shared onchain "funding" UTXO
  • HTLC txs that spend the commit tx (there can be competing HTLC txs that spend the same commit tx output: one for successful payments, one for timed out payments)

There is also a penalty scheme built into these txs: if you cheat, you need to wait before you can spend your outputs but your channel peer can spend them immediately.

And LN is also transitioning to a model where these txs pay minimum fees and can be "bumped" with RBF/CPFP.

=> it's very useful for LN nodes to detect when transactions have been spent, but also how they've been spent and react accordingly (by extracting info from the spending txs for example), and walk up chains of txs that spend each other.

As described above, there are tools in bitcoin core that we could use and would help a bit, and Lightning nodes could also create their own indexes independently or use electrum servers, block explorers, but this would make running LN nodes connected to your own bitcoin node much easier.

@michaelfolkson
Copy link

Concept ACK

I think the use case is clear and if others think the maintenance burden is reasonable there doesn't seem to be an obvious downside.

This PR can be viewed as an "extension" to #24408

Seems like review should be focused on #24408 first but assuming that gets merged, Concept ACK for this.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 12, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/24539.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK sedited
Concept ACK ryanofsky, michaelfolkson, mzumsande, glozow, aureleoules, achow101, hodlinator, willcl-ark, fjahr
Approach ACK w0xlt

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK - I can see why this could be helpful for lightning, the code is very similar to the txindex code.

Some general thoughts:

  • getmempooltxspendingprevout in #24408 takes a list of outpoints, gettxospender takes a single outpoint. Would it make sense to have the same format here (I'd guess a common use case would be to check both RPCs with the same outpoint(s)?)
  • Taking this one step further, getrawtransaction queries transaction data from the mempool or from blocks (if txindex is enabled) in one RPC. Would it make sense to have something similar for outpoints, instead of two separate RPCs?
  • I think that the indexed data will not be deleted in case of a reorg (but possibly overwritten if an outpoint would be spent in a different tx as a result of a reorg). That means that a result from gettxospender could be stale. Would it make sense to delete entries or should we at least mention this possibility in the rpc doc?
  • What's the reason for the dependency on -txindex? As far as I can see it's not technical, so would knowing the fact that an outpoint was used in a tx (even if we can't lookup the details of that tx further) be sufficient for some use cases?

@sstone sstone force-pushed the add-txospender-index branch from aa9f963 to 1b82b7e Compare March 14, 2022 10:27
@sstone
Copy link
Contributor Author

sstone commented Mar 14, 2022

* `getmempooltxspendingprevout` in #24408 takes a list of outpoints, `gettxospender` takes a single outpoint. Would it make sense to have the same format here (I'd guess a common use case would be to check both RPCs with the same outpoint(s)?)

Bitcoin supports batched RPC request so from a functional p.o.v it's equivalent to passing a list of outpoints but it's a bit easier to reason about imho.
I think that the use case for getmempooltxspendingprevout is a bit different: you want to bump fees for a set of txs that are related to the same channel and want to understand who is spending what. You may also have txs with multiple inputs, and would pass them all together to getmempooltxspendingprevout.

For gettxospender the use case is trying to understand why channels have been closed and react if needed, if we could pass in multiple outpoints they would likely be unrelated.

* Taking this one step further, `getrawtransaction` queries transaction data from the mempool or from blocks (if txindex is enabled) in one RPC. Would it make sense to have something similar for outpoints, instead of two separate RPCs?

Maybe, let's see where the discussion in #24408 but to me it feels cleaner to have separate calls.

* I think that the indexed data will not be deleted in case of a reorg (but possibly overwritten if an outpoint would be spent in a different tx as a result of a reorg). That means that a result from `gettxospender` could be stale. Would it make sense to delete entries or should we at least mention this possibility in the rpc doc?

* What's the reason for the dependency on -txindex? As far as I can see it's not technical, so would knowing the fact that an outpoint was used in a tx (even if we can't lookup the details of that tx further) be sufficient for some use cases?

We would almost always want to get the actual tx that spends our outpoint. My reasoning was that relying on -txindex makes gettxospender robust and consistent in the case of reorgs and is also simpler and easier to understand that storing block positions instead of txids.

@glozow
Copy link
Member

glozow commented Apr 1, 2022

Concept ACK, nice

@sstone
Copy link
Contributor Author

sstone commented Apr 25, 2022

Rebased on #24408

@achow101
Copy link
Member

To compute the short channel Id we simply call getrawtransaction which gives us the block hash for the block that confirmed the tx, then load that block to get the tx position in that block.

So you need txindex and make 3 RPC calls? ISTM storing the position in this new index would make it much more efficient and more useful to other people.

@sstone
Copy link
Contributor Author

sstone commented Nov 19, 2025

To compute the short channel Id we simply call getrawtransaction which gives us the block hash for the block that confirmed the tx, then load that block to get the tx position in that block.

So you need txindex and make 3 RPC calls? ISTM storing the position in this new index would make it much more efficient and more useful to other people.

I could easily return the block hash along with the spending tx, txindex would not be needed anymore but users would still need to call getblock.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task test each commit: https://github.com/bitcoin/bitcoin/actions/runs/19615986599/job/56168637751
LLM reason (✨ experimental): Compilation failed due to incorrect return types in src/index/txospenderindex.cpp (static_cast to bool and returning void), leading to a build error.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@sstone sstone force-pushed the add-txospender-index branch 3 times, most recently from bb0cb11 to c7ba5de Compare November 24, 2025 09:46
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block hash addition looks good to me. Just needs a small iwyu fix to get the CI to pass again.

private:
std::unique_ptr<BaseIndex::DB> m_db;
std::pair<uint64_t, uint64_t> m_siphash_key;
bool AllowPrune() const override { return true; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the index stores the tx position in disk, how is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed in 38f2f53

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. It would be good to add a test case for it.

@sstone sstone force-pushed the add-txospender-index branch 2 times, most recently from acf4fba to 08167df Compare November 24, 2025 16:30
Adds an outpoint -> txid index, which can be used to find which transactions spent a given output.
We use a composite key with 2 parts (suggested by @romanz): hash(spent outpoint) and tx position, with an empty value.
To find the spending tx for a given outpoint, we do a prefix search (prefix being the hash of the provided outpoint), and for all keys that match this prefix
we load the tx at the position specified in the key and return it, along with the block hash, if does spend the provided outpoint.
To handle reorgs we just erase the keys computed from the removed block.

This index is extremely useful for Lightning and more generally for layer-2 protocols that rely on chains of unpublished transactions.
If enabled, this index will be used by `gettxspendingprevout` when it does not find a spending transaction in the mempool.
@sstone sstone force-pushed the add-txospender-index branch from 08167df to 38f2f53 Compare November 24, 2025 16:51
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 38f2f53

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The gettxspendingprevout RPC description needs to be updated.
For example

-        "Scans the mempool to find transactions spending any of the given outputs",
+        "Scans the mempool (and the txospenderindex, if available) to find transactions spending any of the given outputs",

@DrahtBot DrahtBot requested review from aureleoules and w0xlt January 14, 2026 02:05
Copy link
Contributor

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 38f2f53

Prefix-scanning in LevelDB as @romanz suggested in #24539 (comment) does indeed simplify the code. With the DBKey including a full CDiskTxPos, we can be sure any hash collisions are safe.

Comment on lines +51 to +52
// Destroys unique_ptr to an incomplete type.
virtual ~TxoSpenderIndex() override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed since we no longer have a custom DB implementation.

Suggested change
// Destroys unique_ptr to an incomplete type.
virtual ~TxoSpenderIndex() override;

m_db->WriteBatch(batch);
}

std::vector<std::pair<COutPoint, CDiskTxPos>> BuildSpenderPositions(const interfaces::BlockInfo& block)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Only used in this file:

Suggested change
std::vector<std::pair<COutPoint, CDiskTxPos>> BuildSpenderPositions(const interfaces::BlockInfo& block)
static std::vector<std::pair<COutPoint, CDiskTxPos>> BuildSpenderPositions(const interfaces::BlockInfo& block)

Comment on lines +144 to +145
EraseSpenderInfos(BuildSpenderPositions(block));
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Odd indentation:

Suggested change
EraseSpenderInfos(BuildSpenderPositions(block));
return true;
EraseSpenderInfos(BuildSpenderPositions(block));
return true;

Comment on lines +64 to +69
: BaseIndex(std::move(chain), "txospenderindex")
{
fs::path path{gArgs.GetDataDirNet() / "indexes" / "txospenderindex"};
fs::create_directories(path);

m_db = std::make_unique<TxoSpenderIndex::DB>(path / "db", n_cache_size, f_memory, f_wipe);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: There's no need to explicitly call create_directories as that is done internally already:

TryCreateDirectories(params.path);

Suggested change
: BaseIndex(std::move(chain), "txospenderindex")
{
fs::path path{gArgs.GetDataDirNet() / "indexes" / "txospenderindex"};
fs::create_directories(path);
m_db = std::make_unique<TxoSpenderIndex::DB>(path / "db", n_cache_size, f_memory, f_wipe);
: BaseIndex(std::move(chain), "txospenderindex"),
m_db{std::make_unique<DB>(gArgs.GetDataDirNet() / "indexes" / "txospenderindex" / "db", n_cache_size, f_memory, f_wipe)}
{

(Also don't need the full TxoSpenderIndex::DB).


bool TxoSpenderIndex::FindSpender(const COutPoint& txo, CTransactionRef& tx, uint256& block_hash) const
{
uint64_t prefix = CreateKeyPrefix(m_siphash_key, txo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
uint64_t prefix = CreateKeyPrefix(m_siphash_key, txo);
const uint64_t prefix{CreateKeyPrefix(m_siphash_key, txo)};

// Destroys unique_ptr to an incomplete type.
virtual ~TxoSpenderIndex() override;

bool FindSpender(const COutPoint& txo, CTransactionRef& tx, uint256& block_hash) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could go back to returning optional again:

Suggested change
bool FindSpender(const COutPoint& txo, CTransactionRef& tx, uint256& block_hash) const;
std::optional<Spender> FindSpender(const COutPoint& txo) const;
Full diff
diff --git a/src/index/txospenderindex.cpp b/src/index/txospenderindex.cpp
index 10c104ab14..b120bb804c 100644
--- a/src/index/txospenderindex.cpp
+++ b/src/index/txospenderindex.cpp
@@ -145,26 +145,26 @@ bool TxoSpenderIndex::CustomRemove(const interfaces::BlockInfo& block)
      return true;
 }
 
-bool TxoSpenderIndex::ReadTransaction(const CDiskTxPos& tx_pos, CTransactionRef& tx, uint256& block_hash) const
+std::optional<TxoSpenderIndex::Spender> TxoSpenderIndex::ReadTransaction(const CDiskTxPos& tx_pos) const
 {
     AutoFile file{m_chainstate->m_blockman.OpenBlockFile(tx_pos, true)};
     if (file.IsNull()) {
-        return false;
+        return std::nullopt;
     }
     CBlockHeader header;
     try {
         file >> header;
         file.seek(tx_pos.nTxOffset, SEEK_CUR);
+        CTransactionRef tx;
         file >> TX_WITH_WITNESS(tx);
-        block_hash = header.GetHash();
-        return true;
+        return Spender{tx, header.GetHash()};
     } catch (const std::exception& e) {
         LogError("Deserialize or I/O error - %s\n", e.what());
-        return false;
+        return std::nullopt;
     }
 }
 
-bool TxoSpenderIndex::FindSpender(const COutPoint& txo, CTransactionRef& tx, uint256& block_hash) const
+std::optional<TxoSpenderIndex::Spender> TxoSpenderIndex::FindSpender(const COutPoint& txo) const
 {
     uint64_t prefix = CreateKeyPrefix(m_siphash_key, txo);
     std::unique_ptr<CDBIterator> it(m_db->NewIterator());
@@ -173,15 +173,15 @@ bool TxoSpenderIndex::FindSpender(const COutPoint& txo, CTransactionRef& tx, uin
     // find all keys that start with the outpoint hash, load the transaction at the location specified in the key
     // and return it if it does spend the provided outpoint
     for (it->Seek(std::pair{DB_TXOSPENDERINDEX, prefix}); it->Valid() && it->GetKey(key) && key.hash == prefix; it->Next()) {
-        if (ReadTransaction(key.pos, tx, block_hash)) {
-            for (const auto& input : tx->vin) {
+        if (auto res{ReadTransaction(key.pos)}) {
+            for (const auto& input : res->tx->vin) {
                 if (input.prevout == txo) {
-                    return true;
+                    return res;
                 }
             }
         }
     }
-    return false;
+    return std::nullopt;
 }
 
 BaseIndex::DB& TxoSpenderIndex::GetDB() const { return *m_db; }
diff --git a/src/index/txospenderindex.h b/src/index/txospenderindex.h
index 908774d9ef..9324d7cfc0 100644
--- a/src/index/txospenderindex.h
+++ b/src/index/txospenderindex.h
@@ -28,13 +28,19 @@ static constexpr bool DEFAULT_TXOSPENDERINDEX{false};
  */
 class TxoSpenderIndex final : public BaseIndex
 {
+public:
+    struct Spender {
+        CTransactionRef tx;
+        uint256 block_hash;
+    };
+
 private:
     std::unique_ptr<BaseIndex::DB> m_db;
     std::pair<uint64_t, uint64_t> m_siphash_key;
     bool AllowPrune() const override { return false; }
     void WriteSpenderInfos(const std::vector<std::pair<COutPoint, CDiskTxPos>>& items);
     void EraseSpenderInfos(const std::vector<std::pair<COutPoint, CDiskTxPos>>& items);
-    bool ReadTransaction(const CDiskTxPos& pos, CTransactionRef& tx, uint256& block_hash) const;
+    std::optional<Spender> ReadTransaction(const CDiskTxPos& pos) const;
 
 protected:
     interfaces::Chain::NotifyOptions CustomOptions() override;
@@ -51,7 +57,7 @@ public:
     // Destroys unique_ptr to an incomplete type.
     virtual ~TxoSpenderIndex() override;
 
-    bool FindSpender(const COutPoint& txo, CTransactionRef& tx, uint256& block_hash) const;
+    std::optional<Spender> FindSpender(const COutPoint& txo) const;
 };
 
 /// The global txo spender index. May be null.
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 04eac58ffa..a26ae2b791 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -709,13 +709,11 @@ static RPCHelpMan gettxspendingprevout()
                     // do nothing, caller has selected to only query the mempool
                 } else if (g_txospenderindex) {
                     // no spending tx in mempool, query txospender index
-                    CTransactionRef spending_tx;
-                    uint256 block_hash;
-                    if (g_txospenderindex->FindSpender(prevout, spending_tx, block_hash)) {
-                        o.pushKV("spendingtxid", spending_tx->GetHash().GetHex());
-                        o.pushKV("blockhash", block_hash.GetHex());
+                    if (auto spender{g_txospenderindex->FindSpender(prevout)}) {
+                        o.pushKV("spendingtxid", spender->tx->GetHash().GetHex());
+                        o.pushKV("blockhash", spender->block_hash.GetHex());
                         if (return_spending_tx.value_or(false)) {
-                            o.pushKV("spendingtx", EncodeHexTx(*spending_tx));
+                            o.pushKV("spendingtx", EncodeHexTx(*spender->tx));
                         }
                         if (!txospenderindex_ready) {
                             // warn if index is not ready as the spending tx that we found may be stale (it may be reorged out)
diff --git a/src/test/txospenderindex_tests.cpp b/src/test/txospenderindex_tests.cpp
index 870262ac4d..5f930aceb8 100644
--- a/src/test/txospenderindex_tests.cpp
+++ b/src/test/txospenderindex_tests.cpp
@@ -45,12 +45,9 @@ BOOST_FIXTURE_TEST_CASE(txospenderindex_initial_sync, TestChain100Setup)
 
     CBlock block = CreateAndProcessBlock(spender, this->m_coinbase_txns[0]->vout[0].scriptPubKey);
 
-    CTransactionRef tx;
-    uint256 block_hash;
-
     // Transaction should not be found in the index before it is started.
     for (const auto& outpoint : spent) {
-        BOOST_CHECK(!txospenderindex.FindSpender(outpoint, tx, block_hash));
+        BOOST_CHECK(!txospenderindex.FindSpender(outpoint));
     }
 
     // BlockUntilSyncedToCurrentChain should return false before txospenderindex is started.
@@ -58,9 +55,10 @@ BOOST_FIXTURE_TEST_CASE(txospenderindex_initial_sync, TestChain100Setup)
 
     txospenderindex.Sync();
     for (size_t i = 0; i < spent.size(); i++) {
-        BOOST_CHECK(txospenderindex.FindSpender(spent[i], tx, block_hash));
-        BOOST_CHECK_EQUAL(tx->GetHash(), spender[i].GetHash());
-        BOOST_CHECK_EQUAL(block_hash, block.GetHash());
+        auto spend{txospenderindex.FindSpender(spent[i])};
+        BOOST_CHECK(spend.has_value());
+        BOOST_CHECK_EQUAL(spend->tx->GetHash(), spender[i].GetHash());
+        BOOST_CHECK_EQUAL(spend->block_hash, block.GetHash());
     }
 
     // It is not safe to stop and destroy the index until it finishes handling

index/blockfilterindex.cpp
index/coinstatsindex.cpp
index/txindex.cpp
index/txospenderindex.cpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with #24539 (comment) that adding support for a new index merits release notes, see #33657 (doc/release-notes-33657.md), and also:

New settings
------------
- `-blockfilterindex` enables the creation of BIP158 block filters for
the entire blockchain. Filters will be created in the background and
currently use about 4 GiB of space. Note: this version of Bitcoin
Core does not serve block filters over the P2P network, although the
local user may obtain block filters using the `getblockfilter` RPC.
(#14121)


bool TxoSpenderIndex::ReadTransaction(const CDiskTxPos& tx_pos, CTransactionRef& tx, uint256& block_hash) const
{
AutoFile file{m_chainstate->m_blockman.OpenBlockFile(tx_pos, true)};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could clarify bool

Suggested change
AutoFile file{m_chainstate->m_blockman.OpenBlockFile(tx_pos, true)};
AutoFile file{m_chainstate->m_blockman.OpenBlockFile(tx_pos, /*fReadOnly=*/true)};

// find all keys that start with the outpoint hash, load the transaction at the location specified in the key
// and return it if it does spend the provided outpoint
for (it->Seek(std::pair{DB_TXOSPENDERINDEX, prefix}); it->Valid() && it->GetKey(key) && key.hash == prefix; it->Next()) {
if (ReadTransaction(key.pos, tx, block_hash)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should be returning something like util::Expected<std::optional<Spender>, TxoSpenderIndex::Error> where we can have 3 results:

  • Spending tx was found (optional is set).
  • Spending tx does not exist (optional is unset).
  • IO error occurred while reading block file.

Otherwise I'm worried users of the RPC will not be able to distinguish the last 2 cases, and potentially miss out on an output being spent due to node IO error.

@DrahtBot DrahtBot requested a review from hodlinator January 14, 2026 09:09
Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK (kind of surprised I never looked at this until now)

if (!txospenderindex_ready) {
// warn if index is not ready as the spending tx that we found may be stale (it may be reorged out)
UniValue warnings(UniValue::VARR);
warnings.push_back("txospenderindex is still being synced.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really useful to return anything if the index isn't synced yet? Unless there is specific use-case for this data I would rather throw here and in these other cases where there are warnings now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the if/else blocks here could be simplified. I would rather have some of them removed but if there is a use-case for this intermittend result data with warning then I can take a stab at restructuring it.

throw JSONRPCError(RPC_MISC_ERROR, strprintf("No spending tx for the outpoint %s:%d in mempool, and txospenderindex is unavailable.", prevout.hash.GetHex(), prevout.n));
} else {
UniValue warnings(UniValue::VARR);
warnings.push_back("txospenderindex is unavailable.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my first look, I also find it kind of odd that we throw an error when the operation at least successfully parsed the mempool (just didn't find a result) but we only give a warning when we didn't do anything useful until this point.

std::optional<bool> return_spending_tx;
if (!request.params[1].isNull()) {
const UniValue& options = request.params[1];
RPCTypeCheckObj(options,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have an example of the options usage as well (above in RPCExamples, GitHub just doesn't let me comment there)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.