--- Day changed Wed Aug 28 2019
00:06 -!- pinheadmz [~matthewzi@37.72.175.72] has quit [Quit: pinheadmz]     
03:21 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has quit [Ping timeout: 252 seconds]     
03:24 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has joined #bitcoin-core-pr-reviews     
05:05 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection]     
05:06 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
06:26 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has quit [Quit: jonatack]     
06:55 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has joined #bitcoin-core-pr-reviews     
07:04 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ynxrmrbcdnwkrwpt] has joined #bitcoin-core-pr-reviews     
07:34 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Ping timeout: 260 seconds]     
07:35 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
07:56 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Read error: Connection reset by peer]     
07:56 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection]     
07:57 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
07:57 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
08:03 -!- pinheadmz [~matthewzi@37.72.175.72] has joined #bitcoin-core-pr-reviews     
08:10 -!- hebasto [~hebasto@95.164.65.194] has joined #bitcoin-core-pr-reviews     
08:11 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection]     
08:11 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
08:13 -!- ccdle12 [~ccdle12@cpc139350-aztw33-2-0-cust310.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews     
08:23 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection]     
08:25 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
08:29 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection]     
08:29 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Read error: Connection reset by peer]     
08:29 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
08:29 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
08:30 -!- lightlike [~lightlike@2001:16b8:573b:900:607b:a6e7:8a81:763b] has joined #bitcoin-core-pr-reviews     
08:34 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Ping timeout: 260 seconds]     
08:34 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 248 seconds]     
08:34 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
08:35 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
08:40 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection]     
08:40 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Read error: Connection reset by peer]     
08:40 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
08:40 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
08:54 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 245 seconds]     
08:54 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Ping timeout: 260 seconds]     
09:11 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
09:21 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
09:38 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews     
09:43 < jnewbery> we'll get started in about 15 minutes. It's a good one this week!
09:45 -!- dergigi [~dergigi@37.48.119.130] has joined #bitcoin-core-pr-reviews     
09:46 -!- afigs [642bef32@100-43-239-50.static-ip.telepacific.net] has joined #bitcoin-core-pr-reviews     
09:48 -!- marcinja_ [~marcinja@204.48.26.93] has joined #bitcoin-core-pr-reviews     
09:52 -!- coisinho [59f67a44@i59F67A44.versanet.de] has joined #bitcoin-core-pr-reviews     
09:58 -!- jnewbery_ [~john@pool-108-21-57-150.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews     
09:59 -!- probably_dillon [4516f113@user-12hds8j.cable.mindspring.com] has joined #bitcoin-core-pr-reviews     
09:59 -!- michaelfolkson [~textual@146.185.56.214] has joined #bitcoin-core-pr-reviews     
09:59 -!- hanhua [68840050@104.132.0.80] has joined #bitcoin-core-pr-reviews     
10:00 < jnewbery> hi     
10:00 < dergigi> hi     
10:00 < marcinja_> hi     
10:00 < ccdle12> hi     
10:00 < jkczyz> hi     
10:00 < lightlike> hi     
10:00 < michaelfolkson> Hey     
10:01 < afigs> sup     
10:01 < nehan> hi     
10:01 < jnewbery> before we start, a couple of reminders:
10:01 < jonatack> hi     
10:01 < jnewbery> - I don't have any more PRs lined up on the agenda. If you have any that you want to request, please comment in https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14
10:02 < jnewbery> - weekly reminder that PR review club exists to help you get reviewing on github. I encourage you all to leave review/test comments on the actual PR!
10:03 < jnewbery> if you have any questions or thoughts about leaving review comments on PRs, they're all on topic in this channel
10:03 -!- phuvp [41d13c92@65.209.60.146] has joined #bitcoin-core-pr-reviews     
10:03 < jnewbery> ok, let's get started. What did you all think about the PR this week? Who had a chance to review/test?
10:04 < michaelfolkson> On that I was thinking of maybe starting this 15 minutes early each week for those who want to chat about basic stuff without wasting John's time
10:04 < jnewbery> no, basic stuff is on topic
10:04 < jnewbery> please feel free to ask during the meeting
10:04 < jnewbery> IRC conversations can be multi-threaded!
10:04 < jonatack> built, ran tests, started to review and grep code
10:04 < michaelfolkson> Ok     
10:05 -!- phuvp [41d13c92@65.209.60.146] has left #bitcoin-core-pr-reviews []     
10:05 < ariard> had a chance to review, like weeks ago
10:05 < lightlike> i tested this a couple of weeks ago, didnn't test the newest fixes.
10:05 < jkczyz> Read through comments and code yesterday/today. Added some minor comment this morning.
10:05 < jnewbery> I see review comments from ariard, lighlike and jkczyz. Great stuff!
10:06 < jnewbery> Any questions about the notes from https://bitcoin-core-review-club.github.io/15759.html ? Anything unclear?
10:06 < jnewbery> And does anyone want to have a go at the first question "What problem is this PR addressing?
10:06 -!- phvup [41d13c92@65.209.60.146] has joined #bitcoin-core-pr-reviews     
10:06 < jonatack> This PR proposes to mitigate the risk of eclipse attacks by separating block
10:07 < jonatack> relay from transaction relay to occlude the network topology
10:07 < dergigi> I built it and looked through the code and the PR; don't have any comments really, still have a lot to learn and wrap my head around; wasn't sure how to test it properly; network tests from PR 14210 would be great I guess
10:07 < jonatack> (that was the easiest question... I *think* :D)
10:08 < jnewbery> dergigi: yeah, our network testing in the functional test suite is a bit deficient. It'd be nice to be able to test outbound connections
10:08 < jnewbery> jonatack: yeah, that's right
10:09 < dergigi> This PR relates to the Erebus Attack as well, right? At least it blocks/conflicts with PR 16702 which implements one of the proposed countermeasures?
10:09 < jnewbery> anyone want to expand on the motivation, or shall we move onto the next question?
10:09 < michaelfolkson> Basic question on the peer management. You initially connect to the DNS seeds but then these connections are dropped and that's where you're at risk of eclipse attack (assuming you haven't manually connected to a peer that you trust)?
10:10 < provoostenator> michaelfolkson: you're at risk with or without the DNS seeds
10:10 < michaelfolkson> You only need one peer to serve you correct transactions/blocks and you are able to detect attack
10:11 < jnewbery> dergigi: I haven't read the Erebus paper, but this PR was opened before that paper was released I believe
10:11 < provoostenator> You won't necessarily detect the attack, but you'll get the proper chain.
10:11 < provoostenator> The DNS seeds are used to get an initial set of IP addresses to try, you then contact those for more, etc.
10:11 < jnewbery> I don't think this PR is directly concerned with Erebus. PRs that change P2P code often conflict
10:11 < michaelfolkson> <provoostenator>: But if you maintained your connection to the DNS seeds then the transactions/blocks they provide would be different to the attacker's?
10:11 < lightlike> I found it interesting that fixing the root cause of topology inference (the way orphan txs are handled) is seen a "lost cause".
10:12 < provoostenator> michaelfolkson:  you don't maintain that connection; you only poll them once and only if you can't find a peer by yourself quickly enough (11 seconds from boot)
10:12 < ariard> lightlike: you think better management of tx dependencies and their conflicts between peers could solve it ?
10:13 < jnewbery> lightlike: I think there's a tradeoff between bandwidth usage and privacy. If you're only connecting to a subset of the network and relaying txs to them, then that leaks information about your topology
10:13 < michaelfolkson> <provoostenator>: What's to stop me manually maintaining the connection to them?
10:13 < lightlike> ariard: I don't know. The author doesn't believe in that: https://github.com/bitcoin/bitcoin/pull/15759#issuecomment-480398802
10:14 < provoostenator> michaelfolkson: when your node starts it checks peers.dat for a list of peers it already knows about and tries to connect to some of them
10:14 < provoostenator> Only when that fails does it call the DNS seeds.
10:14 < provoostenator> michaelfolkson: DNS seeds are not nodes, the only thing they do is return a list of IP addresses.
10:14 < michaelfolkson> I want to maintain a connection to at least one peer I trust and then I don't have to worry about eclipse attacks
10:15 < jnewbery> michaelfolkson: use addnode=<address> to your config
10:15 < provoostenator> Try this:
10:15 < provoostenator> dig -t A seed.bitcoin.sprovoost.nl
10:15 < jnewbery> ok, question two: Why is the tx relay inventory state moved into its own structure?
10:15 < provoostenator> DNS uses port 53 and has a completely different protocol than the Bitcoin P2P network (port 8333)
10:15 < michaelfolkson> Ah thanks
10:16 < jkczyz> To separate state that is not needed for blocks-only peers
10:16 < provoostenator> "I don't have to worry about eclipse attacks" - well, then *that* peer has to worry
10:16 < ariard> lightlike: was thinking about this yesterday and intuitively would say as tx conflicts are solved by blocks you can always provoke conflicts between all you peers as it's unconfirmed data
10:16 < jnewbery> jkczyz: correct. Why?
10:17 < ariard> so patching all heuristics beyond TxProbe seems it to be really hard
10:17 < michaelfolkson> <provoostenator> Maybe those people named on that DNS seed list have a better understanding of the network than me ;)
10:17 -!- phvup [41d13c92@65.209.60.146] has quit [Remote host closed the connection]     
10:17 < jkczyz> jnewbery: the state is not applicable to them since they are not relaying transactions
10:17 < provoostenator> I'm just good at copy-pasting. And actually had to fix a bunch of issues of time.
10:17 < jnewbery> that's right, but why not just not use that state?
10:18 < ariard> michaelfolkson: you should have multiple source of blocks beyond your ISP if so
10:18 < jkczyz> jnewbery: not sure if I parsed that one correctly
10:18 < jnewbery> why not just not initialize that state and don't use it?
10:19 < ariard> reduce memory comsumption
10:19 < jnewbery> why go to the trouble of defining a structure and adding a unique_ptr
10:19 < jkczyz> Ah, yes memory and because it is used to determine if it is blocks-only
10:19 < jnewbery> correct: the justification was to reduce memory consumption (sorry jkczyz - maybe that was just too obvious!)
10:20 < jnewbery> Does the tx relay state require a lot of memory? If so, where?
10:20 -!- pinheadmz_ [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
10:20 < jkczyz> jnewbery: haha, np. I also found it a bit disconcerting that CNode has over 60 member variables
10:21 < lightlike> ariard: yes, but do these conflicts necessarily have to lead to an attacker learning the entire network topology as described in the txprobe paper?
10:21 -!- pinheadmz [~matthewzi@37.72.175.72] has quit [Ping timeout: 245 seconds]     
10:21 -!- pinheadmz_ is now known as pinheadmz     
10:21 < ariard> tx relay state requires memory for inv of txid to send
10:21 < nehan> jnewbery: does it have anything to do with the bloom filter?
10:22 < aj> ariard: but if you don't send inv's that would just be an empty set, which shouldn't take up much memory?
10:23 < jnewbery> nehan: yes. It's the bloom filters that are expensive. I think without those, it wouldn't really be worth changing this (for memory saving - it might be worth it to make the code clearer)
10:23 < jnewbery> There are a couple of bloom filters used by CNode. What are they and what are they for?
10:24 < nehan> jnewbery: it's unclear to my why an unused bloom filter should take up a lot ofmemory
10:24 < ariard> aj: oh yes, assuming we don't fulfill them by mistake in some part of the code
10:24 < jnewbery> ariard: the invs are stored in a std::set, so if no invs are ever put in there, then it's not very expensive
10:25 < nehan> jnewbery: i guess it's this:     vData(std::min((unsigned int)(-1  / LN2SQUARED * nElements * log(nFPRate)), MAX_BLOOM_FILTER_SIZE * 8) / 8),
10:25 < nehan> jnewbery: but another option would be not to initialize until use
10:25 < jnewbery> nehan: that's it
10:26 < jnewbery> nehan: I guess you could do that, but then I think you'd need to update all the sites that initialize and use bloom filters
10:27 < ariard> lightlike: don't know enough about mempool, but would say if peers have different tx dependcy graph, it's going to modify the way they relay tx (you may have other heuristics not yet discovered using RBF or CPFP)
10:27 < jnewbery> ok, so the PR moves the (potentially memory expensive) tx inventory state to its own struct, and only initializes that if the connection relays txs
10:27 < aj> i don't think our bloom filters are easy to reinitialise; if you've got a unique_ptr like pfilter you can; but if you have it in place you define the number of elements and hence memory usage at declaration
10:27 < ariard> would be interesting to dig in :)
10:27 < jnewbery> One thing we need to be careful of when making changes to defaults like this is that it doesn't adversely affect memory usage
10:28 < aj> (also the idea is to eventually have 8 of these instead of just 2, so the cheaper the better)
10:28 < jnewbery> Suhas justifies being able to add these additional peers because it doesn't cause memory usage to increase too much for the average node
10:28 < nehan> aj: initializing the memory on first use != reinitializing. but this is quibbling.
10:29 < jnewbery> any other questions on bloom filters, data structures or memory?
10:29 < jnewbery> Third question: How do we signal a ‘blocks-only’ connection on the P2P network?
10:29 < michaelfolkson> <aj>: I thought the simulation discussion was exploring how many additional outbound connections you should have?
10:30 < jnewbery> hint: it's in the VERSION message
10:32 < jnewbery> psst: it's defined in BIP 37
10:32 < aj> nehan: when initialising the object it allocates the memory, with nElements = 50k; would have to conditionalise that or something to be able to save memory
10:32 < jnewbery> There's an optional `relay` boolean at the end of the VERSION message
10:33 < jnewbery> https://btcinformation.org/en/developer-reference#version
10:33 < jnewbery> if relay is set to False, the peer shouldn't relay txs to you
10:33 < jnewbery> next question: What does aj point out in his review comment that all other reviewers missed?
10:33 < nehan> jnewbery: are people ok with implicitly using a null object pointer as the indicator of whether or not the node supports tx relay?
10:34 < jnewbery> what do you mean about a null object pointer? That it's a 0x00 byte or something more than that?
10:35 < lightlike> jnewbery: that some peers might ignore our version message and still send us INVs, in which we case we would have sent TXs even if the connection is blocks-only
10:35 < nehan> jnewbery: nevermind, my question might not make sense i'll reask later if it does.
10:35 < aj> michaelfolkson: the simulation looks at how many outbounds resulted in the p2p network remaining connected if you assume all the other connections everyone has are useless (because the tx stuff has allowed them to be attacked, maybe)
10:36 < jnewbery> lightlike: exactly. There are peers on the network that ignore `relay`=False and will still send us tx INVs and TX messages
10:36 < ariard> spy nodes or buggy ones ?
10:36 < aj> nehan: (unique_ptr<foo> or optional<foo> are two ways to have a foo or not, but they have different memory usage when you don't have the foo)
10:37 < jnewbery> and the new code didn't expect that, so after we receive tx INVs from those peers, we'll send them tx GETDATAs
10:37 < lightlike> aj: did you spot this by testing in the wild, or by thinking about the PR?
10:37 < jnewbery> ariard: that's a good question. I spent some time looking into it, and I believe they're spy nodes
10:37 < jnewbery> because they're claiming to be v0.18 Bitcoin Core nodes, and I didn't find a bug in our VERSION relay handling
10:38 < aj> lightlike: i thought about it then though "no, suhas wouldn't get that wrong" so ignore it; then tried it live on mainnet and about the third run saw it
10:38 < jnewbery> nehan: sorry, I misunderstood your question. I thought you were talking about the `relay` bool in the VERSION message, not the new tx_relay struct
10:39 < michaelfolkson> <aj> So 2 was chosen due to the trade-off with resource utilization but 8 would still be plausible?
10:39 < jnewbery> moral of the story: even Suhas sometimes misses things :)
10:39 < jnewbery> great catch, aj
10:39 < jonatack> jnewbery: it might be interesting to hear, how did you look into the spy node question? code, watching node logs?
10:39 < michaelfolkson> <aj> The simulation was to see if it was worth bothering with 2 or whether to go higher at this point?
10:40 < nehan> jnewbery: yes, i'm curious why check node->m_tx_relay == nullptr (assuming the reader knows this is only non-null when the node supports tx relay) instead of directly including a separate bool to indicate whether or not the node supports tx relay. i guess it's a code style thing.
10:40 < aj> michaelfolkson: i took it as "seeing if 2 was already enough, or if in really bad scenarios more will be needed"
10:40 < jnewbery> michaelfolkson: 2 was chosen as 'this almost certainly won't cause any harm in the short run, and we can easily increase the number later'
10:40 < michaelfolkson> Ok thanks
10:41 < jnewbery> nehan: I think it's fine to check based on the non-nullness of the pointer. Adding an additional bool which essentially would be equivalent to saying 'this pointer is nun-null' seems unnecessary
10:42 < jnewbery> (and redundant)
10:43 < jnewbery> you could imagine a future change where someone adds a way to downgrade a connection to blocks-only, sets the pointer to null and forgets to update the bool, as one example of why you wouldn't want that redundancy
10:43 < jnewbery> but I agree, it's a style thing
10:44 < jnewbery> Next question: How should we handle peers that send us tx data after we’ve requested a blocks-only connection?
10:44 < lightlike> as to "increase the number later": would it be reasonable to completely separate block and tx relay peers in the future?
10:44 < jonatack> In general, these p2p changes are fascinating but ACKing them for me is held up by the difficulty of testing them.
10:44 < jkczyz> disconnect from them?
10:44 < jnewbery> lightlike: so all connections are blocks-only or tx-only?
10:44 < lightlike> yes     
10:45 < jnewbery> jonatack: (sorry, dropped your question about spy nodes earlier). I noticed that the IP address for the node that was doing this was on Greg's spy node list: https://people.xiph.org/~greg/banlist.cli.txt
10:46 < jnewbery> and I connected to several other nodes on that list, and they also showed this behaviour
10:46 < michaelfolkson> <jonatack>: What difficulties specifically with regards to testing? The attack scenarios?
10:47 < jnewbery> and I spent a while digging into how Bitcoin Core serializes/deserializes VERSION messages (and in particular booleans - `relay` in VERSION is the only boolean we serialize on the P2P network) and couldn't find any bugs
10:47 < jonatack> jnewbery: Thanks. I use Greg's banlist, and maybe should not use it when testing PRs then.
10:47 < ariard> michaelfolkson: thinking about all the ways code branches can be triggered due to a network event or combination of them
10:48 < jnewbery> jonatack: (re:ACKing) that shouldn't discourage you from reviewing. A comment like "code looks correct to me, but I don't feel confident enough about behaviour to give an ACK" is a perfectly helpful contribution
10:49 < jnewbery> lightlike: there is benefit to having both tx and block relay for the same peer, eg compact blocks works under the expectation that the mempool of the peers is similar
10:49 < jnewbery> jkczyz: yeah, I think disconnect from them is reasonable. At the moment we just log and carry on
10:50 < jnewbery> I opened a PR to do that here: https://github.com/bitcoin/bitcoin/pull/16682 (but marked as WIP, since it's less important to get merged than 15759)
10:50 < aj> lightlike: also the overhead of relaying blocks if you're already relaying tx's seems pretty small, and the more redundancy block relay has the better
10:51 < jnewbery> I skipped a question, but I think we already covered it: Why is the P2P behaviour of this PR difficult to test in the functional test framework?
10:51 < jnewbery> https://github.com/bitcoin/bitcoin/issues/14210 is the issue tracking that
10:52 < jnewbery> Any final questions in the last ten minutes?
10:52 < jonatack> michaelfolkson: the difficulty of setting up functional tests to simulate p2p behavior, we lack p2p simulation frameworks... Suhas, Gleb, Jeremy Rubin, Giulia Fanti, have IIRC all mentioned the need for them
10:52 < aj> why is it difficult? can't we setup a bunch of nodes, pre-populate their peers db so they know each other, and have them connect?
10:53 < jnewbery> aj: there are a few limitations:
10:53 < jkczyz> jnewbery: Is there any planned refactoring for the network code? It seems quite unwieldy/complex and perhaps increasingly so with each change.
10:53 < jnewbery> - we can't set up an outbound connection from a bitcoind node to the test framework and control the P2P message flow
10:53 < ariard> like dropping randomly some messages
10:54 < jnewbery> - all peers are considered 'local' because they're on the localhost IP, and they're treated slightly differently in the code
10:54 < jnewbery> I don't think we have any tests that pre-populate the peers db to get them to connect to each other
10:55 < michaelfolkson> Just set up a regtest network with a script?
10:55 < jnewbery> ariard: that'd be interesting testing, as would re-ordering or fuzzing P2P messages
10:56 < jnewbery> michaelfolkson: that's what the functional tests are doing, but the framework is missing some pieces that would allow us to fully test P2P behaviour
10:57 < jonatack> jnewbery: (re: ACKING) Thanks! Good to keep in mind.
10:57 < aj> jnewbery: shouldn't "addnode" be able to construct whatever shape of network we want? maybe adding/removing net_permissions -- so 127.0.0.1:* defaults to LOCAL_PEER perm but that can get dropped with some arg to addnode, maybe?
10:57 < michaelfolkson> Any links to hand on the missing pieces of the framework?
10:58 < jnewbery> let's wrap it up there. What did people think of this one? P2P is quite tricky and there's a lot of required context so I was worried that it might be too much, but I thought this was good.
10:58 < michaelfolkson> I find this stuff really interesting and easier to follow than last week's
10:58 < jnewbery> aj: addnode peers are treated differently from gossiped peers
10:58 < dergigi> That was interesting indeed.
10:58 < dergigi> Oh, last question: would Signet help in testing stuff like this?
10:59 < jonatack> Great choice... Suhas' p2p ones are always good.
10:59 < ariard> jnewbery: oh yes fuzzing P2P messages by just spawning a bunch of Cconnman and let's them talking between each others
10:59 < jkczyz> jnewbery: See my question for an answer to yours ^^^
10:59 < jkczyz> jnewbery: :)
10:59 < jonatack> ariard: Someone could really go to town with p2p simulations. We need to get some funding for this :)
11:00 < michaelfolkson> <dergigi>: As you are running tests on your local machine, I don't think Signet would offer anything on top of regtest
11:00 < jnewbery> aj: yes, arguments to addnode could be useful, or an updatepeer RPC like https://github.com/bitcoin/bitcoin/pull/10160
11:00 < dergigi> michaelfolkson: I see, thank you
11:01 < michaelfolkson> <dergigi>: Maybe if you are doing tests with peers trying to attack you :)
11:01 < ariard> aj: what's your process to test p2p PR, throwing a node on mainet/testnet with the patch and watching logs after a week ?
11:01 < jnewbery> dergigi: (re signet): perhaps yes for manual testing. Our autmated tests use regtest
11:02 < jnewbery> thanks everyone. Great discussion!
11:02 < jonatack> jnewbery: maybe put https://github.com/bitcoin/bitcoin/pull/10160 on the PR club list
11:02 < jnewbery> and thanks aj for dropping in!
11:02 < lightlike> thanks!
11:02 -!- jnewbery_ [~john@pool-108-21-57-150.nycmny.fios.verizon.net] has quit [Quit: leaving]     
11:02 < dergigi> thanks everyone, bye!
11:03 < michaelfolkson> Thanks
11:03 < ariard> thanks!
11:03 < aj> jnewbery: i guess, (a) i don't get why this is hard -- i mean, it's work, but it doesn't seem really tricky, but maybe i'm missing somethng? (b) not clear to me whether it makes more sense to setup the environment (peers.dat) and have bitcoind do it's normal resolution, or have rpc commands that explicitly set stuff up makes more sense, or maybe it makes sense to do both
11:04 < aj> ariard: read the code, read the PR comments, re-read both, run the tests, find something that doesn't make sense and try to make sense of it, repeat; once it kind-of makes sense, run it and see what happens. if things don't quite make sense, maybe add some logging, and run the tests and look through the logs, or run it on mainnet and look through the logs
11:06 < jonatack> aj: thanks, i was curious to hear that too. I recall you were adding custom logging to test your PR to fix the NOTFOUND bug.
11:06 < aj> ariard: sometimes i try refactoring the code to something that i think will be prettier, and discover why that's actually a huge ugly mess
11:06 < aj> printf debugging is the best and it's always a privilige to add it to other people's code
11:08 < jonatack> aj: what kind of time do you spend to review/test a PR in this way?
11:08 < ariard> aj: thanks a lot, specifically the p2p stack I found hard to know if everything makes senses
11:09 < aj> ariard: i'm not sure anyone on earth is smart enough to look at the p2p stack and honestly think everything makes sense?
11:10 < ariard> ahah true
11:11 < aj> jonatack: not really sure, twice as long as i plan to? :)
11:11 -!- marcinja_ [~marcinja@204.48.26.93] has left #bitcoin-core-pr-reviews []     
11:12 < jonatack> aj: yeah, thanks, needed reassurance here :)
11:28 -!- dergigi [~dergigi@37.48.119.130] has quit [Remote host closed the connection]     
11:38 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving]     
11:47 -!- hanhua [68840050@104.132.0.80] has quit [Remote host closed the connection]     
12:11 -!- michaelfolkson [~textual@146.185.56.214] has quit [Quit: Sleep mode]     
12:11 -!- coisinho [59f67a44@i59F67A44.versanet.de] has quit [Remote host closed the connection]     
14:05 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has quit [Quit: pinheadmz]     
14:43 -!- afigs [642bef32@100-43-239-50.static-ip.telepacific.net] has quit [Remote host closed the connection]     
14:49 -!- probably_dillon [4516f113@user-12hds8j.cable.mindspring.com] has quit [Remote host closed the connection]     
15:28 -!- ccdle12 [~ccdle12@cpc139350-aztw33-2-0-cust310.18-1.cable.virginm.net] has quit [Remote host closed the connection]     
16:15 -!- hebasto [~hebasto@95.164.65.194] has quit [Remote host closed the connection]     
16:15 -!- Zenton [~user@unaffiliated/vicenteh] has quit [Ping timeout: 246 seconds]     
17:56 -!- lightlike [~lightlike@2001:16b8:573b:900:607b:a6e7:8a81:763b] has quit [Quit: Leaving]     
19:05 -!- emilengler_ [~emilengle@unaffiliated/emilengler] has joined #bitcoin-core-pr-reviews     
19:08 -!- emilengler [~emilengle@unaffiliated/emilengler] has quit [Ping timeout: 258 seconds]     
19:24 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ynxrmrbcdnwkrwpt] has quit [Quit: Connection closed for inactivity]     
21:32 -!- pinheadmz [~matthewzi@198.8.81.202] has joined #bitcoin-core-pr-reviews     
22:17 -!- pinheadmz_ [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
22:17 -!- pinheadmz [~matthewzi@198.8.81.202] has quit [Ping timeout: 245 seconds]     
22:17 -!- pinheadmz_ is now known as pinheadmz     
23:32 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has quit [Quit: pinheadmz]     
23:54 -!- ccdle12 [~ccdle12@cpc139350-aztw33-2-0-cust310.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews