Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove CWalletTx::vfSpent #3694

Merged
merged 1 commit into from Feb 28, 2014
Merged

Remove CWalletTx::vfSpent #3694

merged 1 commit into from Feb 28, 2014

Conversation

@gavinandresen
Copy link
Contributor

gavinandresen commented Feb 17, 2014

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

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 18, 2014

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

if (!IsFinalTx(*ptx))
// Transactions not from us: not trusted
const CWalletTx* parent = pwallet->GetWalletTx(txin.prevout.hash);
if (parent == NULL || !parent->IsTrusted())

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 21, 2014

Member

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.

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Feb 21, 2014

Author Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 24, 2014

Member

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

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Feb 24, 2014

Author Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 24, 2014

Member

Right, that's not much of a problem.

This comment has been minimized.

Copy link
@sipa

sipa Feb 24, 2014

Member

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.

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Feb 24, 2014

Author Contributor

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.

This comment has been minimized.

Copy link
@sipa

sipa Feb 24, 2014

Member

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.

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Feb 24, 2014

Author Contributor

@sipa: ACK. Replacing the recursion with:

-            if (parent == NULL || !parent->IsTrusted())
+            if (parent == NULL || !parent->IsFromMe() || parent->GetDepthInMainChain() < 0)
                return false;
@@ -439,7 +446,7 @@ class CWalletTx : public CMerkleTx
const CWallet* pwallet;

public:
std::vector<CMerkleTx> vtxPrev;
std::vector<CMerkleTx> vUnused;// Used to be vtxPrev

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 21, 2014

Member

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

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 24, 2014

Member

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.

This comment has been minimized.

Copy link
@sipa

sipa Feb 24, 2014

Member

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.

This comment has been minimized.

Copy link
@sipa

sipa Feb 24, 2014

Member

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

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 24, 2014

Member

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.

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

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 21, 2014

Member

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.

This comment has been minimized.

Copy link
@sipa

sipa Feb 24, 2014

Member

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

if (mapWallet.count(hash) == 0)
return; // Not one of ours

// Break debit/credit balance caches:

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 21, 2014

Member

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 laanwj added this to the 0.9.0 milestone Feb 24, 2014
@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 24, 2014

Nits picked, rebased.

@sipa
sipa reviewed Feb 24, 2014
View changes
src/wallet.h Outdated
if (!IsFinalTx(*ptx))
// Transactions not sent by us: not trusted
const CWalletTx* parent = pwallet->GetWalletTx(txin.prevout.hash);
if (parent == NULL || !parent->IsFromMe() || parent->GetDepthInMainChain() < 0)

This comment has been minimized.

Copy link
@sipa

sipa Feb 24, 2014

Member

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.

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Feb 24, 2014

Author Contributor

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

@sipa
sipa reviewed Feb 24, 2014
View changes
src/wallet.h Outdated
@@ -534,7 +528,8 @@ class CWalletTx : public CMerkleTx
}

nSerSize += SerReadWrite(s, *(CMerkleTx*)this, nType, nVersion,ser_action);
READWRITE(vtxPrev);
std::vector<CMerkleTx> vUnused; // Used to be vtxPrev

This comment has been minimized.

Copy link
@sipa

sipa Feb 24, 2014

Member

Indentation.

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Feb 24, 2014

Author Contributor

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

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 25, 2014

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

@sipa
sipa reviewed Feb 25, 2014
View changes
src/txmempool.cpp Outdated
remove(txConflict, true);
{
list<CTransaction> r = remove(txConflict, true);
result.insert(result.begin(), r.begin(), r.end());

This comment has been minimized.

Copy link
@sipa

sipa Feb 25, 2014

Member

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

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Feb 26, 2014

Author Contributor

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.
@BitcoinPullTester

This comment has been minimized.

Copy link

BitcoinPullTester commented Feb 26, 2014

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

This comment has been minimized.

Copy link
Contributor

djpnewton commented Feb 26, 2014

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

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 28, 2014

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

gavinandresen added a commit that referenced this pull request Feb 28, 2014
@gavinandresen gavinandresen merged commit f60e49d into bitcoin:master Feb 28, 2014
@gavinandresen gavinandresen deleted the gavinandresen:vfspent branch Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.