Where to keep TxConfidenceTable if not in Context?

[schildbach]

TxConfidenceTable currently is the point of truth for transaction confidences. It is meant to exist only one time per network. Transaction.getConfidence() is only a proxy (with apparently some issues, see #1769).

Currently, TxConfidenceTable resides in our Context so we have it available whenever we need to. However, this is in the way of making the context optional and used only for testing purposes. Unfortunately, TxConfidenceTable is needed in production, there is quite some logic (and usually UI) depending on it.

So the question is: where could TxConfidenceTable go?

• Passing in TxConfidenceTable references whereever we need them is likely unfeasible, and is probably the reason Context was implemented in the first place. It would affect many methods of the call chain I assume.
• If we keep the table in the Context, but make Context optional at the same time, we would need to auto-create the context if it doesn't exist yet. This would create an empty TxConfidenceTable, possibly unexpected and thus introduce hard to detect bugs/inconsistencies. But this might actually be ok, since TxConfidenceTable has weak refs anyway, and has a 1000 entry cap. So apps might already have to deal with spurious "losses of confidence". Still it doesn't feel good to me.
• We could make TxConfidenceTable a VM-scoped singleton. Since the table is addressed by tx hash, it could actually support multiple networks at the same time (hashes don't overlap). Still it doesn't feel good to me.
• It could go to an object we already pass around everywhere. Network or NetworkParameters come to mind, but we want to keep Network immutable and get rid of NetworkParameters eventually. Is there some omnipresent object left I oversee?
• We could also try to undo the introduction of TxConfidenceTable (ages ago) such that the individual transactions keep track of confidence again. But in this case we might get rid of the confidence depth first, see Rethink WalletEventListener.onTransactionConfidenceChanged #877 and WIP: Get rid of depth in TransactionConfidence. #1553. Also, we might need to make sure tx instances only exist once per transaction, otherwise we'd have multiple confidences for the same tx. Our quest to make txns immutable might come in handy here?

This ticket is meant for discussing the various options.

[msgilligan]

Why not PeerGroup? It looks like it may have been there originally:

    /** Use "Context.get().getConfidenceTable()" instead */
    @Deprecated @Nullable
    public TxConfidenceTable getMemoryPool() {
        return Context.get().getConfidenceTable();
    }

bitcoinj/core/src/main/java/org/bitcoinj/core/PeerGroup.java

Lines 1642 to 1646 in 0645a83

	/** Use "Context.get().getConfidenceTable()" instead */
	@Deprecated @Nullable
	public TxConfidenceTable getMemoryPool() {
	return Context.get().getConfidenceTable();
	}

[schildbach]

Why not PeerGroup?

Good idea! However a PeerGroup doesn't always exist. One can instantiate a single Peer and receive events which should (potentially) have an effect on transaction confidences.

[msgilligan]

Good idea! However a PeerGroup doesn't always exist.

It could be possible to share it between Peers in a PeerGroup and fallback to a private copy in a standalone Peer.

[schildbach]

Today, I investigated into the option to put TxConfidenceTable into PeerGroup. The Peer <> PeerGroup issue could be solved via listener (Peer broadcasts a "confidence event" up to PeerGroup, which manages confidences). Or maybe one of the existing event would match the problem already.

Anyway, confidence is also needed in places where PeerGroup is far away, like Transaction and Wallet, and more. For those I don't see an easy solution. Except for using Network or NetworkParameters which is available almost everywhere.

[msgilligan]

confidence is also needed in places where PeerGroup is far away, like Transaction and Wallet, and more.

How about a "Blockchain" object? (perhaps AbstractBlockChain or maybe an interface that AbstractBlockChain implements)

Wallet doesn't explicitly "have-a" blockchain, but effectively they do (wallets are added to blockchains not vice-versa, but in order for a wallet to know about unconfirmed transactions and transaction confidences it must be attached to a blockchain.) So there should be a way for a wallet to find its attached blockchain (if any) and access transaction confidence information from there.

Transaction IMO shouldn't have a dependency on confidence objects. (Aside: I think we probably want to split transaction into approximately three objects: TransactionData, TransactionMessage, and WalletTransaction.) The immutable transaction object (TransactionData?) that we are trying to move to should certainly not have direct dependencies on confidence information or the TxConfidenceTable. (See #2547 (comment))

[schildbach]

How about a "Blockchain" object?

Note that "seen" is not a product of the blockchain, but of the P2P network. Of course the inverted argument is true for confidence in PeerGroup. Maybe it's a mistake to try and consolidate those two confidences (P2P based, blockchain based) in one TransactionConfidence object? Maybe we should split them and see if there is much (if any) communication between these two types? Wallet apps could use a convenience wrapper to re-bundle the confidences into one.

Transaction IMO shouldn't have a dependency on confidence objects.

Transactions currently have some logic that depends on confidence. This then has to go elsewhere.

[msgilligan]

How about a "Blockchain" object?

Note that "seen" is not a product of the blockchain, but of the P2P network.

I guess I consider the mempool (or a partial mempool or a partial list of pending transactions) to be a property of a blockchain (or a blockchain network.) If we don't want that to be part of the AbstractBlockchain hierarchy we could define a new object for this concept, I suppose.

I also think we want to abstract from PeerGroup where sensible for use cases like custodial wallets, non-custodial web-wallets, etc.

Maybe it's a mistake to try and consolidate those two confidences (P2P based, blockchain based) in one TransactionConfidence object? Maybe we should split them …

I don't think splitting should be necessary.

Transaction IMO shouldn't have a dependency on confidence objects.

Transactions currently have some logic that depends on confidence. This then has to go elsewhere.

Yes, I think we want to move that elsewhere anyway. (And perhaps "elsewhere" is a WalletTransaction object)

[micheal-swiggs]

• So I'm guessing that we need to remove Transaction.getConfidence() somehow?
• The logic in Peer for updating TxTableConfidences could be moved out to a listener...

[msgilligan]

• So I'm guessing that we need to remove Transaction.getConfidence() somehow?

Yes. I think we should do this after creating a transaction interface and multiple (I propose 3) implementations: one for raw transaction data, one for transaction messages, and a third for wallet transactions. (There might also need to be some kind of transaction builder.) Our current transaction is used for all three roles and this (in my opinion) results in too much complexity and too many dependencies in one object.

It might be OK for to have a .getConfidence() method on a wallet transaction object.

I think we also need to define an object or interface that represents blockchain + mempool state. Currently this full state seems to be in a combination of existing objects including Context. I suppose this new object might also be limited to tracking the state of transactions for a single wallet (or maybe keychain, keybag, etc) See also #1874 about creating a wallet interface and wallet module.

• The logic in Peer for updating TxTableConfidences could be moved out to a listener...

I'd need to look at this a little more closely, but this could be a step we could take before deciding on or implementing some of the larger architectural changes/refactorings that I am proposing.

[micheal-swiggs]

I think we also need to define an object or interface that represents blockchain + mempool state. Currently this full state seems to be in a combination of existing objects including Context. I suppose this new object might also be limited to tracking the state of transactions for a single wallet (or maybe keychain, keybag, etc) See also #1874 about creating a wallet interface and wallet module.

I somewhat agree with this. I mean specifically for confidences I'm guessing we are only interested in confidences for transactions in our wallets. For transactions already past the EVENT_HORIZON there is no need to track them. That leaves the transactions outside of the wallets and before the EVENT_HORIZON, I guess these aren't really supported today and don't need to be? Maybe no one is using BitcoinJ to track transactions outside of wallets...

[msgilligan]

our wallets

Maybe no one is using BitcoinJ to track transactions outside of wallets...

Remember that a bitcoinj Wallet is currently a non-custodial SPV wallet that is serialized via ProtoBuf. There are already people (including me) who are using bitcoinj to implement other types of wallets and non-wallet applications.

One example would be a light wallet that uses a server to track transaction confidence but uses bitcoinj objects in its GUI and a bitcoinj HD keychain for signing transactions.

I can definitely imagine other wallets (or blockchain explorer apps) that might want to track transaction confidence.

[micheal-swiggs]

I can definitely imagine other wallets (or blockchain explorer apps) that might want to track transaction confidence.

Completely agree with this. I guess I was a bit concerned that the new implementation might only track confidences for transactions in a single wallet (or maybe keychain, keybag, etc) but clearly we are going for something as generic as the current implementation.

[micheal-swiggs]

Just having a browse and there seems like for the time being at least that TxConfidenceTable could be made a Singleton on itself? It uses basic construction within Context... For future uses I don't think this would work since the calculation for depth would introduce a dependency to AbstractBlockChain so that we could get the current block height. The current block height could be captured by either making TxConfidenceTable a NewBestBlockListener or making AbstractBlockChain.getChainHead().getHeight() a Supplier<Integer> for TxConfidenceTable. My preference is for the second approach.

[msgilligan]

I haven't forgotten about this issue and am hoping we can move forward with it eventually.

• WIP: Comment out use of TxConfidenceTable in Block.readTransactions() #3189 is a WIP (or proof-of-concept) that shows how the current implementation can really slow down (by 30x) a test/application that tries to read a complete blockchain.
• TxGetConfidence: getConfidence, newConfidence private methods #3190 is a minor refactoring that may make further changes easier.

I also think the refactoring and simplification we've been doing for the 0.17 will help with refactoring TxConfidenceTable.

[msgilligan]

I submitted more related PRs:

• PR Wallet: add getConfidence(tx) method and use it in waitForConfirmations #3194
• PR TestFeeLevel, WalletTest: use wallet.waitForConfirmations() #3195
• PR TransactionConfidence: make broadcastBy, listeners private #3196
• PR TransactionConfidence.Factory: make package private #3199
• PR TransactionConfidence.Factory: convert to interface #3200
• PR TransactionConfidence: store overriding txId (not Transaction ref) (minor BREAKING) #3201