rpc: add rpc to get mempool txs spending specific prevouts #24408

t-bast · 2022-02-21T17:24:18Z

We add an RPC to fetch mempool transactions spending any of the given outpoints.

Without this RPC, application developers need to first call getrawmempool which returns a long list of txid, then fetch each of these transactions individually (getrawtransaction) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool).

For example in lightning, when we discover that one of our channel funding transactions has been spent, we need to find the spending transaction to claim our outputs from it. We are currently forced to fetch the whole mempool to do the analysis ourselves, which is quite costly.

I believe that this RPC is also generally useful when doing some introspection on your mempool after one of your transactions failed to broadcast, for example when you implement RBF at the application level. Fetching and analyzing the conflicting transaction gives you more information to successfully replace it.

test/functional/mempool_packages.py

darosior · 2022-02-21T17:28:21Z

For this usecase, why not using getmempoolentry's spentby field?

t-bast · 2022-02-21T17:30:35Z

For this usecase, why not using getmempoolentry's spentby field?

Because that wouldn't work for confirmed utxos!

My funding transaction (or any L2 protocol initial state) has been confirmed on-chain and I want to check who spends it in the mempool. I can't use getmempoolentry because I don't know yet the txid of the spending tx...

Maybe the RPC name is misleading, because its inputs (txid and vout) aren't necessarily for a mempool tx (and most of the time will not), but the RPC output will be a mempool tx (or null).

darosior · 2022-02-21T17:39:00Z

Right, sorry, i shouldn't have inferred the behaviour from the name. I see it's in the context of txindex then, so i don't have an opinion.

t-bast · 2022-02-21T17:50:42Z

It's much weaker than txindex, it's exposing a txindex for mempool contents, which is why I think it's acceptable (I'm only querying existing mempool functions).

ghost · 2022-02-22T02:04:29Z

For this usecase, why not using getmempoolentry's spentby field?

Because that wouldn't work for confirmed utxos!

My funding transaction (or any L2 protocol initial state) has been confirmed on-chain and I want to check who spends it in the mempool. I can't use getmempoolentry because I don't know yet the txid of the spending tx...

Maybe the RPC name is misleading, because its inputs (txid and vout) aren't necessarily for a mempool tx (and most of the time will not), but the RPC output will be a mempool tx (or null).

Is it possible to add it in existing RPC getmempoolentry by adding more arguments (optional)?

t-bast · 2022-02-22T08:48:35Z

Is it possible to add it in existing RPC getmempoolentry by adding more arguments (optional)?

I'm not sure it would be very ergonomic, because getmempoolentry requires the txid of the mempool transaction you're fetching. If we wanted to overload it, it would have the following two distinct sets of behaviors:

a txid is provided, so we return a mempool transaction that matches this txid
a txid (probably named differently to avoid confusion) and a vout are provided, so we return a mempool transaction spending this given txid:vout

Correctly defining the arguments lists (and which are required) to support both use-cases without creating confusion seems hard to be honest...

nopara73 · 2022-02-22T11:15:28Z

Not being able to walk the chain forward is certainly the source of huge friction and hacky code while working with Bitcoin RPC. I'd even go that far to claim that this is the single largest missing feature that if we'd have, it would have saved the most time for me personally. Certainly cACK this approach, but I'd like to note a general version of it would be more useful, somehow we should be able to determine which transaction spends a UTXO if any, regardless if it's from mempool or not.

glozow

Concept ACK, seems very reasonable to me especially since we already have a mempool function to fetch the transaction. Also in general, it seems useful to be able to fetch the transaction that conflicts with yours.

test/functional/mempool_packages.py

glozow · 2022-02-22T11:24:16Z

src/rpc/blockchain.cpp

+                "\nScans the mempool to find a transaction spending the given output\n",
+                {
+                    {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "transaction id"},
+                    {"n", RPCArg::Type::NUM, RPCArg::Optional::NO, "vout number"},
+                },


I'm wondering if, in the future, you might want to pass in a list of prevouts? (e.g. if you have transaction(s) with multiple inputs and want to see if anything in the mempool conflicts)

That's a good idea, it's worth adding this flexibility right now to avoid running in backwards-compatibility issues if we add it later, I'll try that. I'll take a list of prevouts and return a map of prevout -> spending tx (or null if not found).

