Show transactions as not fully confirmed during background validation #28616

Sjors commented at 9:47 AM on October 9, 2023: member

Fixes #28598: currently if assumeutxo were to be used, transactions might get shown as confirmed even while the entire chainstate is unconfirmed.

This also adds an assumed value to listtransactions etc, shown only for confirmed transactions when a snapshot is loaded.

The UI experience for assume utxo still needs work. This PR makes the wallet UI safer by not rendering transactions as fully confirmed until the background validation is done.

Instead it shows the same icon as when a transaction has 1 confirmation. This is quite conservative. The cost of tricking someone into accepting a fake coin, by giving them a invalid snapshot* and then mining a block on top of it, is higher than a regular doublespend by mining.

pending

* = in the current implementation this involves giving someone a malicious binary or getting them to recompile

Caveat: because transaction state is updated after blockConnected, if you restart the node during background sync, then the transaction will show as fully confirmed ("assumed": false) briefly - until the next block is connected.

DrahtBot commented at 9:47 AM on October 9, 2023: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28616.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

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.

Sjors commented at 9:50 AM on October 9, 2023: member

There's a glitch in this implementation though, probably not a big deal in practice. Because it relies on blockConnected to update the icon, if you restart the node during background sync, then the transaction will show as fully confirmed briefly - until the next block is connected.

Similarly, once background sync finishes, it will show as partially confirmed until a new block comes in. Since we don't show background validation progress yet in the GUI, this glitch would only be noticed by someone looking at the log.

Not sure how involved fixing that is.

maflcko commented at 10:19 AM on October 9, 2023: member

* = in the current implementation this involves giving someone a malicious binary or getting them to recompile

Not sure if this pull request is needed then.

If someone is running a malicious binary or malicious code, the malicious actor will have this code removed from the binary or code.

I think this is only needed when the the utxo hash is added as a config option, so my suggestion would be to close this pull and include it in the pull that makes this an option.

Sjors commented at 12:02 PM on October 9, 2023: member

If someone is running a malicious binary or malicious code

They'd just add code to sweep the wallet.

There's also the (remote) possibility of Bitcoin Core shipping an otherwise valid binary with just a bad hash (accidentally or otherwise).

I think it's reasonable to have some UI differentiator between before and after the background check is finished. There's a difference between your node having validated every block and other people having vouched for the blocks.

That could also be a warning message (which would be followed by a crash if background sync completes and finds a different UTXO set). But this seems a bit less alarmist. Until you receive coins, there's no risk anyway.

in src/wallet/wallet.cpp:1511 in be2be21d17 outdated

