Remove CWalletTx::vfSpent #3694
Conversation
|
Update: no problems with wallet backup/restore, no problems running with -DDEBUG_LOCKORDER (after fixes unrelated to this pull, see #3704 ). |
src/wallet.h
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A valid case in which long chains of unconfirmed transaction could happen is in case of a reorganize.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right, that's not much of a problem.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@sipa: ACK. Replacing the recursion with:
- if (parent == NULL || !parent->IsTrusted())
+ if (parent == NULL || !parent->IsFromMe() || parent->GetDepthInMainChain() < 0)
return false;
|
Nits picked, rebased. |
src/wallet.h
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Indeed. Maybe if reporters would stop calling me I'd stop making dumb mistakes...
|
.... I'll fix the pull-tester ... just have to figure out a robust way of HOW ... |
src/txmempool.cpp
Outdated
There was a problem hiding this comment.
Can you pass this list by reference into the method, and append to it everywhere, rather than copying it in every recursion step?
There was a problem hiding this comment.
ACK, I'll rewrite to pass the list by reference.
Use the spent outpoint multimap to figure out which wallet transaction outputs are unspent, instead of a vfSpent array that is saved to disk.
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/93a18a3650292afbb441a47d1fa1b94aeb0164e3 for binaries and test log. |
|
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 =) |
|
Merging; I think this is good enough for a release candidate release. |
Remove CWalletTx::vfSpent
5 participants
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.