wallet: Actually treat (un)confirmed txs as (un)confirmed by maflcko · Pull Request #24067 · bitcoin/bitcoin

maflcko · 2022-01-14T10:57:48Z

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.

achow101 · 2022-01-14T17:25:26Z

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 · 2022-01-14T18:07:27Z

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.

maflcko · 2022-01-15T07:12:10Z

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 · 2022-01-15T22:22:20Z

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.

maflcko · 2022-01-16T08:21:06Z

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 · 2022-01-16T18:37:40Z

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 · 2022-01-16T21:39:59Z

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:

getbestblockhash

0000004924aa7d2b938b851b740e5efeb54b4aa1e59a207d1e2a7ad8fddf802d

getblockheader 0000004924aa7d2b938b851b740e5efeb54b4aa1e59a207d1e2a7ad8fddf802d

{
  "hash": "0000004924aa7d2b938b851b740e5efeb54b4aa1e59a207d1e2a7ad8fddf802d",
  "confirmations": 1,
  "height": 73295,
  "version": 536870912,
  "versionHex": "20000000",
  "merkleroot": "68d904e541209d287566c3bca783ecdbc4edef56c81be602a7c8b38647a26f76",
  "time": 1642360754,
! "mediantime": 1642357731,
  "nonce": 2927517,
  "bits": "1e016b58",
  "difficulty": 0.002752172677281328,
  "chainwork": "000000000000000000000000000000000000000000000000000000ce943d83c0",
  "nTx": 50,
  "previousblockhash": "0000012e842e76b7d51e8324109f624c952107a33c592b61b8aa408870256a0b"
}

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

 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:

ghost

tACK fa73381 This PR fixes the issue described above.

fjahr · 2022-01-22T17:13:48Z

partial ACK fa73381a88d7b650c01f4e098ec68c96f1a4485b

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

glozow

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?

src/qt/transactionrecord.h

src/qt/transactionrecord.cpp

maflcko · 2022-01-24T13:50:57Z

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.

test/functional/wallet_timelock.py

achow101 · 2022-01-24T16:58:41Z

ACK fa73381a88d7b650c01f4e098ec68c96f1a4485b

maflcko · 2022-01-25T09:20:40Z

The python test I wrote was too cringe (#24067 (comment)), so I force pushed a simpler version. I left all C++ and Qt code the same.

Should be relatively easy to re-ACK.

ghost

reACK fac8165

glozow

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

src/qt/transactionrecord.h

glozow · 2022-01-25T10:37:28Z

src/node/interfaces.cpp

-    bool checkFinalTx(const CTransaction& tx) override
-    {
-        LOCK(cs_main);
-        return CheckFinalTx(chainman().ActiveChain().Tip(), tx);


The time is set in CheckFinalTx

Thanks. Hell, default arguments really are the worst.

bitcoin/src/validation.cpp

Lines 207 to 209 in bd482b3

const int64_t nBlockTime = (flags & LOCKTIME_MEDIAN_TIME_PAST)

? active_chain_tip->GetMedianTimePast()

: GetAdjustedTime();

src/qt/transactionrecord.cpp

achow101 · 2022-01-25T21:12:35Z

ACK fac8165

…confirmed

Summary: ``` 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. ``` Backport of [[bitcoin/bitcoin#24067 | core#24067]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12949

MarcoFalke added 2 commits January 14, 2022 11:24

qt: Treat unconfirmed txs as unconfirmed

dddd05e

interfaces: Remove unused is_final

888841e

maflcko added the Wallet label Jan 14, 2022

maflcko mentioned this pull request Jan 14, 2022

wallet: Use MTP for locktime checks, not adjusted time #24038

Closed

maflcko added the Bug label Jan 14, 2022

maflcko force-pushed the 2201-crappyCode branch from faa60bf to fa73381 Compare January 14, 2022 13:35

maflcko added this to the 23.0 milestone Jan 14, 2022

DrahtBot mentioned this pull request Jan 14, 2022

wallet: avoid rescans under assumed-valid blocks #23997

Merged

This was referenced Jan 15, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

assumeutxo #15606

Closed

bitcoin deleted a comment from treadex Jan 16, 2022

maflcko mentioned this pull request Jan 16, 2022

policy: Remove unused locktime flags #24080

Merged

ghost approved these changes Jan 16, 2022

View reviewed changes

glozow reviewed Jan 24, 2022

View reviewed changes

src/qt/transactionrecord.h Show resolved Hide resolved

src/qt/transactionrecord.cpp Show resolved Hide resolved

achow101 reviewed Jan 24, 2022

View reviewed changes

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

MarcoFalke added 2 commits January 25, 2022 10:15

wallet: Avoid dropping confirmed coins

fa272ea

Remove unused checkFinalTx

fac8165

maflcko force-pushed the 2201-crappyCode branch from fa73381 to fac8165 Compare January 25, 2022 09:19

ghost approved these changes Jan 25, 2022

View reviewed changes

glozow reviewed Jan 25, 2022

View reviewed changes

achow101 merged commit e30b6ea into bitcoin:master Jan 25, 2022

maflcko deleted the 2201-crappyCode branch January 26, 2022 06:38

maflcko mentioned this pull request Jan 26, 2022

PR24067 follow-ups bitcoin-core/gui#538

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022

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

ee9e27d

…confirmed

bitcoin locked and limited conversation to collaborators Jan 26, 2023

	const int64_t nBlockTime = (flags & LOCKTIME_MEDIAN_TIME_PAST)
	? active_chain_tip->GetMedianTimePast()
	: GetAdjustedTime();

Conversation

maflcko commented Jan 14, 2022

Unconfirmed txs in the GUI

Confirmed txs in the wallet

Uh oh!

achow101 commented Jan 14, 2022

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

maflcko commented Jan 15, 2022

Uh oh!

luke-jr commented Jan 15, 2022

Uh oh!

maflcko commented Jan 16, 2022

Uh oh!

luke-jr commented Jan 16, 2022

Uh oh!

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

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jan 22, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 24, 2022

Uh oh!

Uh oh!

achow101 commented Jan 24, 2022

Uh oh!

maflcko commented Jan 25, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jan 14, 2022 •

edited

Loading

ghost commented Jan 16, 2022 •

edited by ghost

Loading