1472 | -        return;
1473 | -    }
1474 |      assert(block.data);
1475 |      LOCK(cs_wallet);
1476 |  
1477 | +    switch (role) {

Sjors commented at 12:15 PM on October 9, 2023:

be2be21d1740966325213258151d7f3477b19e1e: if #28608 lands first, this would check role.validated and role.most_work instead (cc @ryanofsky)

in src/wallet/wallet.cpp:1477 in fff5cb42df outdated

1491 | +    if (role == ChainstateRole::BACKGROUND) {
1492 | +        m_background_validation_height = block.height;
1493 | +        return;
1494 | +    } else if (role == ChainstateRole::NORMAL) {
1495 | +        m_background_validation_height = -1;
1496 | +    } else {}

pablomartin4btc commented at 12:08 AM on October 15, 2023:

Conditions were covered by the switch above.

Sjors commented at 2:16 PM on February 12, 2024:

Dropped.

pablomartin4btc commented at 12:19 AM on October 15, 2023: member

Concept ACK

It makes sense to me to add more information regarding assumeutxo when it's enabled or in used, I'm wondering if, apart from the GUI we should also specify it in the rpc command listtransactions output.

in src/wallet/wallet.cpp:3379 in fff5cb42df outdated

3375 | @@ -3376,7 +3376,6 @@ bool CWallet::IsTxAssumed(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(c
3376 |      return false;
3377 |  }
3378 |  
3379 | -

pablomartin4btc commented at 12:25 AM on October 15, 2023:

On thegui: comit fff5cb42dfe91ce1ba1a1aa2269253f262c6e2b4, was this unintentional?

Sjors commented at 2:17 PM on February 12, 2024:

Dropped

pablomartin4btc commented at 12:28 AM on October 15, 2023: member

Small suggestion, maybe the if/ else style format change on src/qt/transactionrecord.cpp in the gui: commit fff5cb42dfe91ce1ba1a1aa2269253f262c6e2b4 should be done on separated or previous commit, so it'd be easier to review the change in the logic (setting the new AssumedConfirmed status).

maflcko commented at 3:31 PM on October 16, 2023: member

I think it's reasonable to have some UI differentiator between before and after the background check is finished. There's a difference between your node having validated every block and other people having vouched for the blocks.

Are you aware of the RPC call to get the state, which can be accessed via the UI. I think it should be obvious to the user that they loaded an utxo set, after they used the loadutxoset RPC call?

No objection to adding something to the GUI, but I don't think the wallet is the right place.

Until you receive coins, there's no risk anyway.

Can you explain this? It is possible to attach a wallet over P2P to a node that has no wallet compiled in. To me, whatever code you are trying to write here, putting it in the wallet doesn't seem right.

Sjors commented at 8:11 AM on October 23, 2023: member

Until you receive coins, there's no risk anyway. Can you explain this?

The only thing a GUI wallet user can do is generate an address. This is safe whether or not IBD and background sync are complete, and regardless of if the snapshot hash was valid. Once they receive coins on that address it's useful to know if these coins are in their mempool, confirmed in a block but pending background sync, or confirmed in a fully validated chain.

attach a wallet over P2P to a node that has no wallet compiled in

There's no p2p message to communicate background validation state.

A wallet that's connected over IPC (see #19461) should figure out it's own handling of this. The way process seperation is designed at the moment, I believe you can only connect the GUI to a remote Node + Wallet combo, so this PR would work. I.e. you can connect a GUI + wallet to a remote node, that would require rethinking the interface.

I also don't think it's a good idea to make the GUI do more independent reasoning, e.g. taking confirmation height from the wallet interface, the background sync state from the node interface and then determining what to display.

Are you aware of the RPC call to get the state, which can be accessed via the UI.

I assume we'll add a GUI menu option to load the snapshot soon enough. At that point relying on users calling an RPC method and interpreting the result, doesn't seem like a good idea. It's also safer to not rely on the user proactively checking if the background sync is done. Rather with this PR, they will see that the transaction is not fully confirmed, and if they're curious they can look at that RPC method.

putting it in the wallet doesn't seem right.

I think we need to access background sync information in two places:

The GUI sync progress indicator(s)
The wallet

(1) is independent of the wallet and not implemented in this PR. I don't think it would be sufficient. We should not expect users to automatically understand that a background sync in progress has implications for a confirmed transaction near the tip.

Sjors commented at 8:13 AM on October 23, 2023: member

also specify it in the rpc command listtransactions output

I'll look into that.

maflcko commented at 8:26 AM on October 23, 2023: member

attach a wallet over P2P to a node that has no wallet compiled in

There's no p2p message to communicate background validation state.

I mean a wallet attached over P2P, not a full node. For example, https://bitcoinops.org/en/topics/transaction-bloom-filtering/ or similar. Not sure if this is still a use case, or if it is possible, but I don't see why not?

Sjors commented at 2:05 AM on November 1, 2023: member

I would expect that a wallet that connects to us over (untrusted) p2p will get the exact same info regardless of background validation progress.

Sjors force-pushed on Nov 7, 2023

in src/wallet/rpc/transactions.cpp:38 in 82f19c7e97 outdated

  35 | +            entry.pushKV("assumed", wallet.IsTxAssumed(wtx));
  36 | +        }
  37 |      } else {
  38 |          entry.pushKV("trusted", CachedTxIsTrusted(wallet, wtx));
  39 | +        // This can happen when loading a wallet from another node,
  40 | +        // while still in (background) IBD:

Sjors commented at 5:33 AM on November 7, 2023:

82f19c7e97281bbd66a25e53f30fe2f80a4f9d11: I found this while loading an existing wallet into a fresh (signet) node, while it was doing the background sync. That's not a very common scenario, so I haven't bothered looking into deeper causes...

in test/functional/feature_assumeutxo.py:17 in 82f19c7e97 outdated

  11 | @@ -12,6 +12,9 @@
  12 |  ## Possible test improvements
  13 |  
  14 |  - TODO: test submitting a transaction and verifying it appears in mempool
  15 | +- TODO: the "assumed" field for a confirmed wallet transaction should be
  16 | +        present during background sync, start false and become true as
  17 | +        sync progresses

Sjors commented at 5:36 AM on November 7, 2023:

82f19c7e97281bbd66a25e53f30fe2f80a4f9d11: such a test seems hard to write without a (background sync aware) stopatheight RPC. So far I just tested manually that the value is correct: absent before loading a snapshot, true while background sync is below it, false once background sync passed the confirmation height.

maflcko commented at 12:53 PM on November 7, 2023:

I think wallet tests should be separate from consensus tests. Also, it would be good to check what happens if loading a wallet file from a backup, whose transactions happened before the utxo snapshot.

maflcko commented at 2:18 PM on November 8, 2023:

Did that for fun (in the wrong test file), and it prints the error message:

test_framework.authproxy.JSONRPCException: Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299 (-4)

Hacky diff:

diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index ab2e6c4d0b..c54c650532 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -48,7 +48,8 @@ COMPLETE_IDX = {'synced': True, 'best_block_height': FINAL_HEIGHT}
 
 
 class AssumeutxoTest(BitcoinTestFramework):
-
+    def add_options(self, parser):
+        self.add_wallet_options(parser, legacy=False)
     def set_test_params(self):
         """Use the pregenerated, deterministic chain up to height 199."""
         self.num_nodes = 3
@@ -143,6 +144,11 @@ class AssumeutxoTest(BitcoinTestFramework):
 
         self.sync_blocks()
 
+        n0.createwallet('test',blank=True, disable_private_keys=True, descriptors=True)
+        w = n0.get_wallet_rpc("test")
+
+        w.importdescriptors([{"desc":"addr(mjTkW3DjgyZck4KbiRusZsqTgaYTxdSz6z)#jdtmxdcm","timestamp":0,"label":"det_coinbase_key_0"}])
+
         # Generate a series of blocks that `n0` will have in the snapshot,
         # but that n1 doesn't yet see. In order for the snapshot to activate,
         # though, we have to ferry over the new headers to n1 so that it
@@ -160,6 +166,8 @@ class AssumeutxoTest(BitcoinTestFramework):
         for n in self.nodes:
             assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)
 