Done in 0015ceb
I chose to return only the spendingtxid instead of the mempool tx data, because otherwise if a transaction appears multiple times (because it spends many of the provided prevouts) it would be duplicated, which is a (small) waste.

Callers can chain this with getmempoolentry to get more data about the conflicting transaction.
If you think it makes more sense to directly include the mempool tx let me know, I don't have strong feelings about that.

src/rpc/blockchain.cpp

glozow · 2022-02-22T11:32:07Z

I'd like to note a general version of it would be more useful, somehow we should be able to determine which transaction spends a UTXO if any, regardless if it's from mempool or not.

This might be tricky (the non-mempool case) since we would have deleted the UTXO from our chainstate. And not possible sometimes on a pruned node.

michaelfolkson · 2022-02-22T11:55:35Z

Concept ACK - rationale in PR description seems strong and other projects (e.g. Wasabi) have experienced same issue.

I'd like to note a general version of it would be more useful, somehow we should be able to determine which transaction spends a UTXO if any, regardless if it's from mempool or not

I think the use case from @nopara73's comment above would be you don't know whether the transaction spending the UTXO is in your mempool or confirmed onchain and you want to query your mempool first and then your transaction index? I think generally you'd want one RPC to query your mempool and one RPC to query your transaction index. Combining both into a single RPC seems to me against separation of concerns (and is easily resolved by just running two RPC commands).

nopara73 · 2022-02-22T12:01:14Z

FTR there's gettxout. I think my suggestion would fit best there.

t-bast · 2022-02-22T12:31:46Z

FTR there's gettxout. I think my suggestion would fit best there.

@nopara73 do you mean that instead of adding a new RPC like I'm doing, we could update gettxout to return the spending tx when the utxo is spent?

In the case where the spending tx is not in the mempool, I think the only solution would be to walk the chain backwards and analyze each block to see if the spending tx is inside it. However, as @glozow says, if you're running a pruned node, you will likely analyze every block you have and won't find anything, which is very inefficient. Even worse, if the provided utxo never existed, you will walk the whole chain without finding anything...

It looks like a much more debatable change which would involve serious trade-offs, that's why I think restricting this PR to mempool txs has a better chance of being accepted. I could still reuse gettxout instead of adding a new RPC, but that would make it harder (backwards-compatibility wise) to accept a list of prevouts as @glozow suggests here.

DrahtBot · 2022-02-25T06:51:53Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25204 (rpc: remove deprecated top-level fee fields from mempool entries by theStack)
#22341 (rpc: add getxpub by Sjors)
#21422 (Add feerate histogram to getmempoolinfo by kiminuo)

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.

w0xlt

Concept ACK.
Maybe the two commits can be merged. Apparently the first commit is in an invalid state (since the outputs parameter is not converted).

t-bast · 2022-02-25T15:40:35Z

Maybe the two commits can be merged.

Definitely, done. I was waiting for the builds to complete to check that I hadn't missed something else that would need fixing.

maflcko · 2022-03-03T19:23:51Z

Without this RPC, application developers need to first call getrawmempool which returns a long list of txid, then fetch each of these transactions individually (getrawtransaction) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool).

I think without this RPC, you can create a tx that spends the input and only fetch the whole mempool if the input was spent.

Moreover, I wonder what happens if the tx is included in a block right away? So the input will be gone, but there isn't a tx in the mempool that spends it.

While the RPC makes sense on its own, it doesn't fix your problem completely?

doc/release-notes.md

src/rpc/blockchain.cpp

t-bast · 2022-03-03T23:52:08Z

Thanks for the review @MarcoFalke!

I think without this RPC, you can create a tx that spends the input and only fetch the whole mempool if the input was spent.
Moreover, I wonder what happens if the tx is included in a block right away? So the input will be gone, but there isn't a tx in the mempool that spends it.
While the RPC makes sense on its own, it doesn't fix your problem completely?

I probably tried to be a bit too general in my description, let me try to explain in more details my current scenario.

In the specific case where I want to apply this in the short term, I'm trying to spend lightning htlcs. I have a pre-signed transaction that spends the outpoint I'm interested in but doesn't provide enough fees, so I'll want to add more inputs to reach the right feerate.

