Halt processing of unrequested transactions (p2p)

https://github.com/bitcoin/bitcoin/pull/21224

Host: ellemouton  -  PR author: ariard

The PR branch HEAD was 63103da at the time of this review club meeting.

Notes

  • Transaction relay is a three step process: inv -> getdata -> tx:

    1. the relaying node sends an inv message to the receiving node to announce a new transaction.
    2. if the receiving node wants to retrieve the transaction that it learned about in the inv message then it will send a getdata message to the relaying node to request the full transaction. (The receiving node wont send a getdata message to a peer for transactions that it has already seen or if it has already sent a getdata message for the transaction to a different peer.)
    3. the relaying node delivers a tx message to the receiving node. If the relaying node is no longer able to deliver the transaction, it responds with notfound instead of the tx.

    You can learn more about these messages here.

  • Currently if a tx message is received it will not be processed if:

    1. The node is in blocks-only mode and the peer sending the message has no relay permissions
    2. The peer is a blocks-relay only peer

    Otherwise, the node checks if it has already processed this transaction before using the AlreadyHaveTx() checks and if it not then the transaction is processed by AcceptToMemoryPool(). This is done even if it has not made a request for that transaction (i.e: If it recieves a tx message without having first sent a getdata message asking for the tx message then it will still process the transaction). In other words the 3 step process described above is not enforced.

  • This PR aims to change that by requiring a transaction to first be requested by a node before it processes an accociated tx message.

  • The PR author provides additional motivation for the PR and raises some of the concerns that this change brings about in a mailing list thread.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. What is the TxRequestTracker class in src/txrequest.h used for?

  3. What are the different states that a peer/tx “announcement” can be in? Where in the code is an announcement shifted between the different states?

  4. What does the author claim the problem is with the current way in which transaction messages are handled?

  5. What is the change that this PR is making and how does it solve the problem mentioned in Q4?

  6. When checking the TxRequestTracker to see if the node has requested a transaction, why are both the transaction’s txid and wtxid identifiers used? (hint: See PR 19569 for details on fetching orphan parents from wtxid peers)

  7. There are a lot of switches on the PF_RELAY flag in code relevant to this PR. What does the PF_RELAY flag mean? In what case(s) would you want to set this flag for a peer?

  8. Discussion: Since the inv -> getdata -> tx sequence has not been necessary for communicating and receiving transaction data, some other clients don’t bother with the sequence at all. This means that if this change was deployed to Bitcoin Core nodes then other clients would not be able to relay transactions to upgraded Bitcoin Core nodes. Eventually upgraded nodes would make up the majority of the network, and so those clients would have to adapt and update. Do the pros out weigh these cons? And if so, what is fair time frame to allow for the other clients to adapt?

