[WIP] Clone transaction before committing to wallet #21
Conversation
|
I think we should get this in for the release after next, so no rush but good to run in testnet for a bit. |
| // Clone transaction to avoid multiple wallets pointing to the same transaction. This can happen when | ||
| // two wallets depend on the same transaction. | ||
| tx = tx.getParams().getDefaultSerializer().makeTransaction(tx.bitcoinSerialize()); | ||
|
|
There was a problem hiding this comment.
I think this couple of lines can be deleted. tx already cloned bellow
There was a problem hiding this comment.
It stores it in riskDropped and returns, that's why it needs to be cloned here.
There was a problem hiding this comment.
ah, you are right.
|
utACK |
|
@sqrrm |
|
@ManfredKarrer I think it's quite clear that having the same tx instance in multiple wallets in bitcoinj is not how it's intended. When transactions are received from blocks they're cloned, see AbstractBlockChain:882. I think the reason they're not cloned when received from the broadcast callback is an oversight from the wallet developer. The connected outputs are only set after a tx enters the wallet from what I understand. This also fits with how the external txs are cloned when added to multiple wallets. I can still understand your hesitation to merge this too soon, but I think the conclusions are wrong regarding the risks and how the bitcoinj wallet works. Since we're already doing the cloning when publishing from the BSQ wallet it's not extremely urgent to fix this, but as mentioned before, I think it's worth fixing before going live on mainnet. |
|
Yes, lets keep it for later. Just don't want to open up risk when I don't see the use case which causes the problem. |
sqrrm
changed the title
|
Related bitcoinj#1553 |
Created a new PR after the discussions in #18