Show transactions as not fully confirmed during background validation #28616

pull Sjors wants to merge 3 commits into bitcoin:master from Sjors:2023/10/assume-unconfirmed changing 8 files +76 −28
  1. 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.

  2. 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

    For detailed information about the code coverage, see the test coverage report.

    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.

  3. 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.

  4. 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.

  5. 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.

  6. 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)
  7. 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 0:08 am on October 15, 2023:

    Conditions were covered by the switch above.


    Sjors commented at 2:16 pm on February 12, 2024:
    Dropped.
  8. pablomartin4btc commented at 0: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.

  9. 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 0:25 am on October 15, 2023:
    On thegui: comit fff5cb42dfe91ce1ba1a1aa2269253f262c6e2b4, was this unintentional?

    Sjors commented at 2:17 pm on February 12, 2024:
    Dropped
  10. pablomartin4btc commented at 0: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).
  11. 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.

  12. 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:

    1. The GUI sync progress indicator(s)
    2. 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.

  13. 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.

  14. 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?

  15. 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.
  16. Sjors force-pushed on Nov 7, 2023
  17. 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…
  18. 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:

    0test_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:

     0diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
     1index ab2e6c4d0b..c54c650532 100755
     2--- a/test/functional/feature_assumeutxo.py
     3+++ b/test/functional/feature_assumeutxo.py
     4@@ -48,7 +48,8 @@ COMPLETE_IDX = {'synced': True, 'best_block_height': FINAL_HEIGHT}
     5 
     6 
     7 class AssumeutxoTest(BitcoinTestFramework):
     8-
     9+    def add_options(self, parser):
    10+        self.add_wallet_options(parser, legacy=False)
    11     def set_test_params(self):
    12         """Use the pregenerated, deterministic chain up to height 199."""
    13         self.num_nodes = 3
    14@@ -143,6 +144,11 @@ class AssumeutxoTest(BitcoinTestFramework):
    15 
    16         self.sync_blocks()
    17 
    18+        n0.createwallet('test',blank=True, disable_private_keys=True, descriptors=True)
    19+        w = n0.get_wallet_rpc("test")
    20+
    21+        w.importdescriptors([{"desc":"addr(mjTkW3DjgyZck4KbiRusZsqTgaYTxdSz6z)#jdtmxdcm","timestamp":0,"label":"det_coinbase_key_0"}])
    22+
    23         # Generate a series of blocks that `n0` will have in the snapshot,
    24         # but that n1 doesn't yet see. In order for the snapshot to activate,
    25         # though, we have to ferry over the new headers to n1 so that it
    26@@ -160,6 +166,8 @@ class AssumeutxoTest(BitcoinTestFramework):
    27         for n in self.nodes:
    28             assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)
    29 
    30+        w.backupwallet("/tmp/abc.dat")
    31+
    32         self.log.info("-- Testing assumeutxo + some indexes + pruning")
    33 
    34         assert_equal(n0.getblockcount(), SNAPSHOT_BASE_HEIGHT)
    35@@ -201,6 +209,8 @@ class AssumeutxoTest(BitcoinTestFramework):
    36 
    37         assert_equal(n1.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT)
    38 
    39+        n1.restorewallet("hi", "/tmp/abc.dat")
    40+
    41         PAUSE_HEIGHT = FINAL_HEIGHT - 40
    42 
    43         self.log.info("Restarting node to stop at height %d", PAUSE_HEIGHT)
    

    Sjors commented at 0: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.

  19. 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.”

  20. 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?
  21. 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.
  22. 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?
  23. 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.
  24. 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).
  25. Sjors force-pushed on Nov 9, 2023
  26. Sjors force-pushed on Nov 9, 2023
  27. DrahtBot added the label CI failed on Nov 9, 2023
  28. Sjors force-pushed on Nov 9, 2023
  29. Sjors force-pushed on Nov 9, 2023
  30. DrahtBot removed the label CI failed on Nov 9, 2023
  31. 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?
  32. 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.

  33. 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.
  34. achow101 referenced this in commit 4e104e2381 on Jan 11, 2024
  35. DrahtBot added the label Needs rebase on Jan 11, 2024
  36. 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)

  37. Sjors force-pushed on Feb 12, 2024
  38. 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.

  39. DrahtBot removed the label Needs rebase on Feb 12, 2024
  40. 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 0: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

  41. 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 0:06 am on March 14, 2024:

    It’s not really the historical blocks being verified as much as it is the snapshot…

    Maybe just

    0        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”?

  42. 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 0: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.
  43. 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.

  44. 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.

  45. 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.

  46. 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.

  47. DrahtBot added the label Needs rebase on Mar 27, 2024
  48. Sjors force-pushed on Apr 2, 2024
  49. 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).

  50. DrahtBot removed the label Needs rebase on Apr 2, 2024
  51. DrahtBot added the label CI failed on Apr 2, 2024
  52. 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.

    image

  53. 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.
  54. 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.
  55. wallet: track background validation height 3f3dbb60d1
  56. 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
  57. 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
  58. Sjors force-pushed on Oct 1, 2024
  59. 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.

  60. DrahtBot removed the label CI failed on Oct 1, 2024
  61. DrahtBot added the label CI failed on Dec 18, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me