wallet: indicate whether a transaction is in the mempool #21260
Conversation
danben
force-pushed
the
show-conflicted-transactions-not-in-mempool
branch
2 times, most recently
from
def261d to
ea6dcf6
Compare
|
tACK ea6dcf69d4e42d5dd4636079b31151e260c4b45a. I did tx replacement using That's exactly what I wanted, thank you! |
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
I'm not sure there's a valid use case for this, but here's a code review.
leonardojobim
left a comment
leonardojobim
left a comment
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).
darosior
left a comment
darosior
left a comment
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. |
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Quick look, a few comments below.
|
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 |
danben
force-pushed
the
show-conflicted-transactions-not-in-mempool
branch
3 times, most recently
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. |
danben
force-pushed
the
show-conflicted-transactions-not-in-mempool
branch
3 times, most recently
from
a0b8ba7 to
c5cd7a1
Compare
|
I can't reproduce this failure locally, just wondering if any reviewers have any insights |
danben
force-pushed
the
show-conflicted-transactions-not-in-mempool
branch
from
c5cd7a1 to
74c4326
Compare
|
Fixed, all tests passing now |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
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
left a comment
darosior
left a comment
There was a problem hiding this comment.
Concept ACK (based on shesek's answer to my and luke's concerns).
|
Needs to update the doc of |
danben
force-pushed
the
show-conflicted-transactions-not-in-mempool
branch
from
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.
danben
force-pushed
the
show-conflicted-transactions-not-in-mempool
branch
from
9faa0d9 to
46bf0b7
Compare
|
unsigned cr ACK 46bf0b7 |
ryanofsky
left a comment
ryanofsky
left a comment
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
jonatack
left a comment
jonatack
left a comment
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:
| "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"]| 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) |
|
|
||
| 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
11 participants
Added a field to the output of listtransactions/gettransaction to indicate whether a transaction is in the mempool. Closes #21018.