+        w.backupwallet("/tmp/abc.dat")
+
         self.log.info("-- Testing assumeutxo + some indexes + pruning")
 
         assert_equal(n0.getblockcount(), SNAPSHOT_BASE_HEIGHT)
@@ -201,6 +209,8 @@ class AssumeutxoTest(BitcoinTestFramework):
 
         assert_equal(n1.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT)
 
+        n1.restorewallet("hi", "/tmp/abc.dat")
+
         PAUSE_HEIGHT = FINAL_HEIGHT - 40
 
         self.log.info("Restarting node to stop at height %d", PAUSE_HEIGHT)

Sjors commented at 12:45 AM on November 9, 2023:

Interesting that you're getting that error when loading from a backup, but not when loading a regular wallet.

Sjors commented at 1:01 AM on November 9, 2023:

I think wallet tests should be separate from consensus tests.

That makes sense.

Sjors commented at 2:39 AM on November 9, 2023:

Added a wallet test file here: 59796c3926cb146229a2b3e942a5f6994f27d72a

It made me realise we should probably disallow loading a wallet with a birth date below the assume valid block. Though for manual testing it's handy.

Sjors commented at 4:22 AM on November 9, 2023:

I'm still trying to wrap my head around the logic in CWallet::AttachChain which is what triggers the error when loading a backup, but somehow doesn't trigger it when loading an existing wallet. In any case, improving that seems orthogonal to this PR.

