Show transactions as not fully confirmed during background validation #28616
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28616. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
There's a glitch in this implementation though, probably not a big deal in practice. Because it relies on 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. |
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. |
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. |
| assert(block.data); | ||
| LOCK(cs_wallet); | ||
|
|
||
| switch (role) { |
There was a problem hiding this comment.
be2be21: if #28608 lands first, this would check role.validated and role.most_work instead (cc @ryanofsky)
src/wallet/wallet.cpp
Outdated
| if (role == ChainstateRole::BACKGROUND) { | ||
| m_background_validation_height = block.height; | ||
| return; | ||
| } else if (role == ChainstateRole::NORMAL) { | ||
| m_background_validation_height = -1; | ||
| } else {} |
There was a problem hiding this comment.
Conditions were covered by the switch above.
| if (role == ChainstateRole::BACKGROUND) { | |
| m_background_validation_height = block.height; | |
| return; | |
| } else if (role == ChainstateRole::NORMAL) { | |
| m_background_validation_height = -1; | |
| } else {} |
There was a problem hiding this comment.
Small suggestion, maybe the if/ else style format change on src/qt/transactionrecord.cpp in the gui: commit fff5cb4 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).
src/wallet/wallet.cpp
Outdated
| @@ -3376,7 +3376,6 @@ bool CWallet::IsTxAssumed(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(c | |||
| return false; | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
On thegui: comit fff5cb4, was this unintentional?
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.
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. |
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.
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.
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.
I think we need to access background sync information in two places:
(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. |
I'll look into that. |
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? |
|
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
the
2023/10/assume-unconfirmed
branch
from
fff5cb4 to
9325f1e
Compare
src/wallet/rpc/transactions.cpp
Outdated
| } else { | ||
| entry.pushKV("trusted", CachedTxIsTrusted(wallet, wtx)); | ||
| // This can happen when loading a wallet from another node, | ||
| // while still in (background) IBD: |
There was a problem hiding this comment.
82f19c7: 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...
| @@ -12,6 +12,9 @@ | |||
| ## Possible test improvements | |||
|
|
|||
| - TODO: test submitting a transaction and verifying it appears in mempool | |||
| - TODO: the "assumed" field for a confirmed wallet transaction should be | |||
| present during background sync, start false and become true as | |||
| sync progresses | |||
There was a problem hiding this comment.
82f19c7: 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.
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." |
|
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? |
|
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. |
|
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. |
| @@ -12,6 +12,9 @@ | |||
| ## Possible test improvements | |||
|
|
|||
| - TODO: test submitting a transaction and verifying it appears in mempool | |||
| - TODO: the "assumed" field for a confirmed wallet transaction should be | |||
| present during background sync, start false and become true as | |||
| sync progresses | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)There was a problem hiding this comment.
Interesting that you're getting that error when loading from a backup, but not when loading a regular wallet.
There was a problem hiding this comment.
I think wallet tests should be separate from consensus tests.
That makes sense.
There was a problem hiding this comment.
Added a wallet test file here: 59796c3
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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
the
2023/10/assume-unconfirmed
branch
2 times, most recently
from
be6b4af to
d503d27
Compare
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. |
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. |
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. |
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. |
Sjors
force-pushed
the
2023/10/assume-unconfirmed
branch
from
6ccbb96 to
9040f10
Compare
|
Rebased.
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
I agree there are additional benefits to doing the background sync, beyond verification. Although this:
... 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).
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). |
|
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.
|
|
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. |
|
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. |
Determine if a given transaction belongs to a block that is assumed to be valid pending background validation. Add this information to WalletTxStatus.
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.
Sjors
force-pushed
the
2023/10/assume-unconfirmed
branch
from
9040f10 to
3e28159
Compare
|
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. |
|
Not sure what to do here. Seems unlikely this is going to go into |
There is ongoing work in the GUI-QML repo to have snapshot import bitcoin-core/gui-qml#424 But that project is going to take a while, so I'll just make this up for grabs for now.
In that case #28598 should be closed as won't fix. |
Github-Pull: bitcoin#28616 Rebased-From: 3f3dbb6
Determine if a given transaction belongs to a block that is assumed to be valid pending background validation. Add this information to WalletTxStatus. Github-Pull: bitcoin#28616 Rebased-From: a6c53bb
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. Github-Pull: bitcoin#28616 Rebased-From: 3e28159
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
assumedvalue tolisttransactionsetc, 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.* =in the current implementation this involves giving someone a malicious binary or getting them to recompileCaveat: 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.