Remove vtxPrev from CWalletTx #2966

vtxPrev was intended to make it possible to broadcast supporting transactions.

For various reasons it doesn't actually accomplish this goal while still consuming a sizable amount of space in the wallet.

I've removed most references to vtxPrev and replaced them with procedures that pull transactions from mapWallet.

As a side effect this code includes similar performance improvements to CWalletTx::IsConfirmed as #2952

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a80864e8c8fe19101e54287a770769784ddafd17 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.

@pstratem I've forgotten what we've discussed related to this. Do you remember, and is this still the suggested change?

Can you elaborate on "For various reasons it doesn't actually accomplish this goal" ?

I worry that this pull will break a coinjoin implementation that uses the raw transactions API, but from your comment it sounds like that might be broken anyway ( @gmaxwell : have you done any testing with using other people's unconfirmed inputs to build a coinjoin transaction? )

@pstratem, @gmaxwell and I discussed this prior to this pull request.

It seems that currently, vtxPrev is only ever pulled from your wallet, meaning there is no way that something in vtxPrev is something you don't already have. After some digging, we found out this was likely because of the original pay-to-IP protocol, so that unconfirmed dependencies get sent out to receivers automatically (mental note: the payment protocol could use this too). In its current state it is both useless and inefficient (there is a degree of duplication, if you have wallets with many unconfirmed transactions). The solution was to remove it altogether now, and perhaps later re-introduce it when it's useful, in a saner way (without duplication, for starters).

@sipa this is still the suggested change, however at least one bug was revealed and there are likely more that I have not noticed

getbalance conf=0 can be negative if you have transactions which pay to an IsMine address

I just noticed that bug above is actually currently possible to trigger if you happen to have a wallet where vtxPrev has at least one correct entry. Assuming this worked correctly at some point that would be something to fix also.

mental note: the payment protocol could use this too

The payment protocol was designed with this in mind from the start :) That's why you are allowed to submit multiple transactions in a Payment message submission.

Looks good overall,, but a few suggestions:

• Can you rebase? AcceptWalletTransaction is no longer in main, for example.
• It would make things more efficient and shorter, if ListUnconfirmedSupportingTransactions would:
  • Return itself as well, in addition to the dependencies (only if it isn't confirmed already).
  • Would reverse the result already.
  • Would return CWalletTx* objects (as it's called within a cs_wallet, the pointers should remain valid) directly.

There are basically three cases (relay, accept, isconfirmed) where you want to do some operation on the dependencies, and then on the object itself. If the list returned by ListUnconfirmed... already contains the object itself, no code duplicate is necessary for those.

I'm not convinced about the pfCompleteSet semantics yet.

Closing in favor of #3694 which does this as well