The PR branch HEAD was 50fc4df6c at the time of this review club meeting.
Notes
Today’s PR has been extracted from
#16698, which is a project
to improve privacy that involves moving rebroadcast functionality from the
wallet to the node. Having a smaller set of changes is always preferable
since it makes the PR easier to reason about and review. We discussed #16698
at a previous review club.
On current master, the wallet has a method called ResendWalletTransactions
(link).
This method is intended to rebroadcast transactions, but also serves to
ensure a successful initial broadcast. Since only the wallet that originated
the transaction will announce it more than once, there is a potential privacy leak
every time ResendWalletTransactions runs.
Today’s PR reduces the frequency that ResendWalletTransactions runs from
~30 min to every ~day. To do
so, we extract the guarantees around initial broadcast.
Lets clarify some language:
initial broadcast: when a node first notifies the network about a transaction
rebroadcast: when a node sends subsequent notifications to the network
about that transaction
unbroadcast (introduced in this PR): when the initial broadcast of a
transaction hasn’t been deemed successful
In this PR, the mempool keeps track of transactions that it attempts to initially
broadcast. It uses a heuristic of receiving a relevant GETDATA to indicate
a successful initial broadcast. Every 10-15 minutes, it re-attempts broadcast
of the unbroadcast transactions (what a mouthful!)
<jnewbery> reviewing merged PRs is as important as reviewing unmerged PRs (and you get a lot more Bitcoin wizard points for finding a bug in a merged PR)
<robot-visions> you might want to try rebroadcasting the "unbroadcast" ones more eagerly (compared to transactions that HAVE been broadcast to the public network, but not yet mined)
<amiti> by tracking the ones that we dont think have been successfully propagated yet, we can reattempt broadcasting those separately from rebroadcasting transactions that we think have been seen by other nodes
<detoxify> vasild: the privacy improvement is because a network observer can ascertain which node a transaction originated from because it's highly likely a node that's rebroadcsting a txn is the originating node because current default behavior for core is to rebroadcast one's own txn
<lightlike> because we broadcast walltet transactions less often (ideally, in the case that we receive GETDATA for the tx the first broadcast, plus the tx gets mined within a day, only once)
<robot-visions> It makes sense intuitively that "rebroadcast less frequently" helps, but I'm having trouble coming up with a specific attack that gets thwarted.
<jnewbery> robot-visions: I connect to your node and wait. If you INV me the same transaction over and over again, I can be pretty certain that it's in your wallet
<_andrewtoth_> the unbroadcast set factors into this by removing a local tx after it is successfully broadcast, so it doesn't get broadcast to the same peer again
<robot-visions> thanks all! that makes sense. I expect the planned follow up PRs will help even more but I now understand more clearly how this one helps privacy immediately
<robot-visions> sure! "unbroadcast": it's not even in the network yet, so you retry every 10-15 minutes; "rebroadcast": it's already in the network but not yet mined, so you try again every 12-36 hours
<lightlike> MarcoFalke: really? Isn't the most usual case (you get a GETDATA for a tx, plus the tx is mined within a day) covered by this, so that future improvements might only improve privacy in fringe cases (no GETDATA; tx doesnt get mined)?)
<MarcoFalke> lightlike: Yeah, as amiti said it is subjective. It depends at what feerate the wallet creates txs. if they are with a target of 14 days, then the current change doesn't improve that much
<amiti> vasild: its actually a bit more specific than that. the heuristic is that one other node has sent us a GETDATA for this transaction, leading us to believe its probably "in the network"
<jnewbery> A more sophisticated way to determine if a transaction has propagated would be to announce it to some of your peers and wait for it to be INVed by the others
<gleb> If your node is reachable, someone can still easily connect to you issue you that INV etc... Then you probably want several invs from *outbounds*.
<robot-visions> jnewbery: if you send a transaction to a peer in response to `getdata`, will they send an `inv` back for that transaction, or would the per-connection bloom filter prevent it?
<Smeier> it seems like it could allow eclipse attacks to be easier, because a full eclipse isn't necessary. 2-3 nodes could convince yours to stop broadcasting
<jnewbery> _andrewtoth_: maybe? Would that now mean that if you _don't_ announce a tx that everyone else announced, then you're probably the originator?
<amiti> yup correct. I just wanted to highlight that because when this was in the initial stages & part of the bigger 16698 PR I overlooked removing for other reasons
<gleb> Smeier: oh, I'm confusing the two still, joined the meeting too late I guess :) Eclipse is just too much of a strong word for this, you still receive everything from the network and can send other transactions...
<_andrewtoth_> hmm but yes now that I think about it, that could happen. but in that case it is removed from mempool when it's mined, so this PR covers that case
<Smeier> gleb, you're right, censorship is the better term. But currently, to censor a tx, you'd have be all of a node's peers, with jnewbery suggestion, you could do it with just a fe
<gleb> Smeier: I don't think john's idea degrades as compared to the discussed PR approach. One INVs instead of GETDATAs. Invs are a bit easier I guess: they can be send unsolicited.
<lightlike> can someone who knows the lightning network well, tell me how time critical a rebroadcast can be there? Can we lose money if a tx does not get broadcast the first time and we have to wait ~12 hours? If we don't succeed the second time?
<gleb> lightlike: I would say that's probably a responsibility of lightning impl, not Bitcoin Core. Because they may also want to do fee bumping in that case, and our p2p code can absolutely not do that.
<sipa> amiti: actually, i wonder if perhaps your PR does not do this... the wallet rebroadcasting logic applies to all wallet transactions; but only sent transactions (and not received ones) get added to the mempool-based rebroadcasting
<jonatack> gleb: afaik ultimately the LN implementations still need a fuller version of package relay (e.g. changed bitcoin core p2p protocol) to deal with fee management issues when fees rise again
<detoxify> sipa is your point that if you receive a transaction from a peer that the receiver's node will rebroadcast the transaction which will affect unbroadcast/rebroadcast logic?
<sipa> right, i'm saying this PR changed the receiver rebroadcast from frequent to infrequent, without adding a corresponding unbroadcast logic to compensate
<robot-visions> I think I'm having trouble keeping up with the "sender" vs. "receiver" discussion because I'm not clear on what's meant by "a wallet transaction".
<jnewbery> sipa: I'm trying to understand your scenario. I have a Bitcoin node and wallet. I have some other software that connects to the Core node and sends a TX over P2P that spends to my wallet.
<amiti> when I'm tracing with logging, I'll add "abcd" to the start of log prints that I care about, then I'll have a functional test, save the logs, and grep "abcd", to check the code flow
<robot-visions> sipa: just to make sure I understand, your concern is that if a wallet *receives* a transaction, it will still only use the rebroadcast (not the unbroadcast) mechanism?
<robot-visions> sipa: could that result in the unbroadcast set getting very large? for example, by the time a sender receives a transaction, maybe most of its peers have it, so no one will ever send a getdata
<_andrewtoth_> what about the use case where you have a node in your local network that connects to only 1 node that has outside connections. It will never get a GETDATA for a received tx from that one node
<robot-visions> b10c: amiti: If everyone is going to upgrade at the same time, then I'd say yes. But this seems unlikely, so I don't think it's worth the extra complexity to ensure that you can upgrade while keeping your previously persisted mempool.
<MarcoFalke> sipa: AddUnbroadcast should not add the tx to the set. This is a bug right now and will make your node DOS other peers and be uniquely identifiable if it does that. I plan on fixing that.
<sipa> perhaps the solution to all these questions is just the successor... where all nodes start rebroadcasting everything they know that they expect to see confirmed
<jonatack> This PR changes a wallet tx resend timer that hasn't been changed since at least 2011... if ever at all? I would be surprised to not run across a few unexpected effects from this first change to it.
<sipa> so perhaps it's worth just adding to the release notes that transactions submitted locally to the wallet via P2P will not be rebroadcast frequently anymore, which can be circumvented by using sendrawtransaction