wallet: Actually treat (un)confirmed txs as (un)confirmed

MarcoFalke commented at 10:57 am on January 14, 2022: member

The wallet has several issues:

Unconfirmed txs in the GUI

The GUI clumsily attempts to guess if unconfirmed txs are locked until a future time. This is currently based on the locktime only, not nSequence, thus wrong. Fix this by removing the clumsy code and treat all unconfirmed txs as unconfirmed. The GUI already prints whether a tx is in the mempool, in which case the user knows that the tx wasn’t locked until a future time. If the tx is not in the mempool, it might be better to report the exact reject reason from the mempool instead of using incorrect heuristics.

Confirmed txs in the wallet

The wallet drops coins that it incorrectly assumes to be locked until a future time, even if they are already confirmed in the chain. This is because the wallet is using the wrong time (adjusted network time) instead of MTP, due to the -1 default argument of CheckFinalTx.

The issues are fixed in separate commits and there is even a test.

qt: Treat unconfirmed txs as unconfirmed dddd05e7a3

interfaces: Remove unused is_final 888841ea8d

MarcoFalke added the label Wallet on Jan 14, 2022

MarcoFalke added the label Bug on Jan 14, 2022

MarcoFalke force-pushed on Jan 14, 2022

MarcoFalke added this to the milestone 23.0 on Jan 14, 2022

achow101 commented at 5:25 pm on January 14, 2022: member

Why drop checkFinalTx instead of passing in the right flags to use MTP? I think this change would allow coin selection to include UTXOs from non-final txs which is not really safe behavior IMO.

DrahtBot commented at 6:07 pm on January 14, 2022: 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:

#24080 (policy: Remove unused locktime flags by MarcoFalke)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
#15606 (assumeutxo by jamesob)

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.

MarcoFalke commented at 7:12 am on January 15, 2022: member

I think this change would allow coin selection to include UTXOs from non-final txs which is not really safe behavior IMO.

If coin selection were to include coins that are neither confirmed in the chain nor are in the mempool, then that is a separate bug to fix.

luke-jr commented at 10:22 pm on January 15, 2022: member

I think the two issues should be split up. Even if we later add a general “mempool rejection reason” to the GUI, the current intended behaviour tells more information (how many blocks left / when it becomes eligible for confirmation). I would prefer fixing that rather than removing it.

The second issue seems like a strict bugfix, so should get merged faster/cleaner.

MarcoFalke deleted a comment on Jan 16, 2022

MarcoFalke commented at 8:21 am on January 16, 2022: member

Why drop checkFinalTx instead of passing in the right flags to use MTP?

Using the right time for tx.lock_time comparisons would be an improvement, but create even more confusion than it fixes. You’d also have to check nSequence to find non-final txs.

GUI, the current intended behaviour tells more information