The complete flow looks like this:

I verify that the output is unspent with gettxout (without including the mempool)
- If it's spent, then I have nothing to do (actually it triggers another flow to spend the output of that spending tx, but that's orthogonal)
- Otherwise, I check if there is already a mempool tx spending this output with getmempooltxspendingprevout
  - If nothing is in the mempool, I'll use fundrawtransaction to add inputs and broadcast the transaction
  - Otherwise, I'll modify the mempool transaction to pay more fees (by lowering/removing the existing change output and potentially adding new inputs)

Without this RPC, I would have to optimistically fund and broadcast, and if that fails fetch the whole mempool to find the conflicting transaction. I expect to regularly have to go through the RBF flow, so it feels inefficient to have to dump the mempool when that happens :(

You're right that if the current mempool transaction confirms while I'm funding its replacement, I'll need to correctly handle it, but that doesn't seem to be too much of a problem for my use-case.

Does that make sense with this more detailed scenario?

t-bast · 2022-04-25T08:38:13Z

Thanks for the review @glozow!

I don't feel strongly, but if you change your mind, I think it's perfectly fine to include the whole spending tx. There's not much waste since we've already retrieved it from the mempool, and only a privileged user can be using the RPC interface anyway.

I am more and more convinced that it's better to only have the spendingtxid. There is a lot of useful information in the output of getrawtransaction that callers will want, so they will very likely follow up with a call to that RPC anyway (and we don't want to replicate all of its contents in the output of gettxspendingprevout).

We're regularly seeing big coinjoin transactions in our systems, so if we include the whole tx in the RPC response we can easily have responses that span several Mb, which is a waste for bitcoind nodes on remote machines accessed through a (sometimes flaky) VPN (which we've seen some lightning node operators do).

glozow · 2022-04-26T00:17:36Z

re ACK 8c9152c

maflcko · 2022-04-27T12:35:02Z

cc Antoine for review (@darosior @ariard), if interested.

ariard

Code Review ACK 8c9152c

A caveat : the usage of this RPC you're describing sounds unsafe to me. If you're desirous to monitor the spend of a shared-utxo such as LN funding output, your counterparty might be able to broadcast multiple conflicting transactions. If this counterparty knows your full-node, your mempool can even be partitioned from the rest of the network, with a junk feerate, potentially wrecking your fee-bump. Of course, this assumes a bit of cross-layer mapping work on your malicious counterparty. So better to reconciliate from multiple mempools or opt for blind fee-bump after a while imho.

src/rpc/mempool.cpp

t-bast · 2022-04-28T06:39:01Z

A caveat : the usage of this RPC you're describing sounds unsafe to me. If you're desirous to monitor the spend of a shared-utxo such as LN funding output, your counterparty might be able to broadcast multiple conflicting transactions.

Thanks for the comments! But that shouldn't be an issue in our case (at least not related to this RPC). This RPC is helpful to react instantly once a shared output is spent. It doesn't matter if in the end it's another transaction that gets confirmed, we will learn this when we receive blocks and will then adapt. It then becomes a different problem of ensuring you're receiving blocks (avoiding complete eclipse attacks), which we solve by a combination of blockchain watchdogs and a good bitcoind setup.

Protecting against commit tx pinning is an entirely separate issue as well, which exists regardless of whether this RPC exists or not.

luke-jr · 2022-04-29T20:09:11Z

@luke-jr can you detail how that would work?

You would add the txout you need to watch to the wallet (maybe we need to add a way to do that), then the wallet would scan every new block/tx internally, letting you know via listtransactions.

it would be wasteful to need both a watch-only wallet and a regular wallet to make this use-case work?

At least legacy wallets support both normal and watch-only in the same wallet...

w0xlt

tACK 8f9335f

I confirmed that the new RPC correctly returns the ids of transactions that spend the outpoints passed as a parameter.

src/rpc/mempool.cpp

test/functional/rpc_mempool_info.py

t-bast · 2022-05-02T06:59:00Z

@luke-jr thanks, I understand what you mean, I didn't know a wallet could be both normal and watch-only. This is something that could make sense, we are currently doing this scan outside of bitcoind, but having bitcoind do it for us could be useful.

