Mempool tracks locally submitted transactions to improve wallet privacy (p2p)

https://github.com/bitcoin/bitcoin/pulls/18038

Host: amitiuttarwar  -  PR author: amitiuttarwar

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!)

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? (Don’t forget to put your PR review on GitHub.)

  2. 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?
  3. What are possible failure scenarios?

  4. 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?

  5. Should mempool.dat be versioned?

Meeting Log

  113:00 <amiti> #startmeeting
  213:00 <amiti> hi everyone! welcome to another bitcoin core review club
  313:00 <Anonymous93> hey there!
  413:00 <vasild> \o/
  513:00 <amiti> feel free to say hi and let everyone know you are here :)
  613:00 <willcl_ark> hi amiti
  713:00 <robot-visions> hi
  813:00 <fjahr> hi
  913:00 <_andrewtoth_> hi
 1013:00 <vasild> hi
 1113:00 <jnewbery> hi!
 1213:00 <ccdle12> hi
 1313:01 <b10c> hi!
 1413:01 <felixweis> Hi
 1513:01 <amiti> notes are in the usual place: https://bitcoincore.reviews/18038.html
 1613:01 <jkczyz> hi
 1713:01 <jonatack> hi
 1813:01 <elichai2> hi
 1913:01 <amiti> the PR just got merged this morning, but that doesn't mean we should stop reviewing and thinking critically about it :)
 2013:02 <amiti> who has gotten a chance to review the PR? (y/n)
 2113:02 <robot-visions> y
 2213:02 <jnewbery> y
 2313:02 <jonatack> y
 2413:02 <fjahr> y
 2513:02 <b10c> n
 2613:02 <jkczyz> n :(
 2713:02 <Anonymous93> n
 2813:02 <ccdle12> y
 2913:02 <vasild> n
 3013:02 <willcl_ark> n
 3113:02 <_andrewtoth_> n
 3213:03 <amiti> cool!
 3313: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)
 3413:03 <amiti> thanks to those of you that reviewed, and thanks to those of you that haven't but have joined today :)
 3513:04 <amiti> lets get started by talking about what is the unbroadcast set?
 3613:04 <amiti> what is the point of it?
 3713:05 <_andrewtoth_> the point of it is to track transactions submitted locally that have not yet been broadcast to the public network
 3813:05 <amiti> yup!
 3913:05 <amiti> why?
 4013:06 <Smeier> to reduce the necessity of rebroadcasting all txs?
 4113:06 <willcl_ark> to improve privacy
 4213:06 <MarcoFalke> hi
 4313: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)
 4413:06 <amiti> yup! all correct
 4513:06 <vasild> "to improve privacy" - how?
 4613:06 <Smeier> ^
 4713: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
 4813:07 <raj_> hi
 4913:07 <Smeier> Reading some of the conversations, I was confused on whether this is a privacy positive or not
 5013:07 <amiti> great! lets get into it
 5113:08 <amiti> whats the claim for why this PR helps privacy
 5213:08 <amiti> and how does the unbroadcast set factor in?
 5313:08 <instagibbs> currently nodes only re-broadcast the same tx to same peer if it's committed to in their wallet
 5413: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
 5513:09 <gloria43> ^ so if you ever see a node telling you about the same tx twice, it must originate from them?
 5613:09 <detoxify> PR is to reduce the amount of data a network observer has to make conclusions about txn origin
 5713: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)
 5813: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.
 5913:09 <amiti> everyone is correct :D
 6013:10 <sipa> robot-visions: gloria43 just described it
 6113:10 <raj_> How the nodes decide "this one hasn't been propagated successfully, should try rebroadcasting"?
 6213: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
 6313:10 <sipa> also, hi
 6413:10 <instagibbs> robot-visions, snoop can sit and not relay, just wait for you to say it again
 6513: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.
 6613:11 <instagibbs> vasild, no
 6713:11 <Smeier> raj_, if your node receives a getdata request, you consider it broadcast
 6813: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
 6913:11 <detoxify> this is an anti-Eve measure
 7013: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?)
 7113: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
 7213:11 <detoxify> robot-visions more expensive or obviates its possibility entirely
 7313:12 <sipa> also note that you need multiple connections
 7413:12 <amiti> this allows the _wallet_ to attempt rebroadcasting _wallet_ transactions less frequently
 7513:12 <sipa> within one connection there is a bloom filter that keeps track of that the other side already knows
 7613:12 <sipa> which filters out duplicate announcements
 7713:12 <jnewbery> vasild: not currently. See https://github.com/bitcoin/bitcoin/pull/16698 for the longer-term plan
 7813:12 <jnewbery> sipa: how long does that filter take to refresh?
 7913:12 <Smeier> so, does a given node never rebroadcast foreign txs?
 8013:13 <Smeier> and why not?
 8113:13 <MarcoFalke> robot-visions: Yes. And the tx could have been mined in those 24 hours
 8213: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.
 8313:13 <MarcoFalke> Smeier: See the link jnewbery shared
 8413: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
 8513:14 <lightlike> i agree. if the tx has a reasonable miner fee, the attack should be impossible, not just more expensive.
 8613:14 <amiti> 16698 pulls out the *rebroadcast* functionality from wallet to mempool, so *all* nodes will start rebroadcasting some transactions
 8713:14 <elichai2> Do I understand correctly that the big privacy improvements aren't in this PR but will come later?
 8813:15 <MarcoFalke> elichai2: Yes, most of it
 8913:15 <MarcoFalke> Though, this one is also an improvement, I'd say
 9013:15 <amiti> elichai2: I think its subjective
 9113:15 <detoxify> lightlike +1
 9213:15 <amiti> I think this one is a significant improvement, esp in current mempool conditions
 9313:15 <amiti> but also that the next step will be even stronger privacy guarantees
 9413:15 <elichai2> But it sounds like this one actually increases the number of times it rebroadcasts it's txs
 9513:16 <MarcoFalke> elichai2: no, why?
 9613:16 <gloria43> in this PR, is the originating node still the only one rebroadcasting?
 9713:16 <amiti> not quite, to clarify this can somebody explain the difference between "unbroadcast" and "rebroadcast" ?
 9813:16 <elichai2> Oh I'm sorry, I misread
 9913:16 <sipa> gloria43: yes
