TransactionConfidence caching problem in Transaction

[oscarguindzberg]

• There is a problem of TransactionConfidence caching in Transaction
• TransactionConfidence is identified on TxConfidenceTable.table by tx hash
• Steps to reproduce
  • Create a tx
  • invoke Transaction.getConfidence()
  • Modify the tx somehow so its tx id changes (add an input/output, or modify any of the input/outputs, sign an input, etc)
• Result
  • Transaction.confidence has a reference to a “deprectated” TransactionConfidence.
  • There is an entry in TxConfidenceTable for a tx that is “deprecated”.
• See oscarguindzberg@92a9547
• Some typical use cases for this bug is invoking Wallet.completeTx() - that instantiates a TransactionConfidence to set Source.SELF - and then you sign the tx externally (eg hardware wallet or multisig) or you add an output (eg OP_RETURN).
• If you call Transaction.getConfidence().addEventListener() you will be adding a listener to a “deprecated” TransactionConfidence.
• When peers broadcast back the tx or the tx is included in a block a new TransactionConfidence will be created in TxConfidenceTable.table and its listeners (ie no listeners) will be invoked. The intended user listener won’t be invoked because it is listening on a “deprecated” TransactionConfidence.
• On top of that, empty Transaction instances, as they have the same tx hash, share the same TransactionConfidence, as you can see on oscarguindzberg@b812c03
• Possible solution oscarguindzberg@1a5e2ee but it still has the problem that if you set the confidence source or add listeners and then you update the Transaction, the source/listeners are lost.

[oscarguindzberg]

@schildbach I will wait for your feedback before spending more time on this.

[oscarguindzberg]

About "When peers broadcast back the tx...": From a higher level perspective, the confidence listener problem makes PeerGroup.broadcastTransaction() return a TransactionBroadcast whose "future" is never done.

[oscarguindzberg]

I am working on a solution on https://github.com/oscarguindzberg/bitcoinj/commits/tx-confidence-cache but I am still not sure what would be the best solution (failing tests on that branch exposes that).

From a high level perspective, the problem is once you get a reference to TransactionConfidence, you can not modify the Transaction, otherwise, you are in an inconsistent state.
That is not obvious when you are using bitcoinj.

I evaluated 3 potential solutions:

• Setting Transaction.confidence to null on Transaction.unCache() (see oscarguindzberg@9552da9). This is the simplest solution. The problem is when the TransactionConfidence had already been assigned a source or confidence listeners were already added. The source/listeners are lost.
• Updating TransactionConfidence txId (and TxConfidenceTable.table key) when tx is updated (see oscarguindzberg@c5f2120). This requires more changes on the code. But still fails the usecase when 2 tx are being constructed at the same time (either by the same or different threads)
• I started to think about more radical changes. Maybe we can replace TxConfidenceTable with TxTable, i.e. have a canonicalizing mapping of TransactionConfidence, have a canonicalizing mapping of Transaction. TransactionConfidence objects will just be refenced by their Transaction and all the problems mentioned on this issue will be fixed.
  This also might solve some problems of multiple wallets using the same bitcoinj instance (See Clone transaction before adding to wallet bisq-network/bitcoinj#18).