wallet: indicate whether a transaction is in the mempool

danben commented at 11:19 pm on February 21, 2021: contributor

Added a field to the output of listtransactions/gettransaction to indicate whether a transaction is in the mempool. Closes #21018.

fanquake added the label Wallet on Feb 21, 2021

fanquake added the label RPC/REST/ZMQ on Feb 21, 2021

danben force-pushed on Feb 22, 2021

shesek commented at 5:03 pm on February 23, 2021: contributor

tACK ea6dcf69d4e42d5dd4636079b31151e260c4b45a. I did tx replacement using bumpfee and observed that inmempool is returned correctly for both the old and new transactions, and ran the functional tests.

That’s exactly what I wanted, thank you!

in src/wallet/rpcwallet.cpp:184 in ea6dcf69d4 outdated

180@@ -181,6 +181,7 @@ static void WalletTxToJSON(interfaces::Chain& chain, const CWalletTx& wtx, UniVa
181             rbfStatus = "yes";
182     }
183     entry.pushKV("bip125-replaceable", rbfStatus);
184+    entry.pushKV("inmempool", wtx.fInMempool);

luke-jr commented at 3:28 am on February 24, 2021:

"in_mempool" seems like a better name.

luke-jr commented at 3:29 am on February 24, 2021:

Probably should be omitted entirely if confirms != 0

shesek commented at 11:30 am on February 25, 2021:

I think so too.

jonatack commented at 11:34 am on February 25, 2021:

:+1: for in_mempool ( we use snakecased field names now in the JSON RPC output)

luke-jr changes_requested

luke-jr commented at 3:30 am on February 24, 2021: member

I’m not sure there’s a valid use case for this, but here’s a code review.

leonardojobim commented at 4:31 am on February 25, 2021: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/21260/commits/ea6dcf69d4e42d5dd4636079b31151e260c4b45a on Ubuntu 20.04.

The functional tests ran successfully. And on testnet, the inmempool of the transaction with the highest fee was shown as false after the transaction has been included in the block, as expected.

#21260 (review) has a valid point, since if confirms != 0, the inmempool will always be false (it would require changes in the test wallet_basic.py though).

darosior commented at 10:35 am on February 25, 2021: member

What is the rationale for this ? We already have the blockheight field and istm that apart from conflicts (which you can already track with walletconflicts) this holds at all time:

0(blockheight == null) <=> (inmempool == true)

So i’m not sure this adds any new information?

shesek commented at 11:26 am on February 25, 2021: contributor

which you can already track with walletconflicts

You can’t tell which of the transactions is currently in the mempool and which ones were replaced based on walletconflicts, it’ll have the list of other txids in both cases.

I’m not sure there’s a valid use case for this,

Coin selection is one use case, you can use your latest change output but not the previous replaced ones.

Another one is displaying the list of transactions to the user. It makes sense to hide the replaced ones, or at least make them visually distinct from the ‘active’ one.

in test/functional/test_runner.py:197 in ea6dcf69d4 outdated

193@@ -194,6 +194,7 @@
194     'p2p_invalid_messages.py',
195     'p2p_invalid_tx.py',
196     'feature_assumevalid.py',
197+    'wallet_showconflictedinactivetransactions.py',

jonatack commented at 11:35 am on February 25, 2021:

ISTM we use snakecased names now for new functional test files

in test/functional/wallet_showconflictedinactivetransactions.py:9 in ea6dcf69d4 outdated

