wallet: Only check the hash of transactions loaded from disk

achow101 commented at 9:44 pm on September 18, 2019: member

It feels unnecessary to do a full CheckTransaction for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it.

achow101 renamed this:
~~Only check the hash of transactions loaded from disk~~
wallet: Only check the hash of transactions loaded from disk
on Sep 18, 2019

fanquake added the label Wallet on Sep 18, 2019

in src/wallet/walletdb.cpp:221 in 0d1b7d03c2 outdated

218@@ -219,7 +219,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
219             CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef());
220             ssValue >> wtx;
221             CValidationState state;

MarcoFalke commented at 10:27 pm on September 18, 2019:

please remove the consensus #includes as well

MarcoFalke commented at 12:23 pm on September 20, 2019:

@achow101 ^

promag commented at 1:53 pm on September 20, 2019:

0 #include <wallet/walletdb.h>
1
2-#include <consensus/tx_check.h>
3-#include <consensus/validation.h>
4 #include <fs.h>
5 #include <key_io.h>
6 #include <protocol.h>

achow101 commented at 2:11 pm on September 20, 2019:

Done

promag commented at 11:13 pm on September 18, 2019: member

Concept ACK, this reverts to the original check - just check the hash. This might improve loading time if it’s a big wallet and the majority of transactions have lots of inputs.

promag commented at 11:16 pm on September 18, 2019: member

For reference, the call to CheckTransaction was added here and before was.

DrahtBot commented at 0:54 am on September 19, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15921 (validation: Tidy up ValidationState interface by jnewbery)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

achow101 commented at 10:09 pm on September 19, 2019: member

Using my big wallet environment (same one as in #16910), I did 1000 of my wallet loading benchmark (from #16910 (comment)) and found the mean loading time on master to be 18408.14423 ms, while the mean loading time with this PR is 17870.51767 ms. This is about 0.5 second faster, which is ~3% performance improvement. So removing this check does actually result in faster loads for large wallets. I also did a proper 2 sample t-test to check that the results were statistically significant, and it appears that they are.

MarcoFalke added the label Waiting for author on Sep 20, 2019

Only check the hash of transactions loaded from disk cd68594dcd

achow101 force-pushed on Sep 20, 2019

promag commented at 2:27 pm on September 20, 2019: member

ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149, AFAICT the check is not needed, hash comparison gives data integrity.

MarcoFalke commented at 3:11 pm on September 20, 2019: member

ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149

MarcoFalke removed the label Waiting for author on Sep 20, 2019

MarcoFalke commented at 3:26 pm on September 20, 2019: member

Whenever a consensus-invalid tx has made it into the wallet (e.g. CVE-2010-5139), the db layer is not the right place to deal with it.

Also, stuff like this should probably be tested: https://github.com/bitcoin/bitcoin/blame/f8b0b190aa9d13e93ebfe34b5655dbe8ed8df3d3/src/wallet/walletdb.cpp#L484

luke-jr commented at 11:42 pm on September 20, 2019: member

Whenever a consensus-invalid tx has made it into the wallet (e.g. CVE-2010-5139), the db layer is not the right place to deal with it.

Yet it should probably be dealt with somewhere.

How does the behaviour change here affect it?

(a CVE might not be the best example to use - this would be triggered by any unconfirmed transaction becoming invalid in any softfork)

achow101 commented at 0:14 am on September 21, 2019: member

Whenever a consensus-invalid tx has made it into the wallet (e.g. CVE-2010-5139), the db layer is not the right place to deal with it.

Yet it should probably be dealt with somewhere.

How does the behaviour change here affect it?

(a CVE might not be the best example to use - this would be triggered by any unconfirmed transaction becoming invalid in any softfork)

It looks like even having this check at all doesn’t do anything. All it does is trigger a rescan, but if the transaction is seen during the rescan, it isn’t even updated to be the uncorrupted one found! So all that happens is that a rescan is forced on every start. The offending transactions aren’t removed or touched at all. So really, this check doesn’t even change anything. If we removed it entirely, it still wouldn’t change anything.

Edit: Nope, that was wrong. Just did a test and it looks it does update the tx if it can find it. But for those not in the blockchain or the mempool, they will persist in the wallet unchanged. So actually invalid transactions won’t be effected at all.

If we actually implemented this check correctly, I think it would make most sense to do it in the resending logic as all we actually care about for invalid transactions are those that aren’t in the main chain (either unconfirmed or got reorged out) but remain in the wallet file as rescanning wouldn’t detect anything with those.

achow101 commented at 10:57 am on September 21, 2019: member

Did some further investigating on the invalid transaction thing.

Invalid transactions won’t be shown in any transaction lists, they aren’t added to mapWallet. With this change, such transactions would be added to mapWallet and will thus affect balances and available coins for coin selection.

But an important thing to note is that CheckTransaction doesn’t fully check a transaction for validity so an invalid transaction could already exist in the wallet that passes CheckTransaction.

laanwj added the label Resource usage on Sep 30, 2019

laanwj commented at 10:05 am on October 23, 2019: member

ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149

A hash is enough to check integrity.

I think CVEs and potential softforks that invalidate transactions retroactively can be dealt with when they arise.

laanwj referenced this in commit a884b32854 on Oct 23, 2019

laanwj merged this on Oct 23, 2019

laanwj closed this on Oct 23, 2019

sidhujag referenced this in commit 30d73c1d97 on Oct 26, 2019

jasonbcox referenced this in commit 0df084c088 on Sep 8, 2020

DrahtBot locked this on Dec 16, 2021

wallet: Only check the hash of transactions loaded from disk #16911

Conflicts