Does it completely replace this PR though? I can see value in having both options (a dedicated RPC and a watch-only feature), the watch-only mode could be added afterwards (especially if we see people starting to rely on this new RPC).

darosior

Conceptually, this feature needs a transaction indexer and that's what a wallet is. I think ideally this feature should be part of the wallet as a more general "get spending tx by prevout" feature, rather than a very usecase-specific feature leveraging the mempool index.
However, it would be a bit convoluted to use at the moment. It would require them to import a new descriptor for each output they would need to claim, without (currently) the possibility to delete it. The number of descriptors under watch would therefore grow continuously as the software would be used.
Accounting for the immediate benefit to users, and the low cost of this RPC (it only calls into a single mempool function), i agree it can be merged. I believe longer term we should rather improve the wallet instead of providing (what i think are) specialized wallet functionalities at the node level.

light code review ACK 8f9335f -- the code makes sense to me and i didn't spot any issue. But i only quickly glanced at the diff.

(For reference, we have a similar issue with revaultd: in order to fetch a spending transaction from the watchonly wallet whose txid is unknown we need to iterate on the list of the wallet transactions. https://github.com/revault/revaultd/blob/b310208a8b3b420f9b0cddcc03588e264a99538b/src/bitcoind/interface.rs#L777-L864 (fortunately listsinceblock allows us to reduce the cost by only iterating on transactions since the last pool).)

t-bast · 2022-05-02T08:54:20Z

Conceptually, this feature needs a transaction indexer and that's what a wallet is. I think ideally this feature should be part of the wallet as a more general "get spending tx by prevout" feature, rather than a very usecase-specific feature leveraging the mempool index.

I agree, and you're one step ahead, see #24539 :)
Thanks for the comments!

w0xlt

reACK ad14372

We add an RPC to fetch the mempool transactions spending given outpoints. Without this RPC, application developers would need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these txs individually to check whether they spend the given outpoint(s). This RPC can later be enriched to also find confirmed transactions instead of being restricted to mempool transactions.

t-bast · 2022-05-05T13:16:53Z

Simple rebase on master without any changes in 4185570

t-bast · 2022-05-18T07:44:02Z

I don't have any pending issues on that PR, if @ariard @darosior @glozow @MarcoFalke you have time to give this another pass whenever possible it would be great 🙏

darosior

re-utACK 4185570

vincenzopalazzo

re-ACK 4185570

danielabrozzoni

re-tACK 4185570

We add an RPC to fetch the mempool transactions spending given outpoints. Without this RPC, application developers would need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these txs individually to check whether they spend the given outpoint(s). This RPC can later be enriched to also find confirmed transactions instead of being restricted to mempool transactions. Github-Pull: bitcoin#24408 Rebased-From: ad14372

fanquake · 2022-05-23T07:30:12Z

cc @glozow

w0xlt

reACK 4185570

…c prevouts 4185570 Add RPC to get mempool txs spending outputs (t-bast) Pull request description: We add an RPC to fetch mempool transactions spending any of the given outpoints. Without this RPC, application developers need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these transactions individually (`getrawtransaction`) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool). For example in lightning, when we discover that one of our channel funding transactions has been spent, we need to find the spending transaction to claim our outputs from it. We are currently forced to fetch the whole mempool to do the analysis ourselves, which is quite costly. I believe that this RPC is also generally useful when doing some introspection on your mempool after one of your transactions failed to broadcast, for example when you implement RBF at the application level. Fetching and analyzing the conflicting transaction gives you more information to successfully replace it. ACKs for top commit: darosior: re-utACK 4185570 vincenzopalazzo: re-ACK bitcoin@4185570 danielabrozzoni: re-tACK 4185570 w0xlt: reACK bitcoin@4185570 Tree-SHA512: 206687efb720308b7e0b6cf16dd0a994006c0b5a290c8eb386917a80130973a6356d0d5cae1c63a01bb29e066dd721594969db106cba7249214fcac90d2c3dbc

t-bast force-pushed the get-mempool-spending-tx branch from a7966e4 to 2a7db52 Compare February 21, 2022 17:25

t-bast commented Feb 21, 2022

View reviewed changes

test/functional/mempool_packages.py Outdated Show resolved Hide resolved