My guess is that the backup triggers a rescan from the wallet birth date, whereas loading an existing wallet only rescans from where it last left off. In case of the wallet I tested with, that was already synced well beyond the snapshot height - so it would only rescan from there to the tip. That's probably correct behavior.

Sjors commented at 4:27 AM on November 9, 2023:

Oh I see, the test creates the backup before the snapshot height. It's probably worth testing that as well as a backup that was made after the snapshot height.

Sjors commented at 4:31 AM on November 9, 2023:

As for the behavior that this PR introduces, ideally there would a stopatheight RPC, but the test could also - as it does now - use -stopatheight and restart the node. Since it won't automatically connect to the other test nodes, we can inspect the result of getwallettransactions and see if assumed is set correctly.

I'll work on that and make it a separate commit, so it's a bit easier to extract the test scaffold if this PR doesn't make it.

maflcko commented at 8:50 AM on November 7, 2023: member

I would expect that a wallet that connects to us over (untrusted) p2p will get the exact same info regardless of background validation progress.

assumeutxo allows blocks and the mempool to be based on top of the assumed utxo set, so transactions from those blocks and that mempool can be sent on p2p. I am pretty sure that this mempool does not have "the exact same info regardless of background validation progress."

Sjors commented at 10:52 AM on November 7, 2023: member

There's no way for a node to tell its peers that the mempool transactions its relaying rely on an assumed state. The same goes for block relay. Are you suggesting we should change the p2p protocol to add this information?

maflcko commented at 11:43 AM on November 7, 2023: member

No, I am saying, as explained in my first comment, that this pull request isn't needed. If it was needed, a reason should be given in the pull request description. So far I haven't seen a valid reason, reading the whole thread.

Sjors commented at 12:04 PM on November 7, 2023: member

@maflcko I copy-pasted @luke-jr's comment in the PR description, if that helps?

Sjors commented at 12:31 PM on November 7, 2023: member

Before the background sync is finished, we can only check the proof-of-work for new blocks and a few other rules. It's better than SPV, but not the same as knowing the transaction is in a fully validated block. We should display that difference somehow.

Sjors commented at 1:00 AM on November 9, 2023: member

Rebased to see if the asserts added in #28546 catch anything here. (They don't in the test suite and manual testing).

Sjors force-pushed on Nov 9, 2023

DrahtBot added the label CI failed on Nov 9, 2023

Sjors force-pushed on Nov 9, 2023

DrahtBot removed the label CI failed on Nov 9, 2023

in test/functional/test_runner.py:360 in a68f1596b3 outdated

 338 | @@ -339,6 +339,7 @@
 339 |      'feature_filelock.py',
 340 |      'feature_loadblock.py',
 341 |      'feature_assumeutxo.py',
 342 | +    'wallet_assumeutxo.py --descriptors',

maflcko commented at 10:00 AM on November 9, 2023:

Maybe submit this in a separate pull for review?

pablomartin4btc commented at 10:58 PM on November 25, 2023: member

Since you moved the wallet_assumeutexo.py out into #28838 perhaps you can remove commit a68f1596b358cfed0b80334ef7429d1c5475aa4c from this PR now.

My previous comments regarding some parts of the code were not addressed nor answered if you could please take a look when's possible.

Sjors commented at 1:53 PM on November 28, 2023: member

@pablomartin4btc I'll wait for that PR to get merged and then I'll go through review comments again.

