bitcoinj
Always use ConfidenceChange listener
If connected to 1 peer, it will be disconnected and reconnected after the broadcast, so we in fact will hear the inv from that peer.
Otherwise, when connected to just 1 peer, the future is never set.
This looks like a better fix for #2050 then reverting e3513964ed2fcbd0f5e9edb6e160094061a46225 -- if it works with all the use cases. @schildbach ?
@msgilligan I am not sure either what would be the best fix. I just tested this fix in regtest with bitcoin core 0.20.1 on localhost and "sometimes" the future is not set (did not investigate the cause yet)
Eventually, I'd like to see two futures in TransactionBroadcast one that indicates that the message was successfully sent to one or more Peers and one that indicates the broadcast message was seen by other peers. See my suggestion in Issue #2050. I'm thinking we could do some refactoring for version 0.17 once we have lambdas/CompletableFuture. It seems reasonable for the API to let clients find the difference between something that failed to send vs. something that failed to relay.
I'll try your patch in my RegTest environment where I first discovered the issue. Maybe we should test on MainNet or TestNet with the single/trusted peer use case. I don't (yet) understand Bitcoin at the P2P layer well enough to understand all the use cases and failure modes.
it will be disconnected and reconnected after the broadcast,
Why is that the case?
it will be disconnected and reconnected after the broadcast,
Why is that the case?
In normal operation bitcoinj disconnects from peers immediately after broadcasting for privacy reasons (to minimize the amount of information a particular peer can gather on the SPV wallet) and should then connect to new peers. My assumption is that this behavior is essentially being replicated in the single-peer case for consistency/simplicity reasons.
@oscarguindzberg Your PR fixes the bug as it has been manifesting in my RegTest environment on my development system.
When I run the ConsensusJ WalletSendSpec test with your branch (./gradlew bitcoinj-rpcclient:regTest --include-build ../bitcoinj) and workaroundBitcoinJ_015_8_Issue = false (this line), the futures complete correctly.
The lines that would otherwise hang are here and here.
In normal operation bitcoinj disconnects from peers immediately after broadcasting for privacy reasons (to minimize the amount of information a particular peer can gather on the SPV wallet) and should then connect to new peers. My assumption is that this behavior is essentially being replicated in the single-peer case for consistency/simplicity reasons.
But it also has the down side to connect to more nodes and increases the chance to connect to a chainanalysis node. I think privacy with bloomfilters is anyway broken to an extent that cannot be fixed. But now we decrease network stability by adding a questionable privacy improvement.
Does the code not consider the use case to be connected to a single localhost node? That is anyway the only case how to not lose privacy.
Indeed the idea is to get rid of the special casing for the 1 peer case. @chimp1984 It's unrelated to bloom filtering, bitcoinj can be run in "full blocks" mode (not to be mistaken with "full validation").
@oscarguindzberg I think this is a great catch! I certainly have missed that 'if' when I came up with a4f241ef29cdc58d9525357716278fa758d4ab4f.
I just merged this. I keep this issue open as a reminder to backport to 0.15.x.