--- Log opened Wed Jun 23 00:00:46 2021
00:59 -!- belcher_ is now known as belcher
02:02 -!- kexkey_ [~kexkey@static-198-54-132-116.cust.tzulo.com] has joined #bitcoin-core-pr-reviews
02:04 -!- kexkey [~kexkey@static-198-54-132-132.cust.tzulo.com] has quit [Ping timeout: 258 seconds]
03:27 -!- blkncd [uid505676@id-505676.brockwell.irccloud.com] has joined #bitcoin-core-pr-reviews
07:32 < pinheadmz> do non-standard TXs ever get mined? -- and I mean anecdotally, has anyone seen it happen or how often?
07:43 < blkncd> I've seen zero-fee transactions (non-coinbase) getting mined by F2Pool (I think they are pool payout tx) 
07:46 < blkncd> e.g. 9820cf50af09fc045d4317c875e9bab321ced103871b4ff9bc9c739095010f8e
07:55 < pinheadmz> ha interesting!
07:56 < pinheadmz> LOL blockstream.info: "This transaction could save 0.09% on fees by upgrading to native SegWit-Bech32 or 0.07% by upgrading to SegWit-P2SH"
08:56 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has quit [Quit: ZNC - https://znc.in]
09:01 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has joined #bitcoin-core-pr-reviews
09:10 -!- lightlike [~lightlike@user/lightlike] has joined #bitcoin-core-pr-reviews
09:39 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
09:39 -!- siddhantJ19 [~siddhantJ@49.36.217.35] has joined #bitcoin-core-pr-reviews
09:40 -!- davterra [~davterra@178.128.106.205] has quit [Quit: Leaving]
09:44 -!- dopedsilicon [~dopedsili@103.159.243.27] has joined #bitcoin-core-pr-reviews
09:48 -!- dunxen [dunxen@gateway/vpn/protonvpn/dunxen] has joined #bitcoin-core-pr-reviews
09:54 -!- dunxen [dunxen@gateway/vpn/protonvpn/dunxen] has quit [Quit: Leaving...]
09:55 -!- LarryRuane [~LarryRuan@c-98-245-204-148.hsd1.co.comcast.net] has joined #bitcoin-core-pr-reviews
09:56 -!- observer63 [~observer@cpe-23-242-148-67.socal.res.rr.com] has joined #bitcoin-core-pr-reviews
09:58 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
09:59 -!- Azorcode [~Azorcode@200.109.43.67] has joined #bitcoin-core-pr-reviews
10:00 < glozow> #startmeeting
10:00 < LarryRuane> hi
10:01 < schmidty> howdy
10:01 < svav> hi
10:01 < dopedsilicon> Hey
10:01 < michaelfolkson> hola
10:01 < Azorcode> Hi 
10:01 < glozow> hi everyone! welcome to PR Review Club :) we're looking at Two small fixes to node broadcast logic today: https://bitcoincore.reviews/22261
10:01 < lightlike> hi
10:01 < glozow> did anyone get a chance to look at the notes or review the PR?
10:02 < svav> Looked at the notes
10:02 < michaelfolkson> Yup, maybe can't answer all the questions though
10:03 < michaelfolkson> Took a while to get my head around why the fixes were needed
10:03 < glozow> okie dokie
10:03 -!- zakinator [~zakinator@c-73-152-14-173.hsd1.va.comcast.net] has joined #bitcoin-core-pr-reviews
10:03 < glozow> so this PR is making changes to the `BroadcastTransaction()` function - can anyone tell me what this function does?
10:03 -!- ru [~ru@122.174.144.66] has joined #bitcoin-core-pr-reviews
10:03 < glozow> we're here: https://github.com/bitcoin/bitcoin/blob/7317e14a44c6efc545e6fb9bcedee7174e93a8fa/src/node/transaction.cpp#L29
10:04 < glozow> (guesses are fine too)
10:05 < michaelfolkson> So broadcasts a transaction :) Called by RPCs
10:06 < michaelfolkson> Too obvious?
10:06 < lightlike> adds transactions to the mempool and/or unbroadcast set and also relays them immediately
10:06 < LarryRuane> This is to send a transaction that originated in the current node (as opposed to being relayed, one that we recieved from a different node) ... (i'm not too confident in this answer)
10:06 < glozow> lightlike: wonderful, yes. invokes the mempool validation logic + relays
10:07 < glozow> LarryRuane: aha yes, it's interesting that it's in the src/node folder
10:07 < glozow> so these transactions would come from clients (e.g. RPC and wallet)
10:08 < glozow> where is `BroadcastTransaction()` called? is anyone able to link us to some call sites?
10:09 < svav> src/node/transaction.cpp
10:09 < LarryRuane> could we get a little more background on "RPC" versus "wallet"? If I had to guess, I'd say `sendrawtransaction` is RPC, and generally has NOTHING to do with the local wallet ... where as "wallet RPC" would be something like `sendmany`
10:09 < glozow> svav: that's where `BroadcastTransaction()` is defined yes
10:09 < sipa> LarryRuane: that sounds correct
10:10 -!- Azorcode [~Azorcode@200.109.43.67] has quit [Ping timeout: 250 seconds]
10:10 < sipa> sendmany is instructing your local wallet to construct & broadcast a transaction
10:10 < svav> can be called by either sendrawtransaction RPC or wallet RPCs
10:10 < glozow> LarryRuane: correct - `sendrawtransaction` lets you submit a raw transaction directly to the node, you could call that without having the wallet ocmpiled
10:10 < sipa> sendrawtransaction is sending it yourself, no wallet involved
10:11 < glozow> right
10:11 < LarryRuane> thanks sipa and glozow, very helpful
10:11 < michaelfolkson> So is the BroadcastTransaction() function used for both transactions originated in user's wallet and transactions received from other peers?
10:11 < glozow> ok, does anyone know exactly what bugs are being fixed in #22261 or should we walk through it together?
10:12 < LarryRuane> michaelfolkson I think not, not used for tx received from other peers
10:12 < glozow> michaelfolkson: it wouldn't come from other peers
10:12 < glozow> that invokes `AcceptToMemoryPool()` from the net processing layer
10:13 < michaelfolkson> LarryRuane glozow: Ok thanks
10:13 < michaelfolkson> I think I understand what bugs are being fixed but I had to get my head round some terminology
10:13 < glozow> michaelfolkson: feel free to give a guess of what the bugs are?
10:14 < michaelfolkson> Ok so summarizing John's description...
10:14 -!- dopedsilicon [~dopedsili@103.159.243.27] has quit [Quit: Connection closed]
10:15 < michaelfolkson> Rebroadcasting a transaction that peers aren't interested in
10:15 < michaelfolkson> (as it has already been broadcast to them)
10:15 -!- Azorcode [~Azorcode@200.109.43.67] has joined #bitcoin-core-pr-reviews
10:15 < michaelfolkson> Need to define what the unbroadcast set is here
10:15 < glozow> mm okay, i think this probably touches on the unbroadcast set
10:15 < glozow> that's 1 part of the PR
10:16 < glozow> good idea, anybody want to define unbroadcast set?
10:16 < glozow> hint: https://github.com/bitcoin/bitcoin/blob/0844084c/src/txmempool.h#L586
10:16 < glozow> https://bitcoincore.reviews/18038
10:16 < michaelfolkson> I looked it up there, yeah
10:16 < michaelfolkson> "when the initial broadcast of a transaction hasn’t been deemed successful"
10:16 < michaelfolkson> And then you need to define what successful means :)
10:17 < michaelfolkson> receiving a single GETDATA
10:17 < LarryRuane> The way I understand unbroadcast set is, our node originates a tx, sends it out, but isn't sure that any other node has it (maybe the P2P message got dropped?)
10:17 < glozow> LarryRuane: right, the goal is to have an idea of whether our initial broadcast of transactions has succeeded.
10:18 < LarryRuane> other nodes *may* have it, but we're not sure
10:18 < michaelfolkson> LarryRuane: Right the message could have got dropped, the node might already know of the transaction or the node might not be interested in that transaction despite not having it?
10:18 < michaelfolkson> Is that right glozow?
10:19 < glozow> michaelfolkson: right, so we're naming scenarios for why a peer might not send us a GETDATA for a transaction (which would cause the tx to remain in the unbroadcast set)
10:19 < glozow> and you're correct - it's possible there's some connection issue, they might already know the transaction, and "might not be interested" could be that they already rejected the tx or heard it from someone else
10:20 < LarryRuane> so IIUC what makes the unbroadcast set special is that we don't retry those tx sends as quickly as we used to, we wait about up to one day
10:20 < glozow> i don't think we reject tx invs for no reason 🤔
10:20 < michaelfolkson> So it could be "unsuccessful" from your perspective even though all nodes already know and have that transaction
10:20 < glozow> LarryRuane: right
10:20 < glozow> michaelfolkson: yes. that's unlikely if this is the first time broadcasting that transaction, though
10:20 < LarryRuane> for better privacy (sorry if i'm being obvious :)  )
10:21 < glozow> this is only intended for the initial broadcast
10:21 < michaelfolkson> glozow: Right if you are originating the transaction, gotcha
10:21 -!- dopedsilicon [~dopedsili@103.159.243.27] has joined #bitcoin-core-pr-reviews
10:21 < glozow> LarryRuane: not at all, i'm sure this is new information for lots of people! or good to review
10:21 -!- dopedsilicon [~dopedsili@103.159.243.27] has quit [Client Quit]
10:22 < lightlike> if that tx originates from your node, how could it happen that they know it already? (if they learnt about it from some other peer then *that peer or another* must have sent you a GETDATA at some time, so it would be cleared from the unbroadcast set already?)
10:23 < glozow> lightlike: right, it shouldn't be the case that your unbroadcast set has a transaction that has already gone through initial broadcast
10:23 < glozow> but one of the bugs here is you could re-add a transaction to your unbroadcast set
10:23 < LarryRuane> ok but if it's a *set* ... sets can't have duplicates, right?
10:24 < glozow> yes, but once you've removed it, the set won't remember that it's already seen it before
10:24 < LarryRuane> ah, ok
10:25 < glozow> so our issue is: after we've already broadcast the tx, we don't want to re-add it to our unbroadcast set, because that will cause us to keep rebroadcasting it (and we won't remove it because our peer won't re-request it from us)
10:25 < glozow> does that make sense?
10:25 < glozow> peers*
10:25 < lightlike> would we remove it though after it makes it into a block in that situation?
10:26 < michaelfolkson> What was the rationale for adding it to the unbroadcast set before this PR?
10:26 < michaelfolkson> Just an oversight?
10:26 < LarryRuane> is this caused by what may be considered a user error? Do I have this flow right?: User initiates a tx (on our local node) ... gets broadcast (and also added to unbroadcast set) ... GETDATA happens, so we remove from unbroadcast set .. then user re-submits the same tx?
10:26 < glozow> lightlike: i thiiiink so
10:27 < glozow> LarryRuane: correct, it would be because the user called it again
10:28 < LarryRuane> so all what we've discussed so far here, is independed of the txid / wtxid distinction? I'm unclear how that relates to all this
10:28 < michaelfolkson> Yeah that is coming now
10:28 < glozow> LarryRuane: that's relevant here too
10:29 < LarryRuane> is the map key for the unbroadcast set the txid or the wtxid? (guess i could just look it up!)
10:29 < michaelfolkson> glozow: Relevant to the first fix?
10:29 < svav> Are there any diagrams for all this?
10:29 < LarryRuane> yes i was asking about the first fix
10:30 < michaelfolkson> That's the second fix? There's no overlap right?
10:30 < glozow> well, they're intertwined
10:30 < michaelfolkson> Ohh
10:30 -!- zakinator [~zakinator@c-73-152-14-173.hsd1.va.comcast.net] has quit [Quit: Connection closed]
10:30 < glozow> so what happens if you call `BroadcastTransaction()` with a transaction that has same-txid-different-wtxid as a tx in the mempool?
10:30 < glozow> (on master, before this PR)
10:30 < glozow> svav: i'm not sure, i think the review club for 18038 would be best place for info
10:31 -!- Azorcode [~Azorcode@200.109.43.67] has quit [Quit: Connection closed]
10:31 < glozow> (do we know what I mean when i say same-txid-different-wtxid?)
10:31 -!- Azorcode [~Azorcode@200.109.43.67] has joined #bitcoin-core-pr-reviews
10:31 < svav> it will call RelayTransaction() with the wtxid of the argument tx’s wtxid rather than the one in the mempool. This causes the relay to fail (INVs are not sent) for wtxid-relay peers because SendMessages() queries the mempool by wtxid, doesn’t find it, and drops the announcement.
10:31 < LarryRuane> glozow yes I do get that distinction
10:31 < glozow> svav: yes! well said
10:32 < svav> I copied that lol
10:32 < glozow> i know
10:32 < glozow> :P
10:32 < glozow> so before this PR, if you called `BroadcastTransaction()` with a transaction that has same-txid-different-wtxid as a tx in the mempool, it would get re-added to the unbroadcast set without even going through validation
10:33 < glozow> not a huge issue, but not exactly the behavior we want
10:33 < glozow> so the first fix in the PR is to move adding to unbroadcast set after a successful submission to mempool
10:33 < svav> What is the fix then?
10:34 < glozow> the other problem is that we would call `RelayTransaction()` with the wtxid of this transaction, rather than the one in the mempool
10:35 < glozow> ("this" transaction = the argument to the function) 
10:35 < svav> For beginners can someone explain how two transaction can have the same txid but different wtxids?
10:35 < michaelfolkson> I do agree with svav that a diagram would be nice here :)
10:35 < svav> I love diagrams :)
10:35 < michaelfolkson> The interaction between the mempool, the unbroadcast set etc
10:35 < LarryRuane> I'm trying to understand why there would be two tx with the same txid but different witnesses (different wtxid) ... is it because there's some random element in the witness data? so if I submit the same tx twice (but signing each separately), then this can happen?
10:35 < LarryRuane> svav yes you beat me to that question!
10:36 < michaelfolkson> It was discussed yesterday in the L2 onchain workshop right? You find a cheaper witness
10:36 < glozow> mm so the txid of a transaction doesn't commit to the witness data
10:37 < glozow> i'm trying to find a good diagram
10:37 < LarryRuane> right so IIUC the txid commits to the *effects* of a tx only, the wtxid commits also to the authorization
10:37 < glozow> LarryRuane: correct
10:37 < michaelfolkson> https://bitcoin.stackexchange.com/questions/99409/what-does-segwit-remove
10:38 < michaelfolkson> At the bottom
10:38 < michaelfolkson> The wtxids get collected up to the coinbase transaction and are independent of the txids
10:38 < glozow> nono, that's the coinbase witness commitment
10:38 < glozow> we're talking about what transaction data gets serialized to get the wtxid vs the txid
10:39 < glozow> https://usercontent.irccloud-cdn.com/file/1XFdPlGD/image.png
10:39 < glozow> this is the simplest diagram i can find
10:40 < glozow> basically, you don't include witness data in txid haha
10:40 < michaelfolkson> Right the coinbase tx commits to the witnesses but not the transaction attached to that witness
10:40 < sipa> yes they do
10:40 < glozow> er, we're not talking about the coinbase commitment
10:41 < sipa> the coinbase tx commitments contains the merkle root of the wtxid tree
10:41 < glozow> and ^
10:41 < sipa> wtxids commit to the full transaction, witness and non-witness data
10:41 < michaelfolkson> But you can change the wtxid without changing the txid of that transaction
10:41 -!- effexzi [uid474242@id-474242.charlton.irccloud.com] has joined #bitcoin-core-pr-reviews
10:41 < sipa> yes, by changing the witness
10:41 -!- siddhantJ19 [~siddhantJ@49.36.217.35] has quit [Quit: Connection closed]
10:42 < michaelfolkson> The witness data is not committed to in the txid. Otherwise that would reintroduce malleability
10:42 < sipa> yes
10:42 < LarryRuane> sipa so was I correct in my earlier question, you can create an unlimited number of valid witnesses for a given tx (assuming you can create one)?
10:42 < michaelfolkson> Ok thanks. I thought I understood that lol
10:43 < sipa> LarryRuane: yes
10:43 < sipa> LarryRuane: well, technically speaking there is only a finite number of valid witnesses, due to block weight being finite :)
10:43 < glozow> teehee
10:44 < glozow> hok
10:44 < michaelfolkson> Ok sorry glozow let's get back to your question
10:44 < glozow> so our peermanager's `RelayTransaction()` function takes a txid and wtxid parameter
10:44 < glozow> (can anyone tell me why?)
10:45 < glozow> what happens if we're trying to relay a transaction to our peer and can't find it in our mempool?
10:46 < michaelfolkson> We can't find the right wtxid or we literally can't find the txid?
10:46 < glozow> we can't find the transaction
10:46 < glozow> we know the id
10:47 < michaelfolkson> We can't relay it? We don't have it?
10:47 < glozow> yup
10:47 < michaelfolkson> You mean like it returns an error?
10:47 < glozow> no, we just skip it
10:47 < svav> wtxid parameter also used because some peers that prefer to receive wtxid announcements, i.e. wtxid-relay peers
10:48 < glozow> it's fully possible that we want to announce a transaction but it gets evicted from mempool by the time we go to announce it
10:48 < svav> ?
10:48 < glozow> but in this case, it could be because we called `RelayTransaction()` with a wtxid of a transaction that isn't in our mempool
10:48 < glozow> svav: exactly
10:48 -!- ru [~ru@122.174.144.66] has quit [Quit: Connection closed]
10:48 < michaelfolkson> What's the scenario here. The wallet doesn't know what the node is doing? That causes the confusion?
10:49 < michaelfolkson> If they were perfectly in sync this wouldn't happen
10:50 < glozow> ok the scenario is this: we have a transaction. the mempool has one with witness A and we call `BroadcastTransaction()` with the exact same tx, but different witness
10:50 < lightlike> why would you malleate your own transactions?
10:50 < glozow> idk 🤷 maybe there are other parties in it
10:50 < glozow> it's a multisig, idk
10:51 < michaelfolkson> lightlike: It is finding a cheaper witness after previously broadcasting a more expensive witness I think
10:51 < glozow> maybe it has multiple spending paths and you/a counterparty both made transactions
10:51 < LarryRuane> michaelfolkson cheaper meaning smaller (so lower fee)? 
10:51 < michaelfolkson> It isn't deliberate malleation. And obviously the txid isn't malleated post SegWit
10:52 < michaelfolkson> LarryRuane: Smaller, so yeah lower fee
10:52 < glozow> this isn't a show-stopping bug, it's just not the behavior we'd expect
10:53 < glozow> either way, we should try to relay the transaction in our mempool
10:53 < michaelfolkson> "Prior to this commit, if BroadcastTransaction() is called with
10:53 < michaelfolkson> relay=true, then it'll call RelayTransaction() using the txid/wtxid of
10:53 < michaelfolkson> the new tx, not the txid/wtxid of the mempool tx."
10:53 < michaelfolkson> And the mempool doesn't have the new transaction?
10:53 < glozow> yes
10:54 < michaelfolkson> Ok
10:54  * michaelfolkson sweats
10:54 < LarryRuane> what's the worst effect (at a high level) of not fixing these bugs? transactions not being relayed around the network?
10:55 < glozow> good question. anybody have ideas?
10:55 < michaelfolkson> 2) just means less effective relaying right? More failures
10:55 < michaelfolkson> 1) is wasteful. Unnecessary relaying
10:55 < lightlike> for the first one, it would seem the opposite: tx beinged inv'ed too much (although the network already knows about it)
10:57 < glozow> right
10:57 -!- kexkey [~kexkey@static-198-54-132-100.cust.tzulo.com] has joined #bitcoin-core-pr-reviews
10:58 < glozow> no consensus failures. but worth fixing, ya?
10:58 < lightlike> If i understand it correctly, "transactions not being relayed" would rather be a downside of the first fix: if we happen to have bad peers, broadcast to them, get a GETDATA but they don't relay any further, then we'd currently try again after ~24hours (when we have better peers), after the fix we wouldn't do this anymore.
10:58 < LarryRuane> we're about out of time, so this would be too long of a discussion, but I notice there's no new or changed test code with this PR .. would be interesting to get into why, and generally, what kind of PRs need tests and what kind don't
10:58 < michaelfolkson> LarryRuane: If you read the review comments tests were discussed
10:59 < LarryRuane> ok thanks (hadn't done that, will do)
10:59 < michaelfolkson> duncandean is working on tests it appears
10:59 < michaelfolkson> I think everyone agrees it should have tests
10:59 -!- kexkey_ [~kexkey@static-198-54-132-116.cust.tzulo.com] has quit [Ping timeout: 252 seconds]
11:00 < glozow> shameless plug: if you're interested in same-txid-diff-wtxid stuff, i also recommend https://github.com/bitcoin/bitcoin/pull/22253
11:00 < LarryRuane> often the tests are harder to write than the code being tested, I've noticed!
11:00 < glozow> that's all we have time for :)
11:00 < glozow> #endmeeting
11:00 < glozow> This is a test: https://github.com/glozow/bitcoin/commit/5069a834ed06f2df5f43c416dc5a501c3b010b33
11:01 < glozow> thanks for coming everybody!
11:01 < michaelfolkson> That was one tricky small PR
11:01 < lightlike> thanks glozow!
11:01 < glozow> yeah, subtle and tricky
11:01 < LarryRuane> thank you glozow! very informative!!
11:01 < michaelfolkson> Thanks glozow :)
11:01 < svav> Thanks glozow
11:01 < Azorcode> thanks 
11:01 < glozow> thanks for lending your brains for the hour ^_^
11:02 < michaelfolkson> I wonder how John identified it. Probably working on another PR
11:02 < michaelfolkson> Very edge case-like
11:02 < glozow> he found it while reviewing #22253 haha
11:03 < michaelfolkson> Ah nice
11:04 < michaelfolkson> I'll answer this and put it on StackExchange if no one else does https://bitcoin.stackexchange.com/questions/107214/what-does-unbroadcast-mean-what-does-it-mean-for-a-transaction-to-be-successful
11:05 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
11:07 -!- Azorcode [~Azorcode@200.109.43.67] has quit [Quit: Connection closed]
11:33 -!- observer63 [~observer@cpe-23-242-148-67.socal.res.rr.com] has quit [Quit: Connection closed]
13:41 -!- effexzi [uid474242@id-474242.charlton.irccloud.com] has quit [Quit: Connection closed for inactivity]
14:45 -!- ivan4 [~ivan@189.172.206.22] has joined #bitcoin-core-pr-reviews
14:48 -!- ivan3 [~ivan@189.172.217.129] has quit [Ping timeout: 268 seconds]
15:02 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
15:03 -!- lightlike [~lightlike@user/lightlike] has quit [Quit: Leaving]
15:59 -!- TheRec [~toto@user/therec] has quit []
16:09 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews
16:09 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has quit [Changing host]
16:09 -!- TheRec [~toto@user/therec] has joined #bitcoin-core-pr-reviews
17:08 -!- belcher_ [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
17:11 -!- belcher [~belcher@user/belcher] has quit [Ping timeout: 252 seconds]
19:10 -!- provoostenator [~quassel@user/provoostenator] has quit [Ping timeout: 240 seconds]
19:21 -!- provoostenator [~quassel@user/provoostenator] has joined #bitcoin-core-pr-reviews
--- Log closed Thu Jun 24 00:00:49 2021