achow101 referenced this in commit 4e104e2381 on Jan 11, 2024

DrahtBot added the label Needs rebase on Jan 11, 2024

fjahr commented at 3:13 PM on January 12, 2024: contributor

* = in the current implementation this involves giving someone a malicious binary or getting them to recompile

Not sure if this pull request is needed then.

If someone is running a malicious binary or malicious code, the malicious actor will have this code removed from the binary or code.

I have been thinking about this concept for a while now and I tend to agree that this is not necessary for similar reasons as mentioned above but I am also not against this being added. However, I don't consider this a blocker for assumeutxo mainnet params. So ~0.

Another question for me is: Why don't we have something similar when assumevalid is used? (We don't, to my knowledge)

Sjors force-pushed on Feb 12, 2024

Sjors commented at 2:32 PM on February 12, 2024: member

Rebased, but not much else changed given the lack of concept-enthusiasm.

Test commit is dropped since #28838 landed.

I don't consider this a blocker for assumeutxo mainnet params

Agreed. If anywhere, it's a blocker for having automatic and/or very easy UI based snapshot loading.

Another question for me is: Why don't we have something similar when assumevalid is used?

That state is permanent, since we don't have a background check for assumevalid.

Small suggestion, maybe the if/ else style format change on src/qt/transactionrecord.cpp in the gui: ... should be done on separated or previous commit

Going to leave that for now, though I agree the change if a bit noisy.

DrahtBot removed the label Needs rebase on Feb 12, 2024

