Mempool tracks locally submitted transactions to improve wallet privacy (
p2p) Apr 29, 2020
The PR branch HEAD was 50fc4df6c at the time of this review club meeting.
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
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
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!)
Did you review the PR?
Concept ACK, approach ACK, tested ACK, or
(Don’t forget to put your PR review on GitHub.) How does the unbroadcast set work?
What transactions get added to the set?
When are transactions removed from the set?
What happens to transactions in the set?
What are possible failure scenarios?
Why do we only do this for our own transactions? Is there a potential
privacy leak? How does this compare to current behavior in master?
mempool.dat be versioned?
1 13:00 <amiti> #startmeeting
2 13:00 <amiti> hi everyone! welcome to another bitcoin core review club
3 13:00 <Anonymous93> hey there!
5 13:00 <amiti> feel free to say hi and let everyone know you are here :)
6 13:00 <willcl_ark> hi amiti
7 13:00 <robot-visions> hi
19 13:01 <amiti> the PR just got merged this morning, but that doesn't mean we should stop reviewing and thinking critically about it :)
20 13:02 <amiti> who has gotten a chance to review the PR? (y/n)
21 13:02 <robot-visions> y
33 13:03 <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)
34 13:03 <amiti> thanks to those of you that reviewed, and thanks to those of you that haven't but have joined today :)
35 13:04 <amiti> lets get started by talking about what is the unbroadcast set?
36 13:04 <amiti> what is the point of it?
37 13:05 <_andrewtoth_> the point of it is to track transactions submitted locally that have not yet been broadcast to the public network
40 13:06 <Smeier> to reduce the necessity of rebroadcasting all txs?
41 13:06 <willcl_ark> to improve privacy
43 13:06 <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)
44 13:06 <amiti> yup! all correct
45 13:06 <vasild> "to improve privacy" - how?
47 13:07 <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
49 13:07 <Smeier> Reading some of the conversations, I was confused on whether this is a privacy positive or not
50 13:07 <amiti> great! lets get into it
51 13:08 <amiti> whats the claim for why this PR helps privacy
52 13:08 <amiti> and how does the unbroadcast set factor in?
53 13:08 <instagibbs> currently nodes only re-broadcast the same tx to same peer if it's committed to in their wallet
54 13:08 <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
55 13:09 <gloria43> ^ so if you ever see a node telling you about the same tx twice, it must originate from them?
56 13:09 <detoxify> PR is to reduce the amount of data a network observer has to make conclusions about txn origin
57 13:09 <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)
58 13:09 <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.
59 13:09 <amiti> everyone is correct :D
60 13:10 <sipa> robot-visions: gloria43 just described it
61 13:10 <raj_> How the nodes decide "this one hasn't been propagated successfully, should try rebroadcasting"?
62 13:10 <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
64 13:10 <instagibbs> robot-visions, snoop can sit and not relay, just wait for you to say it again
65 13:11 <vasild> Do we ever re-broadcast transactions that did not originate at us in a sole attempt to fool an observer? I guess no.
66 13:11 <instagibbs> vasild, no
67 13:11 <Smeier> raj_, if your node receives a getdata request, you consider it broadcast
68 13:11 <_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
69 13:11 <detoxify> this is an anti-Eve measure
70 13:11 <robot-visions> jnewbery: sipa: so reducing to 24 hours makes this attack more expensive (you have to wait 1 day instead of 30 minutes?)
71 13:11 <amiti> ok. so the point of the unbroadcast set is so the _mempool_ can track transactions and do a best-effort of getting them to the network
72 13:11 <detoxify> robot-visions more expensive or obviates its possibility entirely
73 13:12 <sipa> also note that you need multiple connections
74 13:12 <amiti> this allows the _wallet_ to attempt rebroadcasting _wallet_ transactions less frequently
75 13:12 <sipa> within one connection there is a bloom filter that keeps track of that the other side already knows
76 13:12 <sipa> which filters out duplicate announcements
78 13:12 <jnewbery> sipa: how long does that filter take to refresh?
79 13:12 <Smeier> so, does a given node never rebroadcast foreign txs?
80 13:13 <Smeier> and why not?
81 13:13 <MarcoFalke> robot-visions: Yes. And the tx could have been mined in those 24 hours
82 13:13 <amiti> if I remember correctly, I did some rough calculations that bloom filter can take ~6-12 hours to refresh. but low confidence on that.
83 13:13 <MarcoFalke> Smeier: See the link jnewbery shared
84 13:13 <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
85 13:14 <lightlike> i agree. if the tx has a reasonable miner fee, the attack should be impossible, not just more expensive.
86 13:14 <amiti> 16698 pulls out the *rebroadcast* functionality from wallet to mempool, so *all* nodes will start rebroadcasting some transactions
87 13:14 <elichai2> Do I understand correctly that the big privacy improvements aren't in this PR but will come later?
88 13:15 <MarcoFalke> elichai2: Yes, most of it
89 13:15 <MarcoFalke> Though, this one is also an improvement, I'd say
90 13:15 <amiti> elichai2: I think its subjective
91 13:15 <detoxify> lightlike +1
92 13:15 <amiti> I think this one is a significant improvement, esp in current mempool conditions
93 13:15 <amiti> but also that the next step will be even stronger privacy guarantees
94 13:15 <elichai2> But it sounds like this one actually increases the number of times it rebroadcasts it's txs
95 13:16 <MarcoFalke> elichai2: no, why?
96 13:16 <gloria43> in this PR, is the originating node still the only one rebroadcasting?
97 13:16 <amiti> not quite, to clarify this can somebody explain the difference between "unbroadcast" and "rebroadcast" ?
98 13:16 <elichai2> Oh I'm sorry, I misread
99 13:16 <sipa> gloria43: yes
100 13:16 <elichai2> I thought the old is 1/day and now it's 1/15min but it's the opposite :)
101 13:16 <gloria43> thanks for clarification!
102 13:16 <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
103 13:17 <amiti> robot-visions: very nice!
104 13:17 <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)?)
105 13:17 <Smeier> but only your own txs can be unbroadcast right?
106 13:17 <elichai2> so this is definitely already a privacy improvement
107 13:18 <Smeier> this PR would never label foreign Txs as unbroadcast, because they had to come from outside?
108 13:18 <vasild> "in the network" is a bit fuzzy, what does it mean? "at least one other node has the tx"?
109 13:18 <robot-visions> "in the network" means you received a getdata for that transaction
110 13:18 <willcl_ark> so this PR -> "less chance of being identified" vs "quite likely to be identified"
112 13:18 <amiti> smeier: yes
113 13:18 <amiti> willcl_ark: I don't follow
114 13:18 <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
115 13:18 <Smeier> amiti: thanks
116 13:19 <_andrewtoth_> is only 1 GETDATA enough? Should it wait for more than 1 to prevent it being blackholed?
117 13:19 <Smeier> does the "should've been mined" heuristic consider fee rate?
118 13:19 <amiti> _andrewtoth_: great question! one that I've asked myself a lot.
119 13:19 <amiti> lets go through step by step of how transactions interact with the unbroadcast set
120 13:20 <amiti> 1. how do transactions get added to the set?
121 13:20 <vasild> so, "in the network" indeed means "at least one other node has the tx"
122 13:20 <Smeier> they are locally submitted
123 13:20 <amiti> smeier: correct. what does that mean?
124 13:20 <_andrewtoth_> either from rpc sendrawtransaction or sent by the wallet
125 13:21 <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"
126 13:21 <amiti> so, very much a heuristic
127 13:21 <amiti> and the question _andrewtoth_ was asking is ... is this heuristic sufficient ?
128 13:21 <gleb> we still do best effort to send to every peer, right?
129 13:22 <gleb> heuristic just helps to not do it *again*
130 13:22 <amiti> which is a *really important* question to be asking. because if its insufficient, transactions might not make it out to the network
131 13:22 <Smeier> the best effort is changed to 12-36 hrs I believe
132 13:22 <_andrewtoth_> gleb that's my understanding
133 13:22 <detoxify> gleb same
134 13:22 <_andrewtoth_> it's removed on first GETDATA, but it was announced to all peers and their GETDATA's could come in after
135 13:22 <amiti> gleb: yes, but there can be some edge use cases that cause failures
136 13:23 <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
137 13:23 <amiti> jnewbery: agreed. that seems very robust
138 13:24 <Smeier> is that planned in future PRs?
139 13:24 <amiti> ok, so "locally submitted" transactions get added to the set, which means they were submitted via the wallet or via the rpcs
140 13:24 <_andrewtoth_> jnewbery: could that have privacy improvements as well? Not all your peers will know you've originated it
141 13:25 <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*.
142 13:25 <amiti> not currently planned. would probably have privacy improvements. would have to be implemented very carefully, but probably possible
143 13:25 <amiti> I considered it when designing this PR, but decided to make incremental improvements
144 13:25 <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?
145 13:26 <amiti> what is another way transactions can be removed from the unbroadcast set?
146 13:26 <sipa> robot-visions: that certainly gets filtered
147 13:26 <amiti> (other than on the first GETDATA)
148 13:26 <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
149 13:26 <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?
150 13:26 <sipa> amiti: i assume... by the transaction leaving the mempool?
151 13:26 <robot-visions> amiti: I think transactions could also be removed if they get mined, expired, etc.
152 13:26 <b10c> amiti: transaction is confirmed?
153 13:27 <raj_> when its removed from mempool.
154 13:27 <gleb> Smeier: I won't call it eclipse. Probably censorship (and only of rebroadcast tx). But yeah.
155 13:27 <amiti> sipa: I like how you say you "assume" when you reviewed the pr 😂
156 13:27 <robot-visions> sipa: thanks, makes sense
157 13:27 <sipa> amiti: i just woke up :)
158 13:27 <sipa> but i'm actually going to check now that it does that
159 13:27 <_andrewtoth_> this won't be removed if it is mined, because if it is in unbroadcast set it was never sent to miners
160 13:27 <Smeier> Gleb why wouldn't this affect unbroadcast?
161 13:27 <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
162 13:28 <_andrewtoth_> jnewbery: they would have to have a lot of knowledge of peer topography to determine that no?
163 13:28 <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...
164 13:28 <jnewbery> possibly. I'm just throwing stuff out there :)
165 13:29 <b10c> _andrewtoth_: it could have reached the miner not from our node
166 13:29 <robot-visions> _andrewtoth_: good point, I suppose that can't usually happen unless YOU are the miner or some other edge case
167 13:29 <_andrewtoth_> b10c: how if it's a locally submitted transaction?
168 13:29 <b10c> _andrewtoth_: if I submit it on e.g. two nodes
169 13:29 <robot-visions> _andrewtoth_: maybe someone submitted it in multiple places?
170 13:29 <_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
171 13:29 <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
173 13:30 <jnewbery> _andrewtoth_: One example: you co-signed a transaction with someone. They submit it and you submit it
174 13:30 <sipa> in general both the sender and receiver will rebroadcast
175 13:31 <amiti> ok cool, moving forward..
176 13:31 <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.
177 13:31 <_andrewtoth_> right, lots of cases where it could be mined but still in the unbroadcast set
178 13:31 <amiti> the next two questions we have danced around, but maybe we can dig into it further
179 13:31 <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?
180 13:31 <gleb> lightlike: as low as 2 hours in some cases.
181 13:31 <amiti> ok nevermind i'll pause till we dig into these :)
182 13:32 <amiti> one topic being discussed is different heuristics for determining if the unbroadcast txns have been seen by the network
183 13:32 <gleb> lightlike: generally they widely use 144 blocks, but there are some corner cases where they do like 6-12 blocks iirc.
184 13:33 <_andrewtoth_> doesn't it depend on how many hops the payment is sent?
185 13:33 <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.
186 13:34 <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
187 13:34 <gleb> sipa: very cool consideration. I'm actually curious now.
188 13:35 <amiti> sipa: when you say mempool-based rebroadcast, are you talking about implementation in 16698?
189 13:35 <sipa> amiti: i'm talking about the PR that was just merged, 18038
190 13:35 <amiti> or do you mean mempool-based unbroadcast retries in 18038
191 13:35 <_andrewtoth_> if the wallet receives a tx but it doesn't get confirmed in 24 hours, it won't be rebroadcast. Only sent txs
192 13:35 <amiti> oh yes, only the sender will reattempt
193 13:36 <sipa> addition to the unbroadcast set happens in BroadcastTransaction, which only the sender calls
194 13:36 <sipa> i think the receiver should also call it
195 13:36 <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
196 13:36 <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?
197 13:36 <amiti> sipa: I agree only sender reattempts. can you help me understand a use case why receiver should also call?
198 13:36 <jnewbery> sipa: I don't think so. If you've received it then it's already propagated
199 13:37 <jnewbery> this seems like an improvement to me. The receiver won't needlessly rebroadcast
200 13:37 <_andrewtoth_> a use case is if the tx doesn't get confirmed in 24 hours, and the sender is offline
201 13:37 <sipa> amiti: for starters, conceptually, the receiver is the only one who actually cares the transaction confirms
202 13:37 <jnewbery> the receiver would still rebroadcast after 12-36 hours I believe
203 13:37 <Anonymous93> amiti sidetrack, but how did u go about debugging this PR and knowing which files to modify?
204 13:37 <sipa> and the receiver may have learned about the tx out of band
205 13:38 <amiti> jnewbery: yes they'd rebroadcast after 12-36 hours
206 13:38 <amiti> sipa: if the receiver learned about the txn out of band and wants it to confirm, couldn't they just submit it through the RPC?
207 13:38 <MarcoFalke> sipa: Out of band == sendrawtransaction?
208 13:38 <Smeier> sorry, beginner question, but that means all txs are broadcast again regardless of origin every 12-36 hrs?
209 13:39 <MarcoFalke> Smeier: It is a wallet function to do this every couple of hours
210 13:39 <sipa> right, i'm saying this PR changed the receiver rebroadcast from frequent to infrequent, without adding a corresponding unbroadcast logic to compensate
211 13:39 <amiti> smeier: right now only wallet txns will be broadcast again every 12-36 hours
212 13:39 <jnewbery> Smeier: correct. The wallet will rebroadcast all of its unconfirmed transactions (sent or received) every 12-36 hours
213 13:39 <sipa> amiti: or you run local software that used the p2p interface to submit transactions to the receiver's node
214 13:39 <Smeier> including other unrelated ones? or only sent and received ?
215 13:39 <sipa> i agree it's not the most common scenario
216 13:39 <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".
217 13:39 <sipa> Smeier: only wallet-affecting transactions
218 13:39 <robot-visions> Excluding `rawtransaction`, which transactions are eligible for rebroadcast?
219 13:40 <sipa> robot-visions: all wallet transactions
220 13:40 <instagibbs> robot-visions, wallets will rebroadcast *any* transaction stored in the wallet
221 13:40 <sipa> i don't know what rawtransaction has to do with it?
222 13:40 <amiti> sipa: I think in the case where the recipient resubmits via RPC, then they will add to unbroadcast set as well
223 13:40 <instagibbs> robot-visions, this can include a transaction you sent, a transaction you received, a transaction you *watched*
224 13:40 <instagibbs> aka watchonly
225 13:40 <amiti> if they are doing something that uses the p2p interface to submit, then you're right, the functionality has been changed
226 13:41 <jnewbery> sipa: if you receive it from local software using the p2p interface, then it'll get relayed on after it's accepted to the mempool
227 13:41 <Smeier> what should I read to understand the difference between wallet and p2p interface? do they submit txs differently?
229 13:42 <MarcoFalke> Smeier: They call the same function "BroadcastTransaction"
230 13:42 <amiti> in the section where it wants to relay transactions, it iterates through `mapWallet`
231 13:42 <_andrewtoth_> ahh so received txs still get rebroadcast after 24 hours, that's not affected by unbroadcast set
232 13:42 <sipa> MarcoFalke: wallet and RPC both use BroadcastTransaction
233 13:42 <MarcoFalke> jup, I believe so
234 13:42 <sipa> "p2p interface" to submit transactions... that's just the network
235 13:42 <amiti> you can search the code for what transactions get tracked in `mapWallet` :)
236 13:43 <sipa> jnewbery: i don't understand your last sentence
237 13:43 <robot-visions> sipa: instagibbs: amiti: thanks, that helps a lot!
238 13:43 <_andrewtoth_> Smeier: wallet submits locally, rpcs are sendtoaddress and sendmany. p2p uses INV and GETDATA to broadcast txs
239 13:43 <MarcoFalke> sipa: A transaction added to the mempool will get the txid passed to connman for the inv logic
240 13:44 <MarcoFalke> `RelayTransaction` or so
241 13:44 <sipa> sure, for the initial broadcast
242 13:44 <amiti> Anonymous93: "sidetrack" questions are welcome! not sure what you meant though
243 13:44 <MarcoFalke> sipa: But yeah. There is no unbroadcast for wallet txs received on the p2p interface
244 13:45 <Smeier> _andrewtoth_ wallet submits locally but then it calls BroadcastTransaction, correct? and RPC calls the same?
245 13:45 <sipa> i'm not sure it matters a lot, but i feel it's a behavior change this PR introduced that i missed before
246 13:45 <amiti> sipa: what I'm understanding- if a user is submitting a transaction via a p2p interface, the reattempts at broadcast changes with this PR
248 13:46 <_andrewtoth_> Smeier: wallet is controlled by RPC
249 13:46 <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.
250 13:46 <jonatack> Anonymous93: by participating in the review club! (and reviewing PRs and looking at the codebase, over time it starts to make more sense)
251 13:46 <instagibbs> worth a release note probably that wallets are in charge of rebroadcast, even if external
252 13:46 <amiti> thats true whether they are sender / receiver / whatever
253 13:46 <sipa> jnewbery: and say the sender goes offline
254 13:46 <jnewbery> I'm saying that when we receive the TX and it gets accepted to our mempool, we'll relay it on to other peers.
256 13:46 <Anonymous93> amiti how do u go about debugging ur code to make sure the PR works?
257 13:46 <MarcoFalke> Anonymous93: there are tests included
258 13:47 <amiti> anonymous93: ahahhaha I see. well I like functional tests a lot. and usually when I'm iterating I do manual tests as well
259 13:47 <Anonymous93> do u use GDB, just observing and looking?
260 13:47 <amiti> I also like to log things so I can trace through the C++ code and check my expectations
261 13:47 <Anonymous93> understood, thanks MarcoFalke and amiti
262 13:47 <jnewbery> if you want to force something back into the unbroadcast set, you can call sendrawtranscation, right?
263 13:47 <instagibbs> Anonymous93, also depends on the size of change you're making how much you test and how you test
264 13:48 <Anonymous93> cool
265 13:48 <Smeier> on the topic of checking, I checked out this pr branch, and ran make check (as with normal bitcoin core)
266 13:48 <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
267 13:48 <sipa> jnewbery: my point is that that's an unreasonable behavior change, if anything relies on it
268 13:48 <amiti> its a mixture of techniques :)
269 13:48 <Smeier> How can I look at the tests just for this PR?
270 13:48 <sipa> jnewbery: so far, the wallet has done frequent rebroadcasting of every relevant transaction, not just sent ones
271 13:48 <MarcoFalke> sipa: Agree that that should be mentioned somewhere outside this IRC log
272 13:48 <amiti> smeier: check out the diff of this PR & trace down where tests have been introduced, either unit or functional
273 13:49 <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?
275 13:49 <jnewbery> sipa: but I don't see much difference from if we're the sender. We'll relay it once in both cases
276 13:49 <sipa> this has changed to only frequent rebroadcasting of unbroadcast sent transactions, and infrequent rebroadcasting of everything else
277 13:49 <amiti> sipa, marcofalke: any ideas of where I can document?
278 13:49 <amiti> if this was a use case that people relied on, then we could figure out a way to support it
279 13:49 <sipa> it seems trivial to fix
280 13:50 <amiti> oh yeah?
283 13:50 <jnewbery> the only difference is if you're offline when you submit your receiving transaction over your local p2p network
284 13:50 <sipa> call AddUnbroadcastTx in AddToWallet
285 13:50 <fjahr> Anonymous93: sure, i was planning to rework it but collaborating is better for sure. Thanks!
286 13:51 <Anonymous93> fjahr I looked at it and definitely want to help out!
287 13:51 <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
288 13:51 <jnewbery> sipa: then you'll always echo back transactions sent to you. Seems like a step backwards
289 13:51 <robot-visions> s/sender/receiver
290 13:51 <gleb> sipa: not sure the wording Unbroadcast now really applies that well but yeah, i think the idea makes perfect sense.
291 13:52 <sipa> jnewbery: hmm, i see
292 13:52 <amiti> I'll have to think about it more. I agree with jnewbery that in most cases it seems bad for privacy
293 13:52 <Anonymous93> can we propose fixes that we want to see being reviewed? I'm having trouble fixing one of the unit tests.
294 13:52 <gleb> i wish there was a way to distinguish receiving from regular p2p network and semi-out-of-band whatever.
295 13:52 <gleb> Maybe we could remove it once we get a second inv or something...
297 13:53 <b10c> question 5 is "Should mempool.dat be versioned?"
298 13:53 <sipa> but you may first get 147 INVs very rapidly, request a GETDATA from one of them, which arrives after all INVs have already arrived
299 13:53 <b10c> I think it is versioned, isn't it?
300 13:53 <MarcoFalke> jnewbery: Why? there is a bloom filter to prevent that
301 13:53 <amiti> were talking about this use case of "submitting via p2p", what would that hook up into?
302 13:53 <amiti> ahahhahah b10c thank you :)
303 13:53 <sipa> in which case you'll never get an INV again, so it will remain in the unbroadcast set
304 13:53 <sipa> which would be bad
305 13:54 <amiti> it does have a version on it, but it seems like its actually a bit tricky to update the version
307 13:54 <jnewbery> MarcoFalke: yeah, pfilter will catch it
308 13:54 <_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
309 13:54 <b10c> amiti: oh why?
310 13:54 <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.
311 13:55 <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.
312 13:55 <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
313 13:55 <sipa> MarcoFalke: hmm?
314 13:55 <MarcoFalke> *if it the tx is not in the mempool
315 13:55 <sipa> (i was talking about a hypothetical AddToWallet calling AddUnbroadcast - which I agree now would be a bad idea)
316 13:55 <robot-visions> amiti: do you know what happens if you start the updated version with an old `mempool.dat`?
318 13:56 <MarcoFalke> sipa: Even absent that, it is still a bug and exploitable through mempool.dat
319 13:56 <sipa> MarcoFalke: then i don't know what you're talking about; care to elaborate?
320 13:56 <gleb> I try to think of Bitcoin Core wallet as any other wallet. Would we have the same questions about receiving transactions if it is "external"?
321 13:57 <sipa> gleb: this is all solved once all nodes start rebroadcasting everything
322 13:57 <sipa> so maybe we should just defer the solutions to that
323 13:57 <MarcoFalke> sipa: Will submit a fix instead ;)
324 13:57 <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.
325 13:57 <b10c> amiti: thanks! didn't even mean move on, was just curious about that question
326 13:57 <amiti> ok! so looks like we have 3 minutes left
327 13:57 <amiti> does anybody have any questions
332 13:59 <vasild> Why the current code does not treat own txs like foreign txs and rebroadcast everything? To reduce traffic?
333 13:59 <sipa> vasild: that's the next step; one thing at a time
334 13:59 <MarcoFalke> jnewbery: Probably good to add checks to both AddUnbroadcast and ReattemptInitialBroadcast
335 13:59 <jnewbery> vasild: see the next PR in the sequence
336 13:59 <MarcoFalke> jnewbery: "belt-and-suspenders"
337 13:59 <amiti> yes rebrodacasting *everything* would be a significant hit to the bandwidth, so we have to do it carefully
338 13:59 <amiti> thus the next PR
339 13:59 <vasild> sipa: yes, but I mean why it was not done like that in the first place?
340 13:59 <sipa> vasild: ask satoshi?
342 14:00 <jonatack> I'm certainly looking forward to reviewing the next steps.
343 14:00 <vasild> probably he had a slow internet connection :)
344 14:00 <amiti> ok! that is time
345 14:00 <amiti> thanks all for playing!
347 14:00 <jnewbery> thanks amiti!
348 14:01 <robot-visions> thanks!
349 14:01 <instagibbs> would the next PR fit the bill to start thinking about reforming transaction replacement policy?
350 14:01 <vasild> Cheerz!
351 14:01 <amiti> if anybody has outstanding questions, feel free to DM me and I'll try my best to get to them
352 14:01 <_andrewtoth_> thanks amiti!
353 14:01 <b10c> thanks amiti!
354 14:01 <lightlike> thanks!
355 14:01 <gzhao408> thanks amiti!
356 14:01 <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
357 14:02 <willcl_ark> thanks amiti, that was an interesting discussion to watch
358 14:02 <Smeier> thank you!
359 14:02 <amiti> instagibbs: not entirely sure the relationship you're talking about, but I'll take a closer look at the email
360 14:02 <jonatack> sipa: agree, good idea
361 14:02 <amiti> sipa: will do
362 14:02 <instagibbs> amiti, yeah just read it, you'll likely know the answer more than me
364 14:03 <sipa> i think it's mostly unrelated to this work
365 14:03 <jonatack> thanks amiti et al, excellent session
367 14:03 <sipa> the discussion made me review your changes in more depth :)
368 14:04 <amiti> thats awesome!
369 14:05 <amiti> I learned stuff today (and want to continue..)