Meeting Log

  112:00 <jnewbery> #startmeeting
  212:00 <emzy> hi
  312:00 <kanzure> hi
  412:00 <pinheadmz> hi
  512:00 <dergoegge> hi
  612:00 <maqusat> hi
  712:00 <amiti> hi!
  812:00 <AnthonyRonning> hi
  912:00 <schmidty> hi
 1012:00 <jnewbery> hi folks! It's Wednesday afternoon (at least where I am). You know what that means - another edition of Bitcoin Core PR Review Club.
 1112:00 <elle> hey hey!
 1212:00 <andozw> hiii
 1312:00 <jnewbery> Feel free to say hi to let everyone know you're here.
 1412:00 <ariard> hi
 1512:00 <sdaftuar> hi
 1612:00 <glozow> hi
 1712:00 <jnewbery> If this is your first time, there are some tips here to help you get the most out of your time with us: https://bitcoincore.reviews/your-first-meeting
 1812:00 <michaelfolkson> hi
 1912:00 <jnewbery> Anyone here for this first time?
 2012:00 <sunon> hi
 2112:01 <abstract> hi
 2212:01 <elle> no waaayyys hi craigraw !
 2312:01 <craigraw> Hi all
 2412:01 <jnewbery> welcome craigraw :)
 2512:01 <jnewbery> Notes and questions where you'd expect them: https://bitcoincore.reviews/21224
 2612:02 <jnewbery> This week, we're very lucky to have special guest host elle. It's going to be lekker 😎
 2712:02 <emzy> craigraw: small world :)
 2812:02 <jnewbery> over to you, elle
 2912:02 <elle> Thanks john! Hi everyone! Today we are looking at #21224: Halt processing of unrequested transactions. It was originally part of #20277 but has recently been split off to its own PR. The PR touches some interesting parts of the code that handles how we learn about new transactions. Let’s dive in!
 3012:02 <elle> Also, feel free to ask questions at any time. We are all here to support each other and learn. If you are confused about something, chances are that others are confused about it too and so asking your question will benefit everyone :) I’m also a newbie so please correct me if I misspeak.
 3112:02 <elle> Ok cool, lets get cracking!
 3212:02 <elle> 1. First of all, who has had a chance to review the PR? (y/n). No problem if you haven’t.
 3312:02 <amiti> y
 3412:03 <pinheadmz> y
 3512:03 <jnewbery> y
 3612:03 <felixweis> hi
 3712:03 <AnthonyRonning> conceptually yes, code no :)
 3812:03 <emzy> n
 3912:03 <dergoegge> n
 4012:03 <craigraw> n
 4112:03 <elle> awesomeness! No problem if you havent. We are gonna get into it all now. Lets do this!
 4212:03 <schmidty> y
 4312:04 <maqusat> conceptually yes and partially code
 4412:04 <elle> 2. What is the `TxRequestTracker` class in src/txrequest.h used for?
 4512:04 <pinheadmz> elle its a data sturcture that acts like a relational database to track transaction requests to peers
 4612:04 <dergoegge> managing tx downloads from peers
 4712:05 <jnewbery> pinheadmz: I'd say it's a class rather than a data structure
 4812:05 <pinheadmz> jnewbery ty
 4912:05 <jnewbery> (distinction being that it also has member functions)
 5012:06 <pinheadmz> ah ok works for me
 5112:06 <elle> pinheadmz: dergoegge: indeed! it is used to keep track of the txs that our peers have notified us about and then we also use it to determine which peers we will request the txs from and when we will do so
 5212:06 <elle> what data does it hold? what is its most important member?
 5312:07 <jnewbery> introduced in PR 19988: https://github.com/bitcoin/bitcoin/pull/19988 🥰
 5412:07 <pinheadmz> most important member is the `Index` which is a multi-index um.... thing from boost
 5512:07 <pinheadmz> jnewbery is _that_ a data structure? or a class lol. i guess class ?
 5612:07 <sdaftuar> data structure seems appropriate for multi_index :)
 5712:08 <pinheadmz> sdaftuar 🙏
 5812:08 <elle> pinheadmz: yes indeed. And the index is pretty much a collection of announcements.
 5912:08 <elle> Where each announcement is what?
 6012:09 <glozow> stored and multi-indexed, so you know what to request from which peers, when
 6112:09 <pinheadmz> hm an Announcement is a struct
 6212:09 <pinheadmz> with a txid and a peer id
 6312:09 <glozow> and what to evict
 6412:09 <amiti> its stored as a struct and represents that a peer notified us about a txn, stored until we either get the txn or give up
 6512:09 <pinheadmz> and a state !
 6612:10 <elle> yebo yes! pretty much a peer/txhash combo + state
 6712:10 <elle> and yes pinheadmz that leads us nicely into the next question:
 6812:10 <elle> Im gonna break question 3 up into 2 parts cause it is chunky
 6912:11 <elle> 3.1: First of all: `TxRequestTracker` manages our announcements using a state machine. What are the different states that an announcement can be in?
 7012:11 <pinheadmz> CANDIDATE_DELAYED, CANDIDATE_READY, CANDIDATE_BEST, REQUESTED, COMPLETED
 7112:11 <elle> Yebo!
 7212:11 <pinheadmz> yebo!
 7312:11 <pinheadmz> (more s african slang? :-P )
 7412:12 <elle> You can see the enum here: https://github.com/bitcoin/bitcoin/blob/b54a10e777f912081e5c00dabcf3643b10775a50/src/txrequest.cpp#L39
 7512:12 <sunon> Zulu for yes :)
 7612:12 <elle> haha yes sorry yebo is SA slang :) whoops, it is a habbit
 7712:12 <sunon> Ah I didn't see the other candidate states
 7812:13 <elle> There is also a nice desciption of the 3 overall states here: https://github.com/bitcoin/bitcoin/blob/b54a10e777f912081e5c00dabcf3643b10775a50/src/txrequest.h#L106-L121
 7912:14 <elle> Does someone want to tell us what the 3 overall states mean?
 8012:14 <amiti> CANDIDATE* represents that a peer has announced the tx to us (sent us an INV)
 8112:14 <amiti> REQUESTED represents that we have sent a GETDATA for the tx
 8212:15 <amiti> and COMPLETED means we either got the txn, a NOTFOUND, or the response timed out
 8312:15 <elle> amiti: Nice one! perfect
 8412:15 <dergoegge> when are COMPLETED announcements deleted? i read that they are kept for while because we need them for deduplication
 8512:17 <elle> hmmm im not sure. Will have a look now. Does anyone else know?
 8612:17 <jnewbery> dergoegge: once we've received a transaction or the request state is COMPLETED for all peers (i.e. we don't have anyone else we can ask for the tx)
 8712:18 <michaelfolkson> This is all relatively new code ~ 5 months old. Easy to follow and comments are great
 8812:18 <dergoegge> jnewbery: i see ty
 8912:18 <elle> thanks jnewbery!
 9012:19 <elle> Cool so lets quickly go through where an announcement is shifter between the various states
 9112:19 <eoin> jnewbery: what is a pull request?
 9212:19 <jnewbery> elle: dit is 'n plesier
 9312:19 <pinheadmz> eoin a PR is a proposal to change the source code
 9412:20 <elle> First of all, where do we insert a new announcement? amiti gave us a hint earlier
 9512:20 <amiti> the code to actually shift the states all lays in the txrequest module, but the triggers are in net processing
 9612:20 <elle> amiti: indeed. Where in net_processing does the trigger for adding a new announcement happen?
 9712:21 <eoin> ty pinheadmz
 9812:21 <amiti> I think the main events that kick off state changes are when we ProcessMessage INV, TX, NOTFOUND or when we SendMessages GETDATA
 9912:21 <amiti> so, new announcement is ProcessMessage INV