in src/qt/transactionrecord.cpp:178 in 6ccbb968c1 outdated

 174 | @@ -175,42 +175,31 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, cons
 175 |  
 176 |      // For generated transactions, determine maturity
 177 |      if (type == TransactionRecord::Generated) {
 178 | -        if (wtx.blocks_to_maturity > 0)
 179 | -        {
 180 | +        if (wtx.blocks_to_maturity > 0) {

luke-jr commented at 12:04 AM on March 14, 2024:

These lines shouldn't be reformatted for no reason

Sjors commented at 12:29 PM on March 18, 2024:

I'm changing this function, which seems like the right time for it. Per developer guidelines:

When writing patches, favor the new style over attempting to mimic the surrounding style

in src/qt/transactiontablemodel.cpp:324 in 6ccbb968c1 outdated

 319 | @@ -320,6 +320,9 @@ QString TransactionTableModel::formatTxStatus(const TransactionRecord *wtx) cons
 320 |      case TransactionStatus::Abandoned:
 321 |          status = tr("Abandoned");
 322 |          break;
 323 | +    case TransactionStatus::AssumedConfirmed:
 324 | +        status = tr("%1 confirmations, pending verification of historical blocks").arg(wtx->status.depth);

luke-jr commented at 12:06 AM on March 14, 2024:

It's not really the historical blocks being verified as much as it is the snapshot...

Maybe just

        status = tr("%1 confirmations, pending verification").arg(wtx->status.depth);

Sjors commented at 12:35 PM on March 18, 2024:

That would give the user no clue that we're waiting for background sync to finish.

Perhaps "pending synchronization of historical blocks"?

Or "pending synchronization of historical blocks and snapshot verification"?

in src/qt/transactiontablemodel.cpp:468 in 6ccbb968c1 outdated

 467 | @@ -465,6 +468,8 @@ QVariant TransactionTableModel::txStatusDecoration(const TransactionRecord *wtx)
 468 |          return QIcon(":/icons/transaction_0");

luke-jr commented at 12:13 AM on March 14, 2024:

These are weaker too. Normally, there needs to be a valid transaction to even show up - but in this case, it would be pretty cheap to fake it (no need to mine a block, just get them to use the wrong UTXO set).

Sjors commented at 12:40 PM on March 18, 2024:

Do you mean a fake UTXO set which contains a transaction that pays to the user wallet? Presumably that wouldn't show as Unconfirmed, but in this PR it would show as AssumedConfirmed.

Are you suggesting we should hide it entirely? Or some other state?

Note that in the current approach, where we hardcode the valid snapshot, the wrong UTXO set would refuse to load, so the coins would not show up in a wallet. Only a malicious binary would.

luke-jr commented at 2:37 AM on September 4, 2024:

For example, an attacker might give a lot of people a UTXO set with one additional entry, controlled by his key. Then, he can at any point later send that UTXO to anyone who used his modified UTXO set.

Sjors commented at 10:02 AM on October 1, 2024:

@luke-jr that doesn't work, because we'll refuse to load it. The attacker would have to give them a malicious Bitcoin Core binary, but such a binary can just sweep the wallet.

GBKS commented at 1:09 PM on March 20, 2024: none

This PR makes the wallet UI safer by not rendering transactions as fully confirmed until the background validation is done.

Could you please clarify what you mean by safer? Are there specific scenarios you are thinking of?

From a user perspective, the big benefit of assumeUTXO is that they can use their wallet more quickly. If we then show transactions but communicate to users that they should not really trust what is shown, it does not feel like the wallet is actually ready, which defeats the purpose of the feature. But I might be misunderstanding something, so I'd appreciate the clarification. Thanks in advance.

Sjors commented at 10:06 AM on March 21, 2024: member

it does not feel like the wallet is actually ready, which defeats the purpose of the feature

It isn't ready, but it's usable. Before assumeutxo, when a user creates a new wallet and receives coins while the blockchain is still downloading, they would not see those coins. Now they do see the coins, and they can spend them.

But until the background validation is complete, there is a different trust assumption. It's safer than an unconfirmed transaction, because we can check the proof-of-work. It's about the same as an SPV wallet, which only checks the headers and a merkle inclusion proof.

Only if you fully trust that the development process prevented a malicious / buggy snapshot from being included, can you treat it as fully equivalent to the scenario where you completed background validation. But in that case, you might as well not run the background validation at all - yet we do. So I think we should make these transactions (somehow) look a little less final.

GBKS commented at 10:43 AM on March 27, 2024: none

So I think we should make these transactions (somehow) look a little less final.

Thanks for the response. My thinking is really around using the "1 confirmation" icon. A transaction having 1 confirmation does not communicate very closely that the transaction has 7, but that those 7 have not been fully verified. Another treatment could be to show the full verification icon, but grey it out.

We'll also have to resolve this for the Bitcoin Core App.

maflcko commented at 11:01 AM on March 27, 2024: member

But in that case, you might as well not run the background validation at all - yet we do.

There are reasons for the background download. For example, it is needed to populate the block storage with blocks (when pruning is disabled). Also, it is needed to populate the header tree structure with metadata, such as the transaction count of a block, or the transaction count of the chain. Finally, it serves as method to ensure it is always possible to sync from scratch. Otherwise, if snapshots were the popular choice, and they'd skip background download to save on resources, they may encourage to increase the resource usage to a point where snapshot syncs are the only technically possible way to IBD a fresh node.

DrahtBot added the label Needs rebase on Mar 27, 2024

Sjors force-pushed on Apr 2, 2024

Sjors commented at 1:29 PM on April 2, 2024: member

Rebased.

Another treatment could be to show the full verification icon, but grey it out.

That seems reasonable, but I'm not sure how to implement it. It would need to be a flag that's applied to both the TransactionStatus::Confirming and TransactionStatus::Confirmed states. Patch is welcome, but otherwise it could be improved in a followup.

There are reasons for the background download.

I agree there are additional benefits to doing the background sync, beyond verification.

Although this:

populate the header tree structure with metadata, such as the transaction count of a block, or the transaction count of the chain

... could be committed to separately - it's not consensus critical and overkill to have to download an entire block for.

Downloading historical historical blocks is primarily useful to validate them, secondarily to serve them other nodes (for the same reason not everyone should run a pruned node).

may encourage to increase the resource usage

In this case I assume you're referring to future protocol changes that would encourage that? Because within the current protocol use (some) people already don't feel much restraint unfortunately. I suspect time is the worst enemy, with linear growth over centuries being the biggest barrier to background sync. Unless Moore's Law doesn't stop and continues to offset this linear growth with exponential performance increase in bandwidth, CPU and I/O (Utreexo only fixes the I/O bottleneck).

DrahtBot removed the label Needs rebase on Apr 2, 2024

DrahtBot added the label CI failed on Apr 2, 2024

GBKS commented at 9:15 AM on April 11, 2024: none

Not related to the review of this PR, but I thought I'd follow up on my previous comment (let me know if you don't want me to do this). For the Bitcoin Core App, I am proposing to have show the verification status in the text field that we also use for the date and other statuses in transactions. Users can always click a transaction to see the date if they really need to. This keeps the list simple to parse, which is especially important on mobile. The general verification status is indicated in the mini block clock in the top-right (which also has a helpful tooltip), and the larger block clock when you navigate to that screen. More here, and all feedback is appreciated.

GBKS commented at 10:44 AM on September 4, 2024: none

Was just thinking that an alternative approach could be to leave the transactions list as is, but show a message in the send screen that explains the current status and trust assumptions. This is much more explicit than slightly tweaked icons. Just wanted to add it as an idea for an alternative approach, feel free to ignore for the purpose of this PR.

maflcko commented at 9:50 AM on October 1, 2024: member

Are you still working on this? 28.x will be released without this, so it would be good to explain why it should be merged for 29.x (or later), possibly in a release note. I haven't looked if there is any unaddressed feedback waiting for your reply, but at a minimum you'll have to rebase this for a fresh CI run and to pick up the other AU (and other) changes.

wallet: track background validation height 3f3dbb60d1

wallet: add IsTxAssumed() to WalletTxStatus

Determine if a given transaction belongs to a block that is assumed to be valid pending background validation.

Add this information to WalletTxStatus.

a6c53bb244

gui: add assumed confirmed state

Create a separate status for transactions that are confirmed in
a block that is assumed valid pending background validation.

Use the same icon as for transactions with a single confirmation.

3e281590c7

Sjors force-pushed on Oct 1, 2024

Sjors commented at 11:58 AM on October 1, 2024: member

Rebased and added release note. @maflcko once the UI supports loading a snapshot, this PR should be in the same release (or earlier). But there's no PR implementing that yet, so no specific release to target.

I don't think there's any outstanding feedback (minor tweaks in iconography and wording can wait for a followup). Some people find it useful, others don't.

DrahtBot removed the label CI failed on Oct 1, 2024

DrahtBot added the label CI failed on Dec 18, 2024

DrahtBot removed the label CI failed on Dec 22, 2024

fanquake commented at 12:06 PM on January 23, 2025: member

Not sure what to do here. Seems unlikely this is going to go into 29.x, and as far as I'm aware, nobody is working on: "once the UI supports loading a snapshot" in https://github.com/bitcoin-core/gui. Reading the thread, it doesn't really seem like there is buy in for this change from other contributors or reviewers.

Sjors commented at 8:12 AM on January 24, 2025: member

nobody is working on

There is ongoing work in the GUI-QML repo to have snapshot import https://github.com/bitcoin-core/gui-qml/pull/424

But that project is going to take a while, so I'll just make this up for grabs for now.

Reading the thread, it doesn't really seem like there is buy in for this change from other contributors or reviewers.

In that case #28598 should be closed as won't fix.

Sjors closed this on Jan 24, 2025

luke-jr referenced this in commit 2904c57bc4 on Feb 15, 2025

luke-jr referenced this in commit d8a448249a on Feb 15, 2025

luke-jr referenced this in commit f637b331aa on Feb 15, 2025

DashCoreAutoGuix referenced this in commit 2af15a7f64 on Aug 7, 2025

bitcoin locked this on Jan 24, 2026