Fixing this for the GUI will open up trivial DoS vectors for no benefit. To check for nSequence timelocks, the whole utxo set + mempool needs to be scanned for each input for each unconfirmed wallet transaction. It is already impossible to use a wallet for large tx counts (#16815, #15148, #7475), so I don’t see why we should make that even worse for a feature that has no practical benefit. It is currently not possible to add non-final txs to a wallet, since all wallet txs are added either via the chain, via the mempool, or via the internal wallet itself. In all three cases we know the txs to be final. So for a wallet tx to change to the non-final state, one needs to do a reindex/reorg, in which case the tx will be final once the reindex/reorg has finished. (Unless someone is doing a malicious, potentially large reorg, in which case a mouse-over string in a details window of the GUI should be the least of your problems. In fact you wouldn’t be able to trust it anyway due to the malicious hashrate.) And for the reindex you can already see when the tx was added to the wallet. As you know it was final at the time it was added, adding a string (Open for %n blocks) is just redundant. For example, if your reindex is currently at year 2015 and there is a tx locked to a block height in 2019, adding a string (open for 123123 blocks) doesn’t help the user at all. The user can already tell that 2019 is ahead in the reindex process, which is at 2015.

The second issue seems like a strict bugfix, so should get merged faster/cleaner.

Overall I don’t understand why a questionable feature in the GUI is used as an argument against a patch that is:

a bugfix
only removes code
speeds up the wallet/gui

luke-jr commented at 6:37 pm on January 16, 2022: member

It is currently not possible to add non-final txs to a wallet

Good point. Concept ACK.

(This seems like a bug, but probably non-trivial to fix, and it’s not like the new behaviour is wrong, just less detailed)

ghost commented at 9:39 pm on January 16, 2022: none

Interesting pull request. I was able to reproduce the issue using the below steps manually based on test added in test/functional/wallet_timelock.py:

Run bitcoin-qt (signet) in host machine and virtual machine

Get median time for tip:

 0getbestblockhash
 1
 20000004924aa7d2b938b851b740e5efeb54b4aa1e59a207d1e2a7ad8fddf802d
 3
 4getblockheader 0000004924aa7d2b938b851b740e5efeb54b4aa1e59a207d1e2a7ad8fddf802d
 5
 6{
 7  "hash": "0000004924aa7d2b938b851b740e5efeb54b4aa1e59a207d1e2a7ad8fddf802d",
 8  "confirmations": 1,
 9  "height": 73295,
10  "version": 536870912,
11  "versionHex": "20000000",
12  "merkleroot": "68d904e541209d287566c3bca783ecdbc4edef56c81be602a7c8b38647a26f76",
13  "time": 1642360754,
14! "mediantime": 1642357731,
15  "nonce": 2927517,
16  "bits": "1e016b58",
17  "difficulty": 0.002752172677281328,
18  "chainwork": "000000000000000000000000000000000000000000000000000000ce943d83c0",
19  "nTx": 50,
20  "previousblockhash": "0000012e842e76b7d51e8324109f624c952107a33c592b61b8aa408870256a0b"
21}

Create a raw transaction to send bitcoin from host to VM and use locktime = mediantime -1:

0 createrawtransaction "[{\"txid\":\"cf54ddd4d647073751389dc4e3b41bda6b9c043f59e27e5cc849c22d5eaf7450\",\"vout\":0}]" "[{\"tb1qqs3sdxqhyzkc6n922slxe49r87d6nyvcc705s8\":0.0005}]" "1642357730"

Once the transaction is confirmed observe the inputs available for a transaction in bitcoin-qt based on system time:

unknown approved

unknown commented at 9:42 pm on January 16, 2022: none

tACK https://github.com/bitcoin/bitcoin/pull/24067/commits/fa73381a88d7b650c01f4e098ec68c96f1a4485b This PR fixes the issue described above.

fjahr commented at 5:13 pm on January 22, 2022: member

partial ACK fa73381a88d7b650c01f4e098ec68c96f1a4485b

Partial because I ignored the GUI commit. Otherwise reviewed the code and confirmed that the tests fail on master.

in src/qt/transactionrecord.h:35 in dddd05e7a3 outdated

29@@ -30,8 +30,6 @@ class TransactionStatus
30     enum Status {
31         Confirmed,          /**< Have 6 or more confirmations (normal tx) or fully mature (mined tx) **/
32         /// Normal (sent/received) transactions
33-        OpenUntilDate,      /**< Transaction not yet final, waiting for date */
34-        OpenUntilBlock,     /**< Transaction not yet final, waiting for block */
35         Unconfirmed,        /**< Not yet mined into a block **/
36         Confirming,         /**< Confirmed, but waiting for the recommended number of confirmations **/
37         Conflicted,         /**< Conflicts with other transaction or mempool **/

glozow commented at 12:36 pm on January 24, 2022:

in dddd05e7a3389fcbd90bb4acdfe1f59945d9f381:

IIUC this reduces the possible statuses by eliminating the “Open Until X Block” and “Open Until Y Timestamps” possibilities. So you can also get rid of the qint64 open_for member (on L56)?

MarcoFalke commented at 1:53 pm on January 24, 2022:

Correct. I might do it here if I need to re-push for another reason. Though, there are some other style fixes in the GUI code, which I plan to fixup in a follow-up in order to not hold back a wallet bugfix based on style feedback in gui code.

glozow commented at 10:13 am on January 25, 2022:

Sounds good. Will look out for the followup.

in src/qt/transactionrecord.cpp:193 in dddd05e7a3 outdated

188-        else
189-        {
190-            status.status = TransactionStatus::OpenUntilDate;
191-            status.open_for = wtx.lock_time;
192-        }
193-    }

glozow commented at 12:39 pm on January 24, 2022:

in dddd05e7a3389fcbd90bb4acdfe1f59945d9f381:

Wow Concept ACK, it seems extremely out of place that the gui is trying to clumsily apply consensus rules over here… is there a way for it to ask the node “when will this transaction be final” or?

MarcoFalke commented at 1:48 pm on January 24, 2022:

Currently there is no such function, but someone could write one based on the here removed checkFinalTx. Obviously needs to take into account nSequence as well, instead of just nLockTime.

glozow commented at 10:38 am on January 25, 2022:

Makes sense. Yeah removing for now seems better.

glozow commented at 1:33 pm on January 24, 2022: member

Reviewed first 2 commits. Still trying to understand the wallet part. The test fails on master but it’s hard to find where it’s checking with mocktime instead of MTP?

MarcoFalke commented at 1:50 pm on January 24, 2022: member

The test fails on master but it’s hard to find where it’s checking with mocktime instead of MTP?

Good q. The time is set in CheckFinalTx, see also #24080.

in test/functional/wallet_timelock.py:36 in fa3e73af05 outdated

31+        tx = node.sendtoaddress(ADDRESS_BCRT1_UNSPENDABLE, 5)
32+        tx = tx_from_hex(node.getrawtransaction(tx))
33+        tx.vout = [CTxOut(5000, bytes.fromhex(script_pub_key))]
34+        tx.nLockTime = mtp_tip - 1
35+        tx_hex = node.signrawtransactionwithwallet(tx.serialize().hex())["hex"]
36+        self.generateblock(node, ADDRESS_BCRT1_UNSPENDABLE, [tx_hex])

achow101 commented at 4:56 pm on January 24, 2022:

In fa3e73af050e21b3fbcd7a269f4255abbf6183e3 “wallet: Avoid dropping confirmed coins”

This is way more complicated than it needs to be. We have RPCs that allow the locktime to be set. You can use send with locktime in the options.

MarcoFalke commented at 5:07 pm on January 24, 2022:

Good point, might adjust in the next force push or in a follow-up

MarcoFalke commented at 9:20 am on January 25, 2022:

Thanks, fixed.

achow101 commented at 4:58 pm on January 24, 2022: member

ACK fa73381a88d7b650c01f4e098ec68c96f1a4485b

wallet: Avoid dropping confirmed coins fa272eab44

Remove unused checkFinalTx fac8165443

MarcoFalke force-pushed on Jan 25, 2022

MarcoFalke commented at 9:20 am on January 25, 2022: member

The python test I wrote was too cringe (https://github.com/bitcoin/bitcoin/pull/24067#discussion_r790964097), so I force pushed a simpler version. I left all C++ and Qt code the same.

Should be relatively easy to re-ACK.

unknown approved

unknown commented at 9:29 am on January 25, 2022: none

reACK https://github.com/bitcoin/bitcoin/pull/24067/commits/fac816544317cee6553d60cb0f5f24f6f9ec98de

in src/node/interfaces.cpp:492 in fac8165443

485@@ -486,11 +486,6 @@ class ChainImpl : public Chain
486         const CChain& active = Assert(m_node.chainman)->ActiveChain();
487         return active.GetLocator();
488     }
489-    bool checkFinalTx(const CTransaction& tx) override
490-    {
491-        LOCK(cs_main);
492-        return CheckFinalTx(chainman().ActiveChain().Tip(), tx);

glozow commented at 10:37 am on January 25, 2022:

The time is set in CheckFinalTx

Thanks. Hell, default arguments really are the worst. https://github.com/bitcoin/bitcoin/blob/bd482b3ffebc68130f8a18dabf08ed1aff7ea159/src/validation.cpp#L207-L209

glozow commented at 10:48 am on January 25, 2022: member

code review ACK fac8165443, I understand now how this fixes both issues.

achow101 commented at 9:12 pm on January 25, 2022: member

ACK fac816544317cee6553d60cb0f5f24f6f9ec98de

achow101 merged this on Jan 25, 2022

achow101 closed this on Jan 25, 2022

MarcoFalke deleted the branch on Jan 26, 2022

sidhujag referenced this in commit ee9e27d230 on Jan 28, 2022

Fabcien referenced this in commit 574e3a84ff on Jan 5, 2023

DrahtBot locked this on Jan 26, 2023

wallet: Actually treat (un)confirmed txs as (un)confirmed #24067

Unconfirmed txs in the GUI

Confirmed txs in the wallet

Conflicts