t-bast force-pushed the get-mempool-spending-tx branch from 2a7db52 to cc038b6 Compare February 21, 2022 17:55

DrahtBot added the RPC/REST/ZMQ label Feb 21, 2022

glozow reviewed Feb 22, 2022

View reviewed changes

t-bast force-pushed the get-mempool-spending-tx branch 2 times, most recently from 0b6cdaa to 4ef5a16 Compare February 24, 2022 16:28

t-bast changed the title ~~Add RPC to get mempool spending tx~~ rpc: add rpc to get mempool spending tx Feb 25, 2022

t-bast changed the title ~~rpc: add rpc to get mempool spending tx~~ rpc: add rpc to get mempool txs spending specific prevouts Feb 25, 2022

w0xlt reviewed Feb 25, 2022

View reviewed changes

t-bast force-pushed the get-mempool-spending-tx branch from 263b0a7 to 0015ceb Compare February 25, 2022 15:40

DrahtBot mentioned this pull request Feb 25, 2022

rpc: add path to gethdkey #22341

Closed

DrahtBot added the Needs rebase label Mar 3, 2022

maflcko reviewed Mar 3, 2022

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

maflcko reviewed Mar 3, 2022

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

ariard reviewed Apr 27, 2022

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

t-bast force-pushed the get-mempool-spending-tx branch from 8c9152c to 8f9335f Compare April 28, 2022 06:40

w0xlt approved these changes May 1, 2022

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

test/functional/rpc_mempool_info.py Outdated Show resolved Hide resolved

darosior reviewed May 2, 2022

View reviewed changes

t-bast force-pushed the get-mempool-spending-tx branch from 8f9335f to ad14372 Compare May 2, 2022 11:20

w0xlt approved these changes May 2, 2022

View reviewed changes

t-bast force-pushed the get-mempool-spending-tx branch from ad14372 to 4185570 Compare May 5, 2022 12:57

darosior reviewed May 18, 2022

View reviewed changes

vincenzopalazzo approved these changes May 18, 2022

View reviewed changes

danielabrozzoni approved these changes May 18, 2022

View reviewed changes

fanquake requested a review from glozow May 18, 2022 09:41

w0xlt approved these changes May 23, 2022

View reviewed changes

DrahtBot mentioned this pull request May 25, 2022

rpc: remove deprecated top-level fee fields from mempool entries #25204

Merged

maflcko merged commit 3ba6dd6 into bitcoin:master May 27, 2022

t-bast deleted the get-mempool-spending-tx branch May 29, 2022 22:58

glozow mentioned this pull request Sep 14, 2022

24.0 RC Testing Guide Feedback #26092

Closed

bitcoin locked and limited conversation to collaborators May 29, 2023

rpc: add rpc to get mempool txs spending specific prevouts #24408

rpc: add rpc to get mempool txs spending specific prevouts #24408

Uh oh!

Conversation

t-bast commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

darosior commented Feb 21, 2022

Uh oh!

t-bast commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior commented Feb 21, 2022

Uh oh!

t-bast commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 22, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bast commented Feb 22, 2022

Uh oh!

nopara73 commented Feb 22, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

t-bast Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

t-bast Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow commented Feb 22, 2022

Uh oh!

michaelfolkson commented Feb 22, 2022

Uh oh!

nopara73 commented Feb 22, 2022

Uh oh!

t-bast commented Feb 22, 2022

Uh oh!

DrahtBot commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

t-bast commented Feb 25, 2022

Uh oh!

maflcko commented Mar 3, 2022

Uh oh!

Uh oh!

Uh oh!

t-bast commented Mar 3, 2022

Uh oh!

t-bast commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Apr 26, 2022

Uh oh!

maflcko commented Apr 27, 2022

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-bast commented Apr 28, 2022

Uh oh!

luke-jr commented Apr 29, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

t-bast commented Feb 21, 2022 •

edited

Loading

t-bast commented Feb 21, 2022 •

edited

Loading

t-bast commented Feb 21, 2022 •

edited

Loading

ghost commented Feb 22, 2022 •

edited by ghost

Loading

t-bast Feb 24, 2022 •

edited

Loading

DrahtBot commented Feb 25, 2022 •

edited

Loading

t-bast commented Apr 25, 2022 •

edited

Loading