10012:21 <amiti> or if we are dealing with orphans there's also a code path in ProcessMessage TX to try to request the parent
10112:22 <glozow> what if we see tx in a block?
10212:22 <elle> amiti: exactly. Which TxRequestMethod do we call to add the new announcement?
10312:22 <abstract> eoin if thenomenclature of pull rather than push is confusing, despite the request originating from someone who's changing something (hence a push), it's a request to the repository maintainer to "pull" in the code to the repo
10412:23 <glozow> oh i just realized that was a dumb question oops
10512:23 <glozow> nvm
10612:23 <pinheadmz> glozow i dont think thats dumb, im looking for an asnwer
10712:23 <pinheadmz> it should delete tx requests once a tx is confirmed right?
10812:24 <elle> pinheadmz: that is a good point... im guessing that maybe the `ForgetTxHash` might be called then? im not sure
10912:24 <glozow> yeah definitely should, trying to find where the code path is
11012:24 <amiti> glozow: are you asking about if we see a txn in a block for the first time?
11112:24 <pinheadmz> amiti either / or
11212:24 <amiti> or if we have a txn announcement / in flight and hten we see it in a block
11312:24 <glozow> yeah if we got an inv, maybe sent a getdata, and then we see the actual tx itself in the context of a block
11412:25 <amiti> gotcha
11512:25 <jnewbery> glozow pinheadmz: it's a great question! Take a look here: https://github.com/bitcoin/bitcoin/blob/b54a10e777f912081e5c00dabcf3643b10775a50/src/net_processing.cpp#L1463-L1466
11612:25 <pinheadmz> jnewbery great! so fast
11712:25 <elle> ah! nice one
11812:25 <glozow> ahhh `BlockConnected`
11912:25 <jnewbery> if a tx is confirmed in a block, we don't need to request it anymore, so we delete it from TxRequest
12012:25 <pinheadmz> ah BlockConnected -- same function that evicts confirmed tx from mempool
12112:25 <glozow> yebo jnewbery! (am i using it right?)
12212:25 <pinheadmz> yebo lekker yebo!
12312:26 <jnewbery> glozow: bakgat!
12412:26 <elle> wow guys, hahaha spreading the Afrikaans culture here
12512:27 <elle> ok, lets continue. I think you all understand the state machine so lets move on to question 4
12612:27 <elle> 4. What does the author claim the problem is with the current way in which transaction messages are handled?
12712:27 <emzy> DoS / DDoS possibility by opening many connectionions to a node and send junk transactions that are expensive to validate but. I think after the PR we will be limited by the in-flight transactions?
12812:27 <AnthonyRonning> Transactions could be relayed to a node without solicitation via INV. Results in additional CPU processing which could be abused.
12912:28 <elle> emzy AnthonyRonning: yes exactly. Currently the INV/GETDATA sequence is not enforced
13012:28 <dergoegge> the inv does not prevent DoS though right? inv(junk txs) -> getdata -> tx(junk txs) also works for an attacker
13112:29 <elle> dergoegge: good point!
13212:29 <pinheadmz> dergoegge not to mention... https://invdos.net/
13312:29 <AnthonyRonning> dergoegge: that's my understanding too
13412:29 <pinheadmz> inv messages have had their own issues
13512:29 <ariard> dergoegge: when you're thinking about DoS, it's not a binary question, is a low-fee, expensive-to-validate tx a DoS in period of mempool congestion?
13612:30 <jnewbery> pinheadmz: that particular issue was fixed several releases ago
13712:30 <pinheadmz> jnewbery yah
13812:30 <pinheadmz> jnewbery braydon and i were coworkers for 2 years, he wouldnt even tell me! just that he had a CVE #
13912:31 <elle> 5. What is the change that this PR is making and how does it solve the problem mentioned in Q4?
14012:31 <emzy> A node only processes TX messages that it requsted via GETDATA first.
14112:31 <pinheadmz> elle simply, when it receives a TX message it checks if it was ever requested and if not, drops the tx
14212:32 <elle> emzy: pinheadmz: exactly
14312:32 <pinheadmz> although i notice it does not ban the peer -- it would if the peer was conecnted as blocks_only however
14412:32 <jnewbery> pinheadmz: I think "ever requested _from that peer_
14512:32 <pinheadmz> jnewbery yes
14612:33 <elle> pinheadmz: it cant ban the peer because up till now, the INV/GETDATA sequence has not been needed so some clients dont even bother with it
14712:33 <pinheadmz> p.s. jnewbery so there is only one global TxRequestTracker right? and it gets added to each peer object upon instnatiation ?
14812:33 <pinheadmz> elle ah right
14912:34 <AnthonyRonning> elle: do you know what clients don't do that?
15012:34 <jnewbery> there's only one TxRequestTracker object that gets instantiated when PeerManagerImpl is constructed
15112:34 <elle> ah, jnewbery i think you know of a client that does that?
15212:34 <glozow> does it solve the DoS problem? idk if it would stop an attacker that's sending us cpu-intensive transactions, they can still send plenty of invs and we'll ask for them. I like jnewbery's good highlight of aj's good point: "If what we want to do is rate-limit TX messages we receive from a peer IMO we should literally do that."
15312:34 <felixweis> im not certain but maybe bitcoinj?
15412:35 <jnewbery> it doesn't get added to the Peer objects. It just tracks individual announcements (and makes sure they're cleared up when a peer disconnect, for example)
15512:35 <elle> glozow: yeah that is a great point
15612:35 <jnewbery> yeah bitcoinj is one: https://github.com/bitcoinj/bitcoinj/blob/7d2d8d7792ec5f4ce07ff82980b4e723993221e8/core/src/main/java/org/bitcoinj/core/TransactionBroadcast.java#L145
15712:35 <AnthonyRonning> jnewbery: ty
15812:35 <sdaftuar> glozow: this PR woudl not solve any DoS problems at the moment (though it would achieve ariard's original goal of ignoring unrequested transactions during IBD)
15912:36 <sdaftuar> jnewbery: thanks for sharing that (has anyone commented on the PR or mailing list to that effect? i did not know)
16012:36 <AnthonyRonning> "blast out the TX here for a couple of reasons. Firstly it's simpler" lol
16112:36 <glozow> ignoring unrequested transactions during IBD does make sense to me - do we even have a mempool to add them to?
16212:36 <felixweis> bitcoinj, after it sends a constructed transaction to a peer it does however check for INV messages for that hash on other peers to build confidence the wallet transaction has propagated trough the bitcoin network
16312:36 <elle> why cant we just ignore all txs during IBD?
16412:37 <ben2> Ho often a node receive unrequested txs ? Why would an honest node send unrequested tx ?
16512:37 <ben2> * How
16612:37 <sdaftuar> my (mild) objection to ariard's original proposal is that i think it complicates our logic to special-case things for IBD, especially when it makes sense that they shoudl hold true generally
16712:37 <pinheadmz> glozow no mempool during IBD - how could we verify ?
16812:37 <ariard> glozow: yes but our utxo set might lead us to relay invalid transactions (their utxos has already been spent at tip), and feerate evaluation of such unrequested is really uncertain
16912:37 <jnewbery> sdaftuar: harding shared that with me. I asked whether he thought it should be pointed out on the mailing list, but his understanding was that the author was already aware that clients were doing this
17012:38 <sdaftuar> jnewbery: i was certainly unaware, fyi
17112:38 <jnewbery> from antoine's mailing list post:
17212:38 <jnewbery> Raw TX message processing has always been tolerated by Core and as such
17312:38 <jnewbery> some Bitcoin clients aren't bothering with an INV/GETDATA sequence.
17412:38 <AnthonyRonning> ben2: check out that link jnewbery shared, it has a paragraph of comments as to why the honest node sends unrequested tx's
17512:38 <jnewbery> he didn't think pointing out individual examples of clients that did that would be helpful
17612:39 <sdaftuar> jnewbery: imo it's helpful to know how much we're breaking things (if at all) with changes like this. ie i don't know if it's just researchers doing tests that are the soruce of unrequested transactions, or wallets with widespread communities or what
17712:39 <elle> Quick implementation detail question:
17812:39 <elle> 6. When checking the `TxRequestTracker` to see if the node has requested a transaction, why are both the transaction’s txid and wtxid identifiers used? (hint: See PR 19569 for details on fetching orphan parents from wtxid peers)
17912:39 <ariard> yeah I think bitcoinj has been pointed to me by phantomcircuit a while back
18012:39 <glozow> ariard: it's surprising to me that we try to verify during IBD :O
18112:39 <elle> glozow: agreed
18212:39 <dergoegge> glozow: same
18312:40 <eoin> What does 'IBD' stand for?
18412:40 <felixweis> initial block download
18512:40 <elle> which is why i ask why we cant just ignore all Tx messages during IBD..?
18612:40 <felixweis> when you first start to run a bitcoin node, and all you know is the genesis block
18712:41 <amiti> elle: good question, I'm wondering too
18812:41 <ariard> glozow: I did react the same while reviewing tx request overhaul, and such opening the previous PR
18912:41 <eoin> ty felixweis
19012:42 <jnewbery> eoin: when we first start the node and we're catching up to the tip, we're in a state called IBD until we catch up. That effects various parts of our logic. I don't think all those differences are documented in a single place, but this comment from sipa in a different PR is a good overview: https://github.com/bitcoin/bitcoin/pull/21106#issuecomment-783676898
19112:42 <glozow> ariard: is there any consideration for separating the during-IBD case with the non-IBD case?
19212:42 <amiti> elle: I think we can?
19312:42 <amiti> ariard: was your previous proposal to ignore all txs during IBD? or just unrequested ones?
19412:42 <ariard> amiti: this what my initial motivation until suhas came with the idea of bouncing off unrequested txn, ibd-or-not
19512:42 <glozow> amiti: do we ever request during IBD?
19612:42 <felixweis> eoin: you start requesting/downloading all historical blocks from archival peers and verify their integrity. during that time you can't verify the integrity of almost all recent mempool transactions because they are spending coins created at a much later point in time anyway.
19712:43 <pinheadmz> glozow i thin kthats the question! i dont think we do, but i see the INV messages in the log during IBD
19812:43 <ariard> glozow: yes see the point above breaking raw tx broadcast bitcoin clients
19912:43 <ariard> amiti: all txs, we ignore inv(tx) during IBD
20012:43 <glozow> pinheadmz: you might get an inv, but you shouldn't send a getdata right?
20112:44 <eoin> Is IBD when you first download Bitcoin Core and are downloading the blockchain?
20212:44 <pinheadmz> glozow ariard looking for this logic in net_processing...
20312:44 <felixweis> eoin: correct
20412:45 <ariard> pinheadmz: L2898, net_processing.cpp
20512:45 <glozow> pinheadmz: i think it's this https://github.com/bitcoin/bitcoin/blob/b54a10e777f912081e5c00dabcf3643b10775a50/src/net_processing.cpp#L2973
20612:45 <jnewbery> glozow: yebo
20712:45 <felixweis> your node will use the memory allocated for the mempool (default 300MB) for the dbcache. that speeds up the generation of your local UTXO set because it needs to write to disk less often.
20812:46 <ariard> glozow: yeah this one, not on the same master :p
20912:46 <pinheadmz> `fImporting` ?
21012:46 <emzy> felixweis: interesting detail.
21112:46 <jnewbery> felixweis eoin: these are good questions/answers, but a little off-topic. Perhaps we can circle back to them after the meeting?
21212:46 <glozow> pinheadmz: I think importing from blocks db?
21312:46 <pinheadmz> felixweis yes agreed, interesting optimization
21412:46 <elle> ok cool yeah so we dont add the new announcement to TxRequestTracker if we are in IBD and so never send out a GETDATA
21512:47 <eoin> jnewbery: Ok
21612:47 <elle> Let's navigate back to question 6 (we can skip 7 and do the discussion question after):
21712:47 <elle> 6. When checking the `TxRequestTracker` to see if the node has requested a transaction, why are both the transaction’s txid and wtxid identifiers used? (hint: See PR 19569 for details on fetching orphan parents from wtxid peers)
21812:48 <pinheadmz> elle some peers dont relay by wtxid yet ?
21912:49 <elle> pinheadmz: true, but we need to check both even on a wtxid relay connection
22012:49 <elle> why?
22112:49 <jnewbery> pinheadmz: will eventually all tx fetching be done with wtxid, or are there some cases where txid will still have to be used?
22212:50 <sdaftuar> jnewbery: i sure hope eventually all tx fetching will be done by wtxid, fwiw
22312:50 <glozow> oh, we wouldn't know if a txid and wtxid are for the same thing until we see the tx, right?
22412:50 <pinheadmz> jnewbery i hope this is a trick Q but i feel like if all nodes upgrade then yeah wtxid is all we'll need
22512:51 <glozow> and if you have an orphan, it refers to its parent via txid?
22612:51 <elle> glozow: yebo yes!
22712:51 <jnewbery> glozow: exactly!
22812:51 <ariard> sdaftuar: i think we should, it would prevent tx-relay interferences around non-witness txn mutated with witness
22912:51 <pinheadmz> glozow that makes sense! bc the wtxid isnt in the prevout, the txid is !
23012:51 <elle> pinheadmz: indeedio
23112:51 <ariard> a case not documented around your rejection filters big warnings
23212:51 <jnewbery> the only way we can fetch an orphan's parent is by its txid (since that's what the txin of the child refers to)
23312:52 <jnewbery> sdaftuar: I guess you mean after we have package relay and no longer do orphan handling in the same way?
23412:52 <elle> Cool lets skip 7 and move onto the discusssion. The question is specific but we can also talk more generally about the pros vs cons of this change:
23512:52 <sdaftuar> right, somehow we should make orphan handling better
23612:53 <ariard> jnewbery: epends if package relay gets its own identifier or not...
23712:53 <sdaftuar> for instance, by a p2p change to request all unconfirmed ancestors, that sort of thing
23812:53 <jnewbery> sdaftuar: +1
23912:53 <glozow> ariard: like package id?
24012:54 <glozow> wow package relay sounds great
24112:54 <ariard> glozow: yeah to avoid someone sticking parent-or-children in rejection filter, and thus interfering with your package evaluation
24212:55 <elle> very cool
24312:55 <ariard> glozow: I really hope we would deprecate orphans once we have pacakge relay
24412:56 <elle> but that would result in some duplication right? Cause the parent would be relayed and then when the child comes then the package would be relayed etc etc?
24512:56 <elle> short on time. here is question 8: 8. Discussion: Since the `inv` -> `getdata` -> `tx` sequence has not been necessary for communicating and receiving transaction data, some other clients don’t bother with the sequence at all. This means that if this change was deployed to Bitcoin Core nodes then other clients would not be able to relay transactions to upgraded Bitcoin Core nodes.
24612:56 <elle> Eventually upgraded nodes would make up the majority of the network, and so those clients would have to adapt and update. Do the pros out weigh these cons? And if so, what is fair time frame to allow for the other clients to adapt?
24712:56 <sdaftuar> is bitcoinj the only client we're aware of that does this?
24812:56 <ariard> elle: or you do atomic-package-relay, sounds harder but promise better bandwidth gains
24912:57 <felixweis> im still wondering if packaging, not requesting an orphans parents twice and minisketch can be combined
25012:57 <emzy> Maybe the can do the same as blomfilters (for bitcoinj)? We can generally allow it the same way via a config option.
25112:57 <ariard> elle: I think this question is really interesting because it underscores the fact that Core as a project doesn't have a process for p2p breaking changes
25212:58 <AnthonyRonning> maybe we can do something like if 95% of all relayed transactions have gone through via inv -> getdata -> tx then we enforce it :)
25312:58 <jnewbery> I think the Andreas Schildback android bitcoin wallet was based on the same codebase, so I'd guess it does the same. I don't know of any others (but I wouldn't)
25412:58 <ariard> samurai wallet is java based IIRC
25512:58 <sdaftuar> well i wonder if those clients only connect to NODE_BLOOM peers anyway, then this could potentially be non-breaking (if we wanted it)
25612:58 <felixweis> maybe BRD is using bitcoinj as well
25712:58 <emzy> jnewbery: Bisq is another.
25812:58 <pinheadmz> felixweis i dont think so, BRD is written in C but im going to check ;-)
25912:59 <pinheadmz> i checked bcoin and it does queue all tx broadcastss in INV messages
26012:59 <felixweis> pinheadmz: ok then ignore what i just said
26112:59 <pinheadmz> https://github.com/breadwallet/breadwallet-core/blob/8eb05454df3e2d5cca248b4e24eeffa420c97e3a/bitcoin/BRPeerManager.c#L544
26212:59 <glozow> wait, is the proposal that if they have NODE_BLOOM we're ok with their unrequested txns?
26312:59 <pinheadmz> looking good so far
26412:59 <emzy> But we have at Bisq already another way to broadcast TX. So It is not true anymore.
26513:00 <sdaftuar> i think aj (and jnewbery) made arguments that this is undesirable regardless of whether this is breaking (at least, no discussion on the PR indicated that people thought this was breaking, though it's an open question)
26613:00 <michaelfolkson> What was harding's rationale for not wanting to highlight a specific alternative implementation jnewbery? Not wanting to highlight something that is no longer well maintained?
26713:01 <jnewbery> Yeah, lots of good discussion on the PR between aj and sdaftuar. I'd recommend reading it for a good lesson in "considerations when making p2p protocol changes"
26813:01 <sdaftuar> glozow: right, you could imagine that if we thought this was a good change, but just didn't want to break existing software, that if that existing software doens't connect to default Bitcoin COre nodes anyway then maybe there's room to make a change, at least for users who aren't enabling NODE_BLOOM on their own node
26913:01 <AnthonyRonning> it sounded like it doesn't solve the attack vector, not alone at least.
27013:01 <elle> #endmeeting