Clone transaction before adding to wallet

[sqrrm]

Since PeerGroup can have multiple wallets that all might depend on the
same transaction it was adding the same transaction to multiple wallets.
By cloning the transaction each wallet is guaranteed to have its own,
non shared transactions

[oscarguindzberg]

There were a couple of commits with a workaround to the "same tx shared by multiple wallet" issue:
oscarguindzberg@ba5e10f
oscarguindzberg@5ed2576
oscarguindzberg@6ebcdd5

Have you found a new bug not solved by these workarounds?
Or is this a way to solve the problem for good?

[sqrrm]

I don't think any of those fixes would solve the issue I found. The problem I encountered was that when txA (a tx sending some BTC from BTC wallet to BSQ wallet) was broadcast it was first added in the BTC wallet and on broadcast completion the same txA was added to the BSQ wallet. When spending from the BSQ wallet the spend txB is committed to first the BTC wallet and then to the BSQ wallet. In the commit to the BTC wallet, a txA output used by txB will be tagged as spent and when the BSQ wallet tries to do the same thing the txA output is no longer spendable and the wallet ends up in a bad state.

[oscarguindzberg]

Then, I have 2 suggestions:

• The commits mentioned on my previous comment could be reverted as no longer necessary.
• Every way to add a tx to the wallet should implement this workaround. As an example every call to Wallet.receivePending() and Wallet.receiveFromBlock(). More examples could be found analyzing the different ways to add txs to the wallet.

[sqrrm]

1. Yeah, I think you're right that the transcationConfidence fixes are no longer needed if the txs are never shared by two wallets.
2. Indeed, it's probably better to do the cloning in Wallet.receicePending() to guarantee that no tx is in multiple wallets. For the Wallet.receiveFromBlock() I notice it's already cloned before being added, see AbstractBlockChain:688 so I think the intention is that no tx should be shared between wallets but the internally shared for pending transactions case wasn't handled and not many people have been running with shared wallets like this.

[ManfredKarrer]

@oscarguindzberg I leave that review for you. I will do do a final review as well.

[oscarguindzberg]

I had a closer look.

bitcoinj was not built to work with multiple wallet instances.
A couple of fixes where made, but there is no comprehensive solution.
On one side there is the problem of shared Transaction, TransactionInput and TransactionOutput instances.
But then we have TransactionConfidence instances which live in TxConfidenceTable and there is only one TxConfidenceTable per Context.

So,

• cloning the tx on receivePending() is a step forward. Minor performance improvement: I would move the cloning instruction to be executed after all the checks passed, ie just before commit() is executed.
• I suggested reverting a couple of commits. I was wrong. Even if the Transaction is cloned, TransactionConfidence instance is still shared, so those commits should not be reverted.
• I am evaluating what a comprehensive solution would need. I am looking closely at Wallet's receivePending(), receiveFromBlock(), notifyTransactionIsInBlock(), commitTx(), maybeCommitTx(), reorganize(), receive(), addWalletTransaction(pool, tx) and addWalletTransaction(WalletTransaction wtx) methods to check whether more fixes are necessary. I am thinking whether a bitcoinj refactor is necessary, maybe each wallet may have its own TxConfidenceTable... just thinking out loud. I will post again after I finish my investigation.

[ManfredKarrer]

@sqrrm I think your last commits are wrong (reverts). See @oscarguindzberg's last comment.

[ManfredKarrer]

Cloning a transaction does not clone the connected outputs from the transaction inputs. Not sure if that can become an issue here, btu maybe worth to consider.

@sqrrm Is the only use case where this leads to a problem the one when a user sends BTC from his Bisq BTC wallet to the BSQ wallet? This is not a valid use-case (not possible even as the prefix "B" in the address would not allow sending BTC.
If that is the only use-case I would not add such a high risk change at the current point of development. Adding multi-wallet support to BitcoinJ properly might be a bigger effort and will require lots of testing.

[sqrrm]

@ManfredKarrer The cloning of every incoming tx will ensure that each wallet has unique txs and thus the connected outputs will be unique to that wallet. The current problem is that transactions published by the BTC side of bisq are not cloned if they're relevant to the BSQ wallet. The BSQ transactions are all cloned before being committed and then published inside WalletsManager.publishAndCommitBsqTx().

This proposed change is to make sure we do that from the BTC side as well, and we could also get rid of the explicit cloning in publishAndCommitBsqTx().

I can't think of any current use case but I don't have a complete picture of how the BTC wallet is used. It's possible that this case is the only current issue, but even if it is I think it's worth fixing to make sure we don't get trouble further down the line when we get at atomic BTC/BSQ transactions or something similar.

@oscarguindzberg Good point, better to avoid unnecessary cloning. I will make a new PR without all the reverted commits. I think we should refrain from too much refactoring right now but rather look at doing things slowly and only get this fix in if it's acceptable.

[ManfredKarrer]

@sqrrm Ah ok, that explains why we don't encounted any issues as all BSQ txs are broadcasted by the BSQ wallet and cloned at that step. Also the trade fee txs are cloning the tx for the btc wallet.
So the only use cases are invalid ones (managing to send BTC from Bisq to the BSQ wallet outside the UI).

I agree we should fix that even if it is not a real risk atm. But would prefer to make it at a later point to not add risk now and focus on other more concrete issues.

@sqrrm Is it ok to close that PR?

[sqrrm]

See #21 for new PR