wallet: indicate whether a transaction is in the mempool #21260
Conversation
def261d to
ea6dcf6
Compare
|
tACK ea6dcf6. I did tx replacement using That's exactly what I wanted, thank you! |
There was a problem hiding this comment.
Tested ACK ea6dcf6 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 (comment) 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).
There was a problem hiding this comment.
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:
(blockheight == null) <=> (inmempool == true)
So i'm not sure this adds any new information?
You can't tell which of the transactions is currently in the mempool and which ones were replaced based on
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. |
|
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 |
1eb08a4 to
6d09120
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
a0b8ba7 to
c5cd7a1
Compare
|
I can't reproduce this failure locally, just wondering if any reviewers have any insights |
c5cd7a1 to
74c4326
Compare
|
Fixed, all tests passing now |
|
Needs to update the doc of |
74c4326 to
9faa0d9
Compare
|
@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 |
|
The rationale is to update the help output of each affected method. I think you have to update |
@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 |
|
If some field is missing then it should be added. |
|
Apologies, I was looking in the wrong place. I see they're there. Sorry for the confusion. |
…icate whether the given transaction is in the mempool.
9faa0d9 to
46bf0b7
Compare
|
unsigned cr ACK 46bf0b7 |
There was a problem hiding this comment.
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 46bf0b7 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.
|
Some more context for my Concept ACK that may help motivating this PR. As an application "on top" of Given the information was already present, this is a simple patch allowing consumers of the API to save an additional call :). |
|
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. |
|
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. |
|
@ryanofsky My motivation was described in a comment on this PR: #21260 (comment)
|
…icate whether the given transaction is in the mempool. Github-Pull: bitcoin#21260 Rebased-From: 46bf0b7
There was a problem hiding this comment.
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
| @@ -1372,6 +1372,7 @@ static const std::vector<RPCResult> TransactionDescriptionString() | |||
| "transaction conflicted that many blocks ago."}, | |||
| {RPCResult::Type::BOOL, "generated", "Only present if transaction only input is a coinbase one."}, | |||
| {RPCResult::Type::BOOL, "trusted", "Only present if we consider transaction to be trusted and so safe to spend from."}, | |||
| {RPCResult::Type::BOOL, "in_mempool", "Only present on unconfirmed transactions."}, | |||
There was a problem hiding this comment.
Suggest saying what the field represents.
| {RPCResult::Type::BOOL, "in_mempool", "Only present on unconfirmed transactions."}, | |
| {RPCResult::Type::BOOL, "in_mempool", "Whether the transaction is in the mempool. Only present if the transaction is unconfirmed."}, |
| psbtx = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo['txid'], "vout": utxo['vout']}], | ||
| {address: amt}, | ||
| 0, | ||
| {"replaceable":True, "feeRate":feeRate})['psbt'] |
There was a problem hiding this comment.
The feeRate option in BTC/kvB is expected to be deprecated; use the fee_rate option in sat/vB
| {"replaceable":True, "feeRate":feeRate})['psbt'] | |
| {"replaceable": True, "fee_rate": fee_rate})['psbt'] |
There was a problem hiding this comment.
It might be clearer to use named arguments here, e.g.:
+ psbtx = self.nodes[0].walletcreatefundedpsbt(
+ inputs=[{"txid": utxo["txid"], "vout": utxo["vout"]}],
+ outputs={address: amount},
+ locktime=0,
+ options={"replaceable": True, "fee_rate": fee_rate},
+ )["psbt"]| @@ -210,5 +212,57 @@ def get_unconfirmed_utxo_entry(node, txid_to_match): | |||
| assert_equal(self.nodes[0].gettransaction(txid_3b)["bip125-replaceable"], "no") | |||
| assert_equal(self.nodes[0].gettransaction(txid_4)["bip125-replaceable"], "unknown") | |||
|
|
|||
| def create_and_send_transaction(self, utxo, address, amt, feeRate): | |||
There was a problem hiding this comment.
naming style nit
| def create_and_send_transaction(self, utxo, address, amt, feeRate): | |
| def create_and_send_transaction(self, utxo, address, amount, fee_rate): |
| utxo = self.nodes[0].listunspent()[0] | ||
| address = self.nodes[0].getnewaddress() | ||
|
|
||
| tx1_id = self.create_and_send_transaction(utxo, address, 0.1, 0.001) |
There was a problem hiding this comment.
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).
| tx1_id = self.create_and_send_transaction(utxo, address, 0.1, 0.001) | |
| tx1_id = self.create_and_send_transaction(utxo, address, amount=0.1, fee_rate=100) |
| @@ -107,6 +107,8 @@ def run_test(self): | |||
| {"txid": txid, "label": "watchonly"}) | |||
|
|
|||
| self.run_rbf_opt_in_test() | |||
| self.test_listtransactions_display_in_mempool() | |||
| self.test_gettransaction_display_in_mempool() | |||
There was a problem hiding this comment.
These two tests have nearly identical setup and could be combined into one.
| assert_equal(tx1['txid'], tx1_id) | ||
| assert_equal(tx1['in_mempool'], False) | ||
| assert_equal(tx2['txid'], tx2_id) | ||
| assert_equal(tx2['in_mempool'], True) |
There was a problem hiding this comment.
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
+ node.generate(1)
+ self.sync_all()
+ tx2 = node.gettransaction(tx2_id)
+ assert_equal(tx2["confirmations"], 1)
+ assert "in_mempool" not in tx2 # only present if txn is unconfirmed|
@danben Do you plan to continue working on this? |
|
🐙 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". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
…icate whether the given transaction is in the mempool. Github-Pull: bitcoin#21260 Rebased-From: 46bf0b7
Added a field to the output of listtransactions/gettransaction to indicate whether a transaction is in the mempool. Closes #21018.