Remove CWalletTx::vfSpent

gavinandresen commented at 4:46 pm on February 17, 2014: contributor

Use the spent outpoint multimap to figure out which wallet transaction outputs are unspent, instead of a vfSpent array that is saved to disk.

This is simpler and more robust and will result in smaller wallet.dat files.

Careful review and testing needed. In particular, I have not (yet) tested importing private keys that were used in the past or that were involved in a mutated transaction scenario.

There might also be an issue with taking a wallet.dat file produced with this change (which does not write vfSpent) and using it with an older version, although I think the old ReacceptWalletTransactions() code will behave reasonably in that case and will automatically recompute vfSpent.

I also have not tested for performance with a very large wallet (a wallet containing a very large number of transactions); I expect somebody running a service who HAS a very large wallet to step up and do that– ideally by writing a new qa/rpc-tests/largewallet.sh regression test that creates and tests a large -regtest wallet.

Motivation for this change: it fixes an edge case where transaction malleability results in spendable coins incorrectly being marked as unspendable.

gavinandresen commented at 6:19 pm on February 18, 2014: contributor

Update: no problems with wallet backup/restore, no problems running with -DDEBUG_LOCKORDER (after fixes unrelated to this pull, see #3704 ).

in src/wallet.h: in 1fa606d93a outdated

683-            const CMerkleTx* ptx = vWorkQueue[i];
684-
685-            if (!IsFinalTx(*ptx))
686+            // Transactions not from us: not trusted
687+            const CWalletTx* parent = pwallet->GetWalletTx(txin.prevout.hash);
688+            if (parent == NULL || !parent->IsTrusted())

laanwj commented at 1:10 pm on February 21, 2014:

Using a recursive IsTrusted call here could run out of stack space if there are a lot of transactions depending on each other in the wallet. I don’t think this is very likely, but when they happen stack overflows are nasty and hard to debug. It was avoided with the workqueue-based approach.

gavinandresen commented at 1:25 pm on February 21, 2014:

Long chains of unconfirmed transactions are a very bad idea; I’d vote for limiting the depth of the recursion to something reasonable. I am very tempted to limit it to a depth of one, and eliminate the recursion all-together.

laanwj commented at 2:08 pm on February 24, 2014:

A valid case in which long chains of unconfirmed transaction could happen is in case of a reorganize.

gavinandresen commented at 2:11 pm on February 24, 2014:

Yes, but IsTrusted() is mostly irrelevant during a re-organize. I suppose if there is a deep re-org underway and you try to spend some coins in the middle of it the spend might fail with “insufficient funds”. Very much an edge case not worth worrying about, though, I think.

laanwj commented at 2:20 pm on February 24, 2014:

Right, that’s not much of a problem.

sipa commented at 2:30 pm on February 24, 2014:

There is no need for this recursion at all. If a transaction is either in the blockchain or in the memory pool, all its dependencies are also in the blockchain or the memory pool.

gavinandresen commented at 3:14 pm on February 24, 2014:

Recursion here is necessary if we want to continue allowing spending of unconfirmed change of unconfirmed change (of unconfirmed change, etc). If there is no recursion, then you will be allowed to spend unconfirmed change once, but will then have to wait for that transaction to confirm (if you have no other inputs in your wallet) before being allowed to spend again.

RE: other nits: ACK.

sipa commented at 4:03 pm on February 24, 2014:

Eh no - you have to check the inputs whether they belong to you. But unconfirmed change of unconfirmed change is in the mempool, and you don’t need to check whether its dependencies do - it being in the mempool guarantees that already.

gavinandresen commented at 6:16 pm on February 24, 2014:

@sipa: ACK. Replacing the recursion with:

0-            if (parent == NULL || !parent->IsTrusted())
1+            if (parent == NULL || !parent->IsFromMe() || parent->GetDepthInMainChain() < 0)
2                return false;

in src/wallet.h: in 1fa606d93a outdated

445@@ -439,7 +446,7 @@ class CWalletTx : public CMerkleTx
446     const CWallet* pwallet;
447 
448 public:
449-    std::vector<CMerkleTx> vtxPrev;
450+    std::vector<CMerkleTx> vUnused;// Used to be vtxPrev

laanwj commented at 1:13 pm on February 21, 2014:

Could move this to a local variable inside serialize() instead of a property.

laanwj commented at 2:10 pm on February 24, 2014:

I just realized that vtxPrev could be useful for re-broadcasting (others) parent transactions in the case of a reorganization. Or is this covered some other way? Right now we store only our own transactions in the wallet.

sipa commented at 2:29 pm on February 24, 2014:

Even now, vtxPrev only stores our own transactions, so it is utterly pointless to keep it.

The correct solution - if we want to deal better with unconfirmed out-of-wallet dependencies - is to add these to the wallet directly instead of inside the transactions they depend on. In practice, these lead to huge duplication of transactions in large wallets. @pstratem has some more context, I believe.

sipa commented at 2:41 pm on February 24, 2014:

I’m in favor of moving this variable to inside the serialization implementations. It will save memory.

laanwj commented at 2:41 pm on February 24, 2014:

OK, agreed, vtxprev is not the way to do this. Storing transactions in the wallet only on the top level (and not nested into other transactions) makes sense.

in src/wallet.h: in 1fa606d93a outdated

621@@ -671,7 +622,7 @@ class CWalletTx : public CMerkleTx
622         int64_t nCredit = 0;
623         for (unsigned int i = 0; i < vout.size(); i++)
624         {
625-            if (!IsSpent(i))
626+            if (!pwallet->IsSpent(COutPoint(GetHash(), i)))

laanwj commented at 2:38 pm on February 21, 2014:

Nit: I can’t help but notice that all calls to pwallet->IsSpent (apart from the one in the UI) construct a COutPoint object specifically to pass to the function. Maybe IsSpent(const uint256&, int n) would be more convenient.

sipa commented at 2:16 pm on February 24, 2014:

Agree. I do believe the C++ compiler avoids the extra copying involved, though.

in src/wallet.cpp: in 1fa606d93a outdated

609     AddToWalletIfInvolvingMe(hash, tx, pblock, true);
610+
611+    if (mapWallet.count(hash) == 0)
612+        return; // Not one of ours
613+
614+    // Break debit/credit balance caches:

laanwj commented at 3:21 pm on February 21, 2014:

The -walletnotify and UI notification dispatch for the transaction update happens in AddToWallet. Ideally we’d want to do this invalidation before the notification is launched to prevent races. One idea would be to move this logic here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet.cpp#L566 where previously the WalletUpdateSpent happened? (execution will get there anyway through AddToWalletIfInvolvingme as fExisted will be true as well as fUpdate)

laanwj added this to the milestone 0.9.0 on Feb 24, 2014

gavinandresen commented at 6:27 pm on February 24, 2014: contributor

Nits picked, rebased.

in src/wallet.h: in 88ee2e5e7f outdated

683-            const CMerkleTx* ptx = vWorkQueue[i];
684-
685-            if (!IsFinalTx(*ptx))
686+            // Transactions not sent by us: not trusted
687+            const CWalletTx* parent = pwallet->GetWalletTx(txin.prevout.hash);
688+            if (parent == NULL || !parent->IsFromMe() || parent->GetDepthInMainChain() < 0)

sipa commented at 8:05 pm on February 24, 2014:

Even that < 0 test isn’t necessary: if this transaction is in the mempool or blockchain, all its parents are too.

We only need to check that all inputs are from us.

gavinandresen commented at 8:53 pm on February 24, 2014:

Indeed. Maybe if reporters would stop calling me I’d stop making dumb mistakes…

in src/wallet.h: in 88ee2e5e7f outdated

527@@ -534,7 +528,8 @@ class CWalletTx : public CMerkleTx
528         }
529 
530         nSerSize += SerReadWrite(s, *(CMerkleTx*)this, nType, nVersion,ser_action);
531-        READWRITE(vtxPrev);
532+	std::vector<CMerkleTx> vUnused; // Used to be vtxPrev

sipa commented at 8:08 pm on February 24, 2014:

Indentation.

gavinandresen commented at 8:36 pm on February 24, 2014:

Fixed (emacs settings were wrong on my new computer….)

gavinandresen commented at 0:09 am on February 25, 2014: contributor

…. I’ll fix the pull-tester … just have to figure out a robust way of HOW … (issue is bitcoind’s still running from previous failed test; ideally tests would be run in a fresh VM so they cannot interfere with each other).

Remove CWalletTx::vfSpent

Use the spent outpoint multimap to figure out which wallet transaction
outputs are unspent, instead of a vfSpent array that is saved
to disk.

93a18a3650

in src/txmempool.cpp: in 0c39205dcd outdated

130             const CTransaction &txConflict = *it->second.ptx;
131             if (txConflict != tx)
132-                remove(txConflict, true);
133+            {
134+                list<CTransaction> r = remove(txConflict, true);
135+                result.insert(result.begin(), r.begin(), r.end());

sipa commented at 8:21 pm on February 25, 2014:

Can you pass this list by reference into the method, and append to it everywhere, rather than copying it in every recursion step?

gavinandresen commented at 3:23 pm on February 26, 2014:

ACK, I’ll rewrite to pass the list by reference.

BitcoinPullTester commented at 5:32 pm on February 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/93a18a3650292afbb441a47d1fa1b94aeb0164e3 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

djpnewton commented at 8:40 pm on February 26, 2014: contributor

I have a reasonably biggish wallet (~7.5K transactions). What is a good performance test for this (what would exercise the right code paths)?

The listaccounts rpc is consistently 40ms faster on my wallet with this branch =)

gavinandresen commented at 8:16 pm on February 28, 2014: contributor

Merging; I think this is good enough for a release candidate release.

gavinandresen referenced this in commit f60e49d49c on Feb 28, 2014

gavinandresen merged this on Feb 28, 2014

gavinandresen closed this on Feb 28, 2014

gavinandresen deleted the branch on Mar 12, 2014

DrahtBot locked this on Sep 8, 2021

Remove CWalletTx::vfSpent #3694