10013:16 <elichai2> I thought the old is 1/day and now it's 1/15min but it's the opposite :)
10113:16 <gloria43> thanks for clarification!
10213: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
10313:17 <amiti> robot-visions: very nice!
10413: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)?)
10513:17 <Smeier> but only your own txs can be unbroadcast right?
10613:17 <elichai2> so this is definitely already a privacy improvement
10713:18 <Smeier> this PR would never label foreign Txs as unbroadcast, because they had to come from outside?
10813:18 <vasild> "in the network" is a bit fuzzy, what does it mean? "at least one other node has the tx"?
10913:18 <robot-visions> "in the network" means you received a getdata for that transaction
11013:18 <willcl_ark> so this PR -> "less chance of being identified" vs "quite likely to be identified"
11113:18 <gleb> hi
11213:18 <amiti> smeier: yes
11313:18 <amiti> willcl_ark: I don't follow
11413: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
11513:18 <Smeier> amiti: thanks
11613:19 <_andrewtoth_> is only 1 GETDATA enough? Should it wait for more than 1 to prevent it being blackholed?
11713:19 <Smeier> does the "should've been mined" heuristic consider fee rate?
11813:19 <amiti> _andrewtoth_: great question! one that I've asked myself a lot.
11913:19 <amiti> lets go through step by step of how transactions interact with the unbroadcast set
12013:20 <amiti> 1. how do transactions get added to the set?
12113:20 <vasild> so, "in the network" indeed means "at least one other node has the tx"
12213:20 <Smeier> they are locally submitted
12313:20 <amiti> smeier: correct. what does that mean?
12413:20 <_andrewtoth_> either from rpc sendrawtransaction or sent by the wallet
12513: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"
12613:21 <amiti> so, very much a heuristic
12713:21 <amiti> and the question _andrewtoth_ was asking is ... is this heuristic sufficient ?
12813:21 <gleb> we still do best effort to send to every peer, right?
12913:22 <gleb> heuristic just helps to not do it *again*
13013:22 <amiti> which is a *really important* question to be asking. because if its insufficient, transactions might not make it out to the network
13113:22 <Smeier> the best effort is changed to 12-36 hrs I believe
13213:22 <_andrewtoth_> gleb that's my understanding
13313:22 <detoxify> gleb same
13413:22 <_andrewtoth_> it's removed on first GETDATA, but it was announced to all peers and their GETDATA's could come in after
13513:22 <amiti> gleb: yes, but there can be some edge use cases that cause failures
13613: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
13713:23 <amiti> jnewbery: agreed. that seems very robust
13813:24 <Smeier> is that planned in future PRs?
13913:24 <amiti> ok, so "locally submitted" transactions get added to the set, which means they were submitted via the wallet or via the rpcs
14013:24 <_andrewtoth_> jnewbery: could that have privacy improvements as well? Not all your peers will know you've originated it
14113: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*.
14213:25 <amiti> not currently planned. would probably have privacy improvements. would have to be implemented very carefully, but probably possible
14313:25 <amiti> I considered it when designing this PR, but decided to make incremental improvements
14413: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?
14513:26 <amiti> what is another way transactions can be removed from the unbroadcast set?
14613:26 <sipa> robot-visions: that certainly gets filtered
14713:26 <amiti> (other than on the first GETDATA)
14813: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
14913: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?
15013:26 <sipa> amiti: i assume... by the transaction leaving the mempool?
15113:26 <robot-visions> amiti: I think transactions could also be removed if they get mined, expired, etc.
15213:26 <b10c> amiti: transaction is confirmed?
15313:27 <raj_> when its removed from mempool.
15413:27 <gleb> Smeier: I won't call it eclipse. Probably censorship (and only of rebroadcast tx). But yeah.
15513:27 <amiti> sipa: I like how you say you "assume" when you reviewed the pr 😂
15613:27 <robot-visions> sipa: thanks, makes sense
15713:27 <sipa> amiti: i just woke up :)
15813:27 <sipa> but i'm actually going to check now that it does that
15913:27 <_andrewtoth_> this won't be removed if it is mined, because if it is in unbroadcast set it was never sent to miners
16013:27 <Smeier> Gleb why wouldn't this affect unbroadcast?
16113: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
16213:28 <_andrewtoth_> jnewbery: they would have to have a lot of knowledge of peer topography to determine that no?
16313: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...
16413:28 <jnewbery> possibly. I'm just throwing stuff out there :)
16513:29 <b10c> _andrewtoth_: it could have reached the miner not from our node
16613:29 <robot-visions> _andrewtoth_: good point, I suppose that can't usually happen unless YOU are the miner or some other edge case
16713:29 <_andrewtoth_> b10c: how if it's a locally submitted transaction?
16813:29 <b10c> _andrewtoth_: if I submit it on e.g. two nodes
16913:29 <robot-visions> _andrewtoth_: maybe someone submitted it in multiple places?
17013: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
17113: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
17213:29 <Smeier> few
17313:30 <jnewbery> _andrewtoth_: One example: you co-signed a transaction with someone. They submit it and you submit it
17413:30 <sipa> in general both the sender and receiver will rebroadcast
17513:31 <amiti> ok cool, moving forward..
17613: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.
17713:31 <_andrewtoth_> right, lots of cases where it could be mined but still in the unbroadcast set
17813:31 <amiti> the next two questions we have danced around, but maybe we can dig into it further
17913: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?
18013:31 <gleb> lightlike: as low as 2 hours in some cases.
18113:31 <amiti> ok nevermind i'll pause till we dig into these :)
18213:32 <amiti> one topic being discussed is different heuristics for determining if the unbroadcast txns have been seen by the network
18313:32 <gleb> lightlike: generally they widely use 144 blocks, but there are some corner cases where they do like 6-12 blocks iirc.
18413:33 <_andrewtoth_> doesn't it depend on how many hops the payment is sent?
18513: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.
18613: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
18713:34 <gleb> sipa: very cool consideration. I'm actually curious now.
18813:35 <amiti> sipa: when you say mempool-based rebroadcast, are you talking about implementation in 16698?
18913:35 <sipa> amiti: i'm talking about the PR that was just merged, 18038
19013:35 <amiti> or do you mean mempool-based unbroadcast retries in 18038
19113: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
19213:35 <amiti> oh yes, only the sender will reattempt
19313:36 <sipa> addition to the unbroadcast set happens in BroadcastTransaction, which only the sender calls
19413:36 <sipa> i think the receiver should also call it
19513: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
19613: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?
19713:36 <amiti> sipa: I agree only sender reattempts. can you help me understand a use case why receiver should also call?
19813:36 <jnewbery> sipa: I don't think so. If you've received it then it's already propagated
19913:37 <jnewbery> this seems like an improvement to me. The receiver won't needlessly rebroadcast
20013:37 <_andrewtoth_> a use case is if the tx doesn't get confirmed in 24 hours, and the sender is offline
20113:37 <sipa> amiti: for starters, conceptually, the receiver is the only one who actually cares the transaction confirms
20213:37 <jnewbery> the receiver would still rebroadcast after 12-36 hours I believe
20313:37 <Anonymous93> amiti sidetrack, but how did u go about debugging this PR and knowing which files to modify?
20413:37 <sipa> and the receiver may have learned about the tx out of band
20513:38 <amiti> jnewbery: yes they'd rebroadcast after 12-36 hours
20613: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?
20713:38 <MarcoFalke> sipa: Out of band == sendrawtransaction?
20813:38 <Smeier> sorry, beginner question, but that means all txs are broadcast again regardless of origin every 12-36 hrs?
20913:39 <MarcoFalke> Smeier: It is a wallet function to do this every couple of hours
21013:39 <sipa> right, i'm saying this PR changed the receiver rebroadcast from frequent to infrequent, without adding a corresponding unbroadcast logic to compensate
21113:39 <amiti> smeier: right now only wallet txns will be broadcast again every 12-36 hours
21213:39 <jnewbery> Smeier: correct. The wallet will rebroadcast all of its unconfirmed transactions (sent or received) every 12-36 hours
21313:39 <sipa> amiti: or you run local software that used the p2p interface to submit transactions to the receiver's node
21413:39 <Smeier> including other unrelated ones? or only sent and received ?
21513:39 <sipa> i agree it's not the most common scenario
21613: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".
21713:39 <sipa> Smeier: only wallet-affecting transactions
21813:39 <robot-visions> Excluding `rawtransaction`, which transactions are eligible for rebroadcast?
21913:40 <sipa> robot-visions: all wallet transactions
22013:40 <instagibbs> robot-visions, wallets will rebroadcast *any* transaction stored in the wallet
22113:40 <sipa> i don't know what rawtransaction has to do with it?
22213:40 <amiti> sipa: I think in the case where the recipient resubmits via RPC, then they will add to unbroadcast set as well
22313:40 <instagibbs> robot-visions, this can include a transaction you sent, a transaction you received, a transaction you *watched*
22413:40 <instagibbs> aka watchonly
22513:40 <amiti> if they are doing something that uses the p2p interface to submit, then you're right, the functionality has been changed
22613: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
22713:41 <Smeier> what should I read to understand the difference between wallet and p2p interface? do they submit txs differently?
22813:42 <amiti> robot-visions: if you wanna better understand, check out the ResendWalletTransactions method https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1986
22913:42 <MarcoFalke> Smeier: They call the same function "BroadcastTransaction"
23013:42 <amiti> in the section where it wants to relay transactions, it iterates through `mapWallet`
23113:42 <_andrewtoth_> ahh so received txs still get rebroadcast after 24 hours, that's not affected by unbroadcast set
23213:42 <sipa> MarcoFalke: wallet and RPC both use BroadcastTransaction
23313:42 <MarcoFalke> jup, I believe so
23413:42 <sipa> "p2p interface" to submit transactions... that's just the network
23513:42 <amiti> you can search the code for what transactions get tracked in `mapWallet` :)
23613:43 <sipa> jnewbery: i don't understand your last sentence
23713:43 <robot-visions> sipa: instagibbs: amiti: thanks, that helps a lot!
23813:43 <_andrewtoth_> Smeier: wallet submits locally, rpcs are sendtoaddress and sendmany. p2p uses INV and GETDATA to broadcast txs
23913:43 <MarcoFalke> sipa: A transaction added to the mempool will get the txid passed to connman for the inv logic
24013:44 <MarcoFalke> `RelayTransaction` or so
24113:44 <sipa> sure, for the initial broadcast
24213:44 <amiti> Anonymous93: "sidetrack" questions are welcome! not sure what you meant though
24313:44 <MarcoFalke> sipa: But yeah. There is no unbroadcast for wallet txs received on the p2p interface
24413:45 <Smeier> _andrewtoth_ wallet submits locally but then it calls BroadcastTransaction, correct? and RPC calls the same?
24513: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
24613: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
24713:46 <sipa> right
24813:46 <_andrewtoth_> Smeier: wallet is controlled by RPC
24913: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.
25013: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)
25113:46 <instagibbs> worth a release note probably that wallets are in charge of rebroadcast, even if external
25213:46 <amiti> thats true whether they are sender / receiver / whatever
25313:46 <sipa> jnewbery: and say the sender goes offline
25413: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.
25513:46 <sipa> sure
25613:46 <Anonymous93> amiti how do u go about debugging ur code to make sure the PR works?
25713:46 <MarcoFalke> Anonymous93: there are tests included
25813: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
25913:47 <Anonymous93> do u use GDB, just observing and looking?
26013:47 <amiti> I also like to log things so I can trace through the C++ code and check my expectations
26113:47 <Anonymous93> understood, thanks MarcoFalke and amiti
26213:47 <jnewbery> if you want to force something back into the unbroadcast set, you can call sendrawtranscation, right?
26313:47 <instagibbs> Anonymous93, also depends on the size of change you're making how much you test and how you test
26413:48 <Anonymous93> cool
26513:48 <Smeier> on the topic of checking, I checked out this pr branch, and ran make check (as with normal bitcoin core)
26613: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
26713:48 <sipa> jnewbery: my point is that that's an unreasonable behavior change, if anything relies on it
26813:48 <amiti> its a mixture of techniques :)
26913:48 <Smeier> How can I look at the tests just for this PR?
27013:48 <sipa> jnewbery: so far, the wallet has done frequent rebroadcasting of every relevant transaction, not just sent ones
27113:48 <MarcoFalke> sipa: Agree that that should be mentioned somewhere outside this IRC log
27213:48 <amiti> smeier: check out the diff of this PR & trace down where tests have been introduced, either unit or functional
27313: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?
27413:49 <Anonymous93> fjahr can u move this onto GitHub so that we can maintain it: https://gist.github.com/fjahr/2cd23ad743a2ddfd4eed957274beca0f
27513:49 <jnewbery> sipa: but I don't see much difference from if we're the sender. We'll relay it once in both cases
27613:49 <sipa> this has changed to only frequent rebroadcasting of unbroadcast sent transactions, and infrequent rebroadcasting of everything else
27713:49 <amiti> sipa, marcofalke: any ideas of where I can document?
27813:49 <amiti> if this was a use case that people relied on, then we could figure out a way to support it
27913:49 <sipa> it seems trivial to fix
28013:50 <amiti> oh yeah?
28113:50 <robot-visions> Smeier: one way is to look at individual commits in the PR: https://github.com/bitcoin/bitcoin/pull/18038/commits
28213:50 <_andrewtoth_> Smeier: In the commit https://github.com/bitcoin/bitcoin/pull/18038/commits/297a1785360c4db662a7f3d3ade7b6b503258d39
28313:50 <jnewbery> the only difference is if you're offline when you submit your receiving transaction over your local p2p network
28413:50 <sipa> call AddUnbroadcastTx in AddToWallet
28513:50 <fjahr> Anonymous93: sure, i was planning to rework it but collaborating is better for sure. Thanks!
28613:51 <Anonymous93> fjahr I looked at it and definitely want to help out!
28713: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
28813:51 <jnewbery> sipa: then you'll always echo back transactions sent to you. Seems like a step backwards
28913:51 <robot-visions> s/sender/receiver
29013:51 <gleb> sipa: not sure the wording Unbroadcast now really applies that well but yeah, i think the idea makes perfect sense.
29113:52 <sipa> jnewbery: hmm, i see
29213:52 <amiti> I'll have to think about it more. I agree with jnewbery that in most cases it seems bad for privacy
29313:52 <Anonymous93> can we propose fixes that we want to see being reviewed? I'm having trouble fixing one of the unit tests.
29413:52 <gleb> i wish there was a way to distinguish receiving from regular p2p network and semi-out-of-band whatever.
29513:52 <gleb> Maybe we could remove it once we get a second inv or something...
29613:52 <Anonymous93> https://github.com/bitcoin/bitcoin/issues/18776
29713:53 <b10c> question 5 is "Should mempool.dat be versioned?"
29813: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
29913:53 <b10c> I think it is versioned, isn't it?
30013:53 <MarcoFalke> jnewbery: Why? there is a bloom filter to prevent that
30113:53 <amiti> were talking about this use case of "submitting via p2p", what would that hook up into?
30213:53 <amiti> ahahhahah b10c thank you :)
30313:53 <sipa> in which case you'll never get an INV again, so it will remain in the unbroadcast set
30413:53 <sipa> which would be bad
30513:54 <amiti> it does have a version on it, but it seems like its actually a bit tricky to update the version
30613:54 <b10c> it is: https://github.com/bitcoin/bitcoin/blob/0ef0d33f7562c3b7f9c021549e70b3b4dbcc504c/src/validation.cpp#L5006-L5009
30713:54 <jnewbery> MarcoFalke: yeah, pfilter will catch it
30813: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
30913:54 <b10c> amiti: oh why?
31013: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.
31113: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.
31213: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
31313:55 <sipa> MarcoFalke: hmm?
31413:55 <MarcoFalke> *if it the tx is not in the mempool
31513:55 <sipa> (i was talking about a hypothetical AddToWallet calling AddUnbroadcast - which I agree now would be a bad idea)
31613:55 <robot-visions> amiti: do you know what happens if you start the updated version with an old `mempool.dat`?
31713:55 <amiti> b10c: check out this comment thread here. https://github.com/bitcoin/bitcoin/pull/18038#discussion_r397982629. marcofalke gave some descriptions and I learned a bunch
31813:56 <MarcoFalke> sipa: Even absent that, it is still a bug and exploitable through mempool.dat
31913:56 <sipa> MarcoFalke: then i don't know what you're talking about; care to elaborate?
32013: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"?
32113:57 <sipa> gleb: this is all solved once all nodes start rebroadcasting everything
32213:57 <sipa> so maybe we should just defer the solutions to that
32313:57 <MarcoFalke> sipa: Will submit a fix instead ;)
32413: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.
32513:57 <b10c> amiti: thanks! didn't even mean move on, was just curious about that question
32613:57 <amiti> ok! so looks like we have 3 minutes left
32713:57 <amiti> does anybody have any questions
32813:58 <sipa> 2
32913:58 <amiti> =P
33013:58 <jnewbery> MarcoFalke: sipa: I believe we just need to do this: https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609 to prevent the unbroadcast set potentially getting out of sync with the mempool
33113:58 <fjahr> Anonymous93: Done :) https://github.com/fjahr/debugging_bitcoin
33213:59 <vasild> Why the current code does not treat own txs like foreign txs and rebroadcast everything? To reduce traffic?
33313:59 <sipa> vasild: that's the next step; one thing at a time
33413:59 <MarcoFalke> jnewbery: Probably good to add checks to both AddUnbroadcast and ReattemptInitialBroadcast
33513:59 <jnewbery> vasild: see the next PR in the sequence
33613:59 <MarcoFalke> jnewbery: "belt-and-suspenders"
33713:59 <amiti> yes rebrodacasting *everything* would be a significant hit to the bandwidth, so we have to do it carefully
33813:59 <amiti> thus the next PR
33913:59 <vasild> sipa: yes, but I mean why it was not done like that in the first place?
34013:59 <sipa> vasild: ask satoshi?
34113:59 <sipa> :p
34214:00 <jonatack> I'm certainly looking forward to reviewing the next steps.
34314:00 <vasild> probably he had a slow internet connection :)
34414:00 <amiti> ok! that is time
34514:00 <amiti> thanks all for playing!
34614:00 <instagibbs> amiti, have you thought much about rusty's point here https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-April/017797.html
34714:00 <jnewbery> thanks amiti!
34814:01 <robot-visions> thanks!
34914:01 <instagibbs> would the next PR fit the bill to start thinking about reforming transaction replacement policy?
35014:01 <vasild> Cheerz!
35114:01 <amiti> if anybody has outstanding questions, feel free to DM me and I'll try my best to get to them
35214:01 <_andrewtoth_> thanks amiti!
35314:01 <b10c> thanks amiti!
35414:01 <lightlike> thanks!
35514:01 <gzhao408> thanks amiti!
35614: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
35714:02 <willcl_ark> thanks amiti, that was an interesting discussion to watch
35814:02 <Smeier> thank you!
35914:02 <amiti> instagibbs: not entirely sure the relationship you're talking about, but I'll take a closer look at the email
36014:02 <jonatack> sipa: agree, good idea
36114:02 <amiti> sipa: will do
36214:02 <instagibbs> amiti, yeah just read it, you'll likely know the answer more than me
36314:02 <amiti> 👌
36414:03 <sipa> i think it's mostly unrelated to this work
36514:03 <jonatack> thanks amiti et al, excellent session
36614:03 <sipa> thanks!
36714:03 <sipa> the discussion made me review your changes in more depth :)
36814:04 <amiti> thats awesome!
36914:05 <amiti> I learned stuff today (and want to continue..)