0@@ -0,0 +1,52 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2020 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test that listtransactions shows whether a transaction is in the mempool."""
6+from test_framework.test_framework import BitcoinTestFramework
7+from test_framework.util import assert_equal
8+
9+class WalletShowConflictedInactiveTransactionsTest(BitcoinTestFramework):

jonatack commented at 11:37 am on February 25, 2021:

Why not add this test in the existing test/functional/wallet_listtransactions.py / ListTransactionsTest instead of creating a new file.

jonatack commented at 11:39 am on February 25, 2021: member

Quick look, a few comments below.

jonatack commented at 11:43 am on February 25, 2021: member

Would need to update the listtransactions and gettransaction RPC helps (and add a release note).

in test/functional/wallet_showconflictedinactivetransactions.py:38 in ea6dcf69d4 outdated

33+            assert_equal(tx['txid'], tx1_id)
34+            assert_equal(tx['inmempool'], True)
35+
36+        tx2_id = self.create_and_send_transaction(utxo, address, 10, 0.002)
37+
38+        new_txs = self.nodes[0].listtransactions(count=4)

jonatack commented at 11:51 am on February 25, 2021:

Maybe add tests for gettransaction() as well.

danben commented at 6:44 pm on February 25, 2021:

Do you have an opinion on where they should go?

danben commented at 2:49 pm on February 26, 2021:

Never mind, looks like there’s precedence now for wallet_listtransactions.py

danben commented at 10:17 pm on February 25, 2021: contributor

Would need to update the listtransactions and gettransaction RPC helps (and add a release note).

I wasn’t sure what to do about that since as far as I can tell none of the other stuff from WalletTxToJSON shows up in the help text

danben force-pushed on Feb 25, 2021

in src/wallet/rpcwallet.cpp:184 in 6d09120094 outdated

180@@ -181,6 +181,8 @@ static void WalletTxToJSON(interfaces::Chain& chain, const CWalletTx& wtx, UniVa
181             rbfStatus = "yes";
182     }
183     entry.pushKV("bip125-replaceable", rbfStatus);
184+    if (confirms == 0)

luke-jr commented at 0:11 am on February 26, 2021:

Needs braces or on a single line

danben commented at 0:18 am on February 26, 2021:

Ah right, I was just trying to make it match the surrounding code but I see from the style guide that you’re correct. Will fix

DrahtBot commented at 5:54 am on February 26, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22798 (doc: Fix RPC result documentation by MarcoFalke)
#22100 (refactor: Clean up new wallet spend, receive files added #21207 by ryanofsky)

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.

danben force-pushed on Feb 26, 2021

danben force-pushed on Feb 28, 2021

danben commented at 9:46 pm on March 1, 2021: contributor

I can’t reproduce this failure locally, just wondering if any reviewers have any insights

danben force-pushed on Mar 8, 2021

danben commented at 2:42 am on March 8, 2021: contributor

Fixed, all tests passing now

in src/wallet/rpcwallet.cpp:184 in 74c4326eb0 outdated

180@@ -181,7 +181,7 @@ static void WalletTxToJSON(interfaces::Chain& chain, const CWalletTx& wtx, UniVa
181             rbfStatus = "yes";
182     }
183     entry.pushKV("bip125-replaceable", rbfStatus);
184-
185+    if (confirms == 0) entry.pushKV("in_mempool", wtx.fInMempool);

promag commented at 8:35 am on April 22, 2021:

nit, could move after L163.

promag commented at 8:53 am on April 22, 2021: member

Code review ACK 74c4326eb0deb52e96faace921a12eedfbfdf78a.

I don’t see any downside of exposing this field.

I think there’s no need to have split test functions for listtransactions and gettransaction, both use the same setup.

BTW, I think @jonatack already mentioned this needs release notes.

darosior commented at 10:00 am on April 22, 2021: member

Concept ACK (based on shesek’s answer to my and luke’s concerns).

MarcoFalke commented at 6:50 am on April 26, 2021: member

Needs to update the doc of gettransaction?

danben force-pushed on Apr 26, 2021

danben commented at 8:08 pm on April 26, 2021: contributor

@promag @MarcoFalke re: help text, as I mentioned above I wasn’t sure how to think about this because this would be the only field originating in WalletTxToJSON that appears in the help text for either RPC. I’m happy to just add it and not think too hard about it but I’m curious to know the rationale here if there is one.

promag commented at 8:23 pm on April 26, 2021: member

The rationale is to update the help output of each affected method. I think you have to update TransactionDescriptionString()?

danben commented at 8:35 pm on April 26, 2021: contributor

The rationale is to update the help output of each affected method. I think you have to update TransactionDescriptionString()? @promag Thanks for the quick response. My question wasn’t about why help text should be added for this field, but why it isn’t there for all of the other fields (that also come from WalletTxToJSON). That is the only reason I was hesitant to add it in the first place.

promag commented at 9:15 pm on April 26, 2021: member

If some field is missing then it should be added.

danben commented at 9:19 pm on April 26, 2021: contributor

Apologies, I was looking in the wrong place. I see they’re there. Sorry for the confusion.

Added a field to the output of gettransaction/listtransactions to indicate whether the given transaction is in the mempool. 46bf0b7b5d

danben force-pushed on Apr 26, 2021

MarcoFalke commented at 8:26 am on April 27, 2021: member

unsigned cr ACK 46bf0b7b5d8c44bd7032c473f9878cfb59018161

darosior approved

darosior commented at 8:32 am on May 3, 2021: member

utACK 46bf0b7b5d8c44bd7032c473f9878cfb59018161

ryanofsky approved

ryanofsky commented at 1:21 pm on May 4, 2021: member

Concept -0 on this PR. Code looks good and test coverage for listtransactions is nice. But the PR is poorly motivated, and if original issue reporter tries to use this solution to solve his problem I think it could result in a poor user experiences with transactions mysteriously disappearing from history, coins mysteriously disappearing from coin selection, and the potential for accidentally sending payments multiple times when attempting to spend once.

On a meta level, I think we should require PRs like this to be better motivated. PR descriptions should make motivations clear and be described in terms of specific use cases. Here, no use-case is mentioned in the PR description or in the linked issue description. You have to decipher an IRC discussion linked to by a commenter in the discussion thread in the issue this PR links to.

I don’t have any problems with the implementation of this PR, and will add my code review ACK 46bf0b7b5d8c44bd7032c473f9878cfb59018161 if other folks working on wallet code want to merge this. But I also think perhaps a more ideal way to move forward would be to close this PR and close the linked issue for not being well enough motivated, and to encourage the original issue reporter to fully describe the use case and problem in a new issue so we can find a better solution.

darosior commented at 1:33 pm on May 4, 2021: member

Some more context for my Concept ACK that may help motivating this PR.

As an application “on top” of bitcoind’s wallet, one may want to track the state of an outgoing transaction. Using gettransaction one is able to know if the transaction was broadcasted, and whether it’s confirmed (using the blockheight field). If it’s not, one need another RPC call (getmempoolentry) to make sure the transaction is “still current”.

Given the information was already present, this is a simple patch allowing consumers of the API to save an additional call :).

ryanofsky commented at 1:44 pm on May 4, 2021: member

That makes sense. If you want to do something like show the transaction in green if it’s in the mempool and broadcasted and gray if it was never broadcast or is stuck for some reason, you could use this field for that.

I guess was just questioning how #21018 seems to want to use this field, conflating whether a transaction is in the mempool with whether the transaction is abandoned or replaced.

promag commented at 1:50 pm on May 4, 2021: member

I’ve ACK just because it’s a simple field already exposed in the RPC interface. Seems fine if it saves another request or bach of requests in case of listing transactions - as always clients must be aware that these flags can change right after the RPC response is built.

shesek commented at 11:21 pm on June 26, 2021: contributor

@ryanofsky My motivation was described in a comment on this PR: #21260 (comment)

Coin selection is one use case, you can use your latest change output but not the previous replaced ones.

Another one is displaying the list of transactions to the user. It makes sense to hide the replaced ones, or at least make them visually distinct from the ‘active’ one.

luke-jr referenced this in commit e78ff2f8c0 on Jun 27, 2021

in src/wallet/rpcwallet.cpp:1375 in 46bf0b7b5d

1371@@ -1372,6 +1372,7 @@ static const std::vector<RPCResult> TransactionDescriptionString()
1372                "transaction conflicted that many blocks ago."},
1373            {RPCResult::Type::BOOL, "generated", "Only present if transaction only input is a coinbase one."},
1374            {RPCResult::Type::BOOL, "trusted", "Only present if we consider transaction to be trusted and so safe to spend from."},
1375+           {RPCResult::Type::BOOL, "in_mempool", "Only present on unconfirmed transactions."},

jonatack commented at 9:51 am on June 29, 2021:

Suggest saying what the field represents.

0           {RPCResult::Type::BOOL, "in_mempool", "Whether the transaction is in the mempool. Only present if the transaction is unconfirmed."},

in test/functional/wallet_listtransactions.py:219 in 46bf0b7b5d

211@@ -210,5 +212,57 @@ def get_unconfirmed_utxo_entry(node, txid_to_match):
212         assert_equal(self.nodes[0].gettransaction(txid_3b)["bip125-replaceable"], "no")
213         assert_equal(self.nodes[0].gettransaction(txid_4)["bip125-replaceable"], "unknown")
214 
215+    def create_and_send_transaction(self, utxo, address, amt, feeRate):
216+        psbtx = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo['txid'], "vout": utxo['vout']}],
217+                                                      {address: amt},
218+                                                      0,
219+                                                      {"replaceable":True, "feeRate":feeRate})['psbt']

jonatack commented at 9:56 am on June 29, 2021:

The feeRate option in BTC/kvB is expected to be deprecated; use the fee_rate option in sat/vB

0                                                      {"replaceable": True, "fee_rate": fee_rate})['psbt']

jonatack commented at 10:02 am on June 29, 2021:

It might be clearer to use named arguments here, e.g.:

0+        psbtx = self.nodes[0].walletcreatefundedpsbt(
1+            inputs=[{"txid": utxo["txid"], "vout": utxo["vout"]}],
2+            outputs={address: amount},
3+            locktime=0,
4+            options={"replaceable": True, "fee_rate": fee_rate},
5+        )["psbt"]

in test/functional/wallet_listtransactions.py:215 in 46bf0b7b5d

211@@ -210,5 +212,57 @@ def get_unconfirmed_utxo_entry(node, txid_to_match):
212         assert_equal(self.nodes[0].gettransaction(txid_3b)["bip125-replaceable"], "no")
213         assert_equal(self.nodes[0].gettransaction(txid_4)["bip125-replaceable"], "unknown")
214 
215+    def create_and_send_transaction(self, utxo, address, amt, feeRate):

jonatack commented at 9:56 am on June 29, 2021:

naming style nit

0    def create_and_send_transaction(self, utxo, address, amount, fee_rate):

in test/functional/wallet_listtransactions.py:229 in 46bf0b7b5d

224+    def test_listtransactions_display_in_mempool(self):
225+        self.log.info('Testing that listtransactions correctly displays whether a transaction is in the mempool')
226+        utxo = self.nodes[0].listunspent()[0]
227+        address = self.nodes[0].getnewaddress()
228+
229+        tx1_id = self.create_and_send_transaction(utxo, address, 0.1, 0.001)

jonatack commented at 9:57 am on June 29, 2021:

When calling RPCs with lots of arguments, consider using named keyword arguments instead of positional arguments to make the intent of the call clear to readers (see test/functional/README.md).

0        tx1_id = self.create_and_send_transaction(utxo, address, amount=0.1, fee_rate=100)

in test/functional/wallet_listtransactions.py:111 in 46bf0b7b5d

106@@ -107,6 +107,8 @@ def run_test(self):
107                                 {"txid": txid, "label": "watchonly"})
108 
109         self.run_rbf_opt_in_test()
110+        self.test_listtransactions_display_in_mempool()
111+        self.test_gettransaction_display_in_mempool()

jonatack commented at 9:59 am on June 29, 2021:

These two tests have nearly identical setup and could be combined into one.

in test/functional/wallet_listtransactions.py:264 in 46bf0b7b5d

259+        tx1 = self.nodes[0].gettransaction(tx1_id)
260+        tx2 = self.nodes[0].gettransaction(tx2_id)
261+        assert_equal(tx1['txid'], tx1_id)
262+        assert_equal(tx1['in_mempool'], False)
263+        assert_equal(tx2['txid'], tx2_id)
264+        assert_equal(tx2['in_mempool'], True)

jonatack commented at 10:01 am on June 29, 2021:

It would be good to also test for the absence of the in_mempool field when the transaction has more than zero confirmations, e.g. somethnig like

0+        node.generate(1)
1+        self.sync_all()
2+        tx2 = node.gettransaction(tx2_id)
3+        assert_equal(tx2["confirmations"], 1)
4+        assert "in_mempool" not in tx2  # only present if txn is unconfirmed

jonatack commented at 10:22 am on June 29, 2021: member

Concept ACK. The commit message is a bit long and a number of suggestions follow. If it helps, I’ve implemented them in the following branch; feel free to use it or pull it in if you like to save time:

https://github.com/jonatack/bitcoin/commits/listtransactions_in_mempool_test

jonatack commented at 7:21 pm on July 20, 2021: member

@danben Do you plan to continue working on this?

DrahtBot added the label Needs rebase on Sep 3, 2021

DrahtBot commented at 9:27 am on September 3, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 12:28 pm on December 22, 2021: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

fanquake added the label Up for grabs on Apr 26, 2022

fanquake closed this on Apr 26, 2022

luke-jr referenced this in commit ee0a735e6c on May 21, 2022

DrahtBot locked this on Apr 26, 2023

wallet: indicate whether a transaction is in the mempool #21260

Conflicts