< jnewbery>
fanquake: I've reviewed. Looks good to me.
< jonasschnelli>
can someone unban me in #bitcoin #bitcoin-dev?
< jnewbery>
wumpus, fanquake: I think it might be time to merge #19070. It has 5 ACKs/concept ACKs
< gribble>
https://github.com/bitcoin/bitcoin/issues/19070 | p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS by jnewbery · Pull Request #19070 · bitcoin/bitcoin · GitHub
< jnewbery>
It's opt-in and very self-contained, so if there are any problems with it, then it'll be very easy to revert
< bitcoin-git>
[bitcoin] naumenkogs opened pull request #19697: Minor improvements on ADDR caching (master...2020-08-addr-cache-follow-up) https://github.com/bitcoin/bitcoin/pull/19697
< jnewbery>
only topic so far is priorities/focus. Please come prepared with one or two sentences about what your current priority/focus is to share with the group. Thanks!
< * fanquake>
wonders if he should attend to achieve back to back to back bitcoin meetings 🤔
< aj>
i'm mostly caring about taproot-critical-path-things, which i think is now mostly not p2p stuff
< aj>
but copied stuff off my whiteboard in case it's missing anything interesting or important
< adiabat>
hi
< jnewbery>
sdaftuar: any priorities?
< sdaftuar>
i've got a bunch of things i am thinking about...
< sdaftuar>
i'd say my current priorities are to get the transaction download stuff (sipa's 19184 i think) reviewed. and erlay is on my mind right after that
< sdaftuar>
but i'm also thinking about a bunch of other things that i want to mention, because if others are interested in any then maybe we can make progress on other fronts as well
< jnewbery>
do you want to list them now?
< sdaftuar>
some stuff is related to network-topology improvements:
< sdaftuar>
more block-relay only peers (which is probably gated on negotiating block-relay connections at connect-time)
< sdaftuar>
more improvements to syncing our tips with more peers (possibly including tx-relay-peer rotation, which can help here as well)
< sdaftuar>
improved eviction logic (pr open)
< sdaftuar>
and other stuff is related to transaction relay policy, particularly package relay, which is a whole beast of a topic by itself
< sdaftuar>
but also rbf pinning (which may be a related problem)
< dongcarl>
jonatack: do you have link to bip324 discussion?
< jonatack>
so will stick with review and p2p refactoring on the side until that's done: we need a universal explicit feerate rpc
< jnewbery>
ok, thanks jonatack
< jonatack>
dongcarl: yes, will post, that's it for now
< jnewbery>
troygiorshev: priorities?
< troygiorshev>
two p2p things I've been focusing on
< troygiorshev>
big refactor: #19107, moving header verification from net_processing to net. came out of a PR review club of a jonasschnelli pr. it's not flashy, but cleaning up this interface will make everything easier going forward.
< gribble>
https://github.com/bitcoin/bitcoin/issues/19107 | p2p: Move all header verification into the network layer, extend logging by troygiorshev · Pull Request #19107 · bitcoin/bitcoin · GitHub
< troygiorshev>
feature: #19031 addrv2. tor v2 deprecation and obsolescence is quickly approaching, addrv2 is needed before we can update to tor v3.
< ajonas>
I can wait until we move onto the next topic
< jnewbery>
ok
< jnewbery>
amiti: priorities?
< amiti>
My main focus has been 19316- simplifying how we track different types of connections. Got some reviews yesterday & hopefully its getting close to merge, so planning to address outstanding review comments in a follow up.
< amiti>
(ps @dongcarl, @jnewbery if you wanna take another look :))
< amiti>
After that I’m excited about #19315 to enable more p2p testing. And I want to make my way back to the rebroadcast work!
< amiti>
In terms of review, there’s a lot of PRs I’m excited about and slowly making my way through. Currently reviewing #19670. Also on my list are #17428 (anchors), #19184 (tx logic overhaul). and the per-peer stuff #19509 & #19607 is also interesting
< amiti>
oh I didn't see that yet. awesome thanks!!
< jnewbery>
thanks amiti
< jnewbery>
fanquake: priorities?
< fanquake>
Nothing I am/have been working on is really p2p related. Can probably skip me.
< jnewbery>
ok
< jnewbery>
pinheadmz: priorities?
< pinheadmz>
sorry not much to contribute today
< jnewbery>
no problem
< jnewbery>
ariard: priorities?
< ariard>
yes so AltNet (#18988) is pending on Russ multiprocess
< gribble>
https://github.com/bitcoin/bitcoin/issues/18988 | RFC: Introducing AltNet, a pluggable framework for alternative transports by ariard · Pull Request #18988 · bitcoin/bitcoin · GitHub
< ariard>
it would make it far easier to leverage the new multiprocess framework introduced
< ariard>
also spend a bit of time evaluating which package relay/RBF pinning flavor would solve pinning
< ariard>
I'm also interested in tx-relay peer rotation, to improve transaction propagation wrt to pinning/mempool obstruction
< ariard>
I would be glad to get #19645, even if utility is reduced until further mempool/transaction relay policy changes, that's a first step to solve wtxid-pinning issues
< ariard>
and I'm staying available to review transaction request overhaul/erlay/others
< ariard>
that's it
< jnewbery>
thanks ariard!
< jnewbery>
theStack: priorities?
< jnewbery>
elichai2: priorites?
< jnewbery>
sipa: priorities?
< jnewbery>
(if you missed your turn you can jump in again later)
< sipa>
so
< sipa>
the next thing on my list is addressing some feedback in 19184 (tx overhaul), and rebasing on top of wtxid relay
< sipa>
i'm also interested in helping with bip324 efforts and addrv2, though i haven't found much time for that
< sipa>
outbound peer rotation also sounds interesting; i wasn't aware there was recent interest in that
< aj>
jnewbery: (priorities?)
< elichai2>
I don't work on anything p2p related right now, but I do plan to review a bunch of stuff, so if there'll be a high priority for review out of this it would be great
< jnewbery>
we haven't done adiabat and vasild yet, but I can go next
< jnewbery>
My short-term focus is on the backports. I've reviewed #19680 and 19681. I've also backported wtxid relay in #19606, which I still think we should backport to v0.20.
< jnewbery>
I also have a branch that backports the orphan relay stuff on top of that, which I think makes sense to PR separately, but I can add to v0.20 if that makes it easier for reviewers.
< jnewbery>
I'd advocate for people to bump reviewing backports up their priority list (both for these backports and in general inBitcoin Core)
< jnewbery>
Longer-term, I want to make progress on #19398, where the main goal is to clarify the interface between net and net_processing, while not expanding the scope of cs_main (and then eventually reduce the scope of cs_main by moving data into the new Peer struct).
< jnewbery>
Thanks everyone! I hope that topic wasn't too slow. I just wanted to make sure everyone had a chance to share what they're working on/prioritizing.
< jnewbery>
we had one other topic suggestion from ajonas
< vasild>
so, I address review suggestions as quickly as possible and rebase it to resolve conflicts. But mostly it is in the hands of reviewers. While waiting on that I am reviewing some randomly picked PRs.
< ajonas>
Ok. A few months ago, I was reading over https://diyhpl.us/wiki/transcripts/bitcoin-core-dev-tech/2018-03-07-priorities/, which articulates some possibilities for how to better coordinate review. Since then, I've been experimenting with asking for reviews directly. (This was also the motivation of #18949).
< jnewbery>
vasild: importance is subjective. Being on that list doesn't necessarily mean that other people think it's important, but everyone is allowed to add one PR to the list, so you can see what each author is prioritizing.
< ajonas>
To date, I've cherry picked the PRs that I think I have a chance to help out with so while the numbers look good, there are some notable exceptions where I couldn't move the needle.
< ajonas>
And on some of those I just got lucky I think,
< jonatack>
ajonas: some of those are merged
< jonatack>
e.g. 16756
< ajonas>
Right. Column J shows the time from PR open to merge
< troygiorshev>
ajonas: how did you define "review" in "review to merged (days)". first ACK, first ACK on the current rebase, when you felt that there was enough review that it was RFM?
< ajonas>
Sorry that's column I
< jonatack>
ajonas: oic
< ajonas>
yeah, that's misleading troygiorshev. I mean first nag to merge.
< vasild>
Yes, importance is subjective. However, it would be convenient to have one place to look for important PRs, even if that place contains different people's lists.
< troygiorshev>
ajonas: ah ok
< ajonas>
Anyways, if anyone interested in p2p reviewing and would like to opt-in, I'd be interested in expanding my experiment that part of the code.
< jnewbery>
I think there are so many other factors, that I
< sdaftuar>
ajonas: you mean opt-in to being nagged by you, right?
< jnewbery>
'm not sure the numbers have much meaning
< aj>
i had been maintaining https://github.com/users/ajtowns/projects/1 to track p2p/mempool PRs by category things, but hadn't automated it and slacked off this year, it's on my todo to try automating again
< ajonas>
sdaftuar: yes
< jnewbery>
aj: I found that project board useful to find p2p PRs
< ajonas>
jnewbery: fair enough. I'm just trying to track the work I've done. Not trying to claim credit for the merges. Just trying to help coordinate.
< jonatack>
vasild: i think it's fine to define one's own list of important PRs to review. e.g. longer term for me would be: BIP155/addrv2, BIP324, BIPs340-342, BIP325
< troygiorshev>
vasild: I plan on using this meeting log as a "one place to look" :)
< aj>
jnewbery: maybe ajonas should opt me in to nags about it then
< jnewbery>
ajonas: I understand! I'm just saying that you might not find much signal in the quantative data there
< vasild>
ack
< ajonas>
That's all I had.
< jnewbery>
ajonas: are you asking people to opt-in now or should they message you?
< ajonas>
Either works.
< jnewbery>
ok, thanks
< sipa>
i am open to nagging
< jnewbery>
no more proposed topics. Was there anything else anyone wanted to discuss? sdaftuar: it sounded like you might have wanted to go into a bit more detail on some of your priorities?
< sdaftuar>
topic suggestion: feature negotiation (new bip proposal from me)
< fanquake>
One related comment I'd make is that the "ACK recap" comments can sometimes be misleading. I think there can also be confusion as to why a PR which looks like it has *lots* of ACKs, maybe after a review-club bomb, hasn't been merged.
< ajonas>
sipa: great!
< troygiorshev>
fanquake: ack recap?
< jnewbery>
#topic feature negotiation
< aj>
troygiorshev: ajonas sometimes posts a PR comment summarising previous acks
< sdaftuar>
i was planning to send this to the mailing list today or tomorrow, so figured this would be a good place to mention it
< amiti>
fanquake: can you tell me more? I find those ACK recap comments helpful when there's lots of convo on the PR. is there something that can be done to make them more useful?
< troygiorshev>
aj: thanks
< sdaftuar>
basically wtxid-relay uses a new feature-negotiation method (exchanging messages between version and verack), that would be nice to codify as a method in the future
< jonatack>
amiti: +1 i find them useful as well
< sdaftuar>
however, i think we need to make sure software on the network knows to ignore unknown messages pre-verack to make this a possibility. bitcoin core historically has disallowed unknown messages pre-verack
< sdaftuar>
so i think it would be nice to get this out there and hopefully make this a standard way we can do things going forward
< sdaftuar>
(end)
< jnewbery>
is the idea that each feature has its own p2p message for enabling the feature (like wtxidrelay)?
< sdaftuar>
features that need to negotiate at connection startup time.
< ariard>
sdaftuar: I think that's good it was unclear between matt and I on bip339 implemn in rust-bitcoin about why 339 bumps both protocol version and wtxid-relay
< troygiorshev>
(like addrv2)
< sdaftuar>
so many features don't need that, which is fine
< fanquake>
amiti: it depends on the PR. Concept ACKs from months ago, after the code has changed significantly are not always relevant. A large amount of ACKs from new/unknown contributors obviously don't hold as much weight as from contributors with more experience in that part of the code.
< sdaftuar>
but the next time we want to negotiate something that is in place before a connection is fully setup, i think this is the best way to do it
< sdaftuar>
in particular i'd like to leverage this method for negotiating block-relay only connections
< ajonas>
fanquake: I have made an effort to stay away from review club PRs
< fanquake>
It's also sometimes unclear what reviewers actually mean when they ACK. i.e if they've just run the functional tests after glancing at the diff on GH, that isn't necessarily meaningful.
< jnewbery>
fanquake: can we discuss this next?
< aj>
sdaftuar: i thought wtxid message made sense, so making it easily reusable makes sense
< amiti>
sdaftuar: I'm a +1 to an explicit message/negotiation for block-relay only connections
< fanquake>
jnewbery: sure. don't want to hijack.
< troygiorshev>
sdaftuar: concept ACK
< jonatack>
troygiorshev: i think vasild's addrv2 implementation can set itself anytime during the connection, not only at handshake
< sdaftuar>
one point that occurred to me, is that might update the draft i pasted above to NOT further bump the version number to signal support
< ariard>
if I understand well this bip is disentangling protocol version bump from feature negotiation by allowing feature signaling between version/verack
< sdaftuar>
because software that chooses to not implement wtxid-relay should already be adopting this proposed bip, basically (if they bump their version number to 70016 or higher at any point)
< sdaftuar>
that's a bit in the weeds though for there
< sdaftuar>
here*
< jnewbery>
I've always thought that extending the version message to include supported and required features would be a good way to negotiate features
< sdaftuar>
ariard: yes. more importantly than that is that we're establishing that unknown messages should not cause peer disconnections, which could be problematic for network topology if we get this wrong in the future
< sdaftuar>
even if those unknown messages are before VERACK
< troygiorshev>
jonatack: right. I think I remember discussion as to whether that was the right choice though
< sdaftuar>
wtxid-relay BIP implied that, i'm just now making that more explicit. this would require a change to bitocin core as well
< sdaftuar>
vasild: that's basically how we've done every p2p protocol upgrade in the last 4-5 years i think?
< sipa>
i find the reliance on protocol versions to enable feature-negotiability a bit ugly still - less so than the earlier feature negotiation through version numbers directly, but still
< sdaftuar>
well either that or a service bit i guess
< sdaftuar>
but service bits are rare
< troygiorshev>
vasild: your point is the motivation for jnewbery's "extended version" iirc
< jnewbery>
sdaftuar: do you know whether there are nodes that disconnect on unknown message types? Bitcoin Core just drops them (which I think is the only sensible behaviour)
< sdaftuar>
jnewbery: it's been the de facto standard (not documneted anywhere to my knowledge) that unknonw messages after a connection is setup are ignored by software
< vasild>
jonatack: troygiorshev: yes, the BIP155 sendaddrv2 can come any time, but we try to do it early so that when the peer is about to advertise his own address to us, he has the option to send us addrv2 - would be important if his address is torv3 because he wouldn't be able to advertise it to us with addr(v1)
< sdaftuar>
it has not been the standard, to my knowledge, to allow unknown messages pre-verack
< jnewbery>
ah ok. That's the difference
< jnewbery>
ok, any final topics?
< jnewbery>
that's a wrap then. Thanks everyone
< ariard>
sdaftuar: what do you mean by broad agreement? we need this BIP to be widely deployed before using the new feature signaling mechanism ?
< jnewbery>
Let me know if you liked the format of this meeting or if you want to change it up for next time.
< aj>
sdaftuar: my thinking on big changes priority-wise is: tx relay overhaul up next, then erlay if we can get to it; but package relay is back to the drawing board. happy to think about ...
< troygiorshev>
jnewbery: this was great, thanks for organizing it!
< sdaftuar>
aj: yeah that's where i'm at as well on that side of things
< aj>
sdaftuar: ... some of the topology type stuff, in before erlay i guess?
< sdaftuar>
aj: i'm trying to figure out how to squeeze in topology work too
< adiabat>
maybe tangential but... any thoughts on port flexibilty
< adiabat>
I've gotten servers shut down, and all they do is see that 8333 is open
< sdaftuar>
ariard: well, if any software authors brought up concerns about why using unknown messages between version and verack is a problem, then we should factor that in--
< sdaftuar>
if some software clients choose to not follow my proposal, that would create implications for us trying to use it down the road
< sdaftuar>
as it could partition the network
< sdaftuar>
i am not aware of any opposition; i brought this issue up with wtxid-relay and no one commented on it
< jnewbery>
#endmeeting
< lightningbot>
Meeting ended Tue Aug 11 16:00:20 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< jonatack>
fanquake: did you want to expand on the acking question
< aj>
adiabat: i think the reason not to do other ports was to prevent bitcoin nodes being used as a primitive botnet that'll start connecting to arbitrary services (or to be mistaken for being part of a botnet if someone spams military.gov's rsh port as a thing to connect to for bitcoin p2p maybe. never been able to judge if that makes much sense these days
< instagibbs>
adiabat, really
< adiabat>
I guess it's tricky; with 8333 it sticks out a lot and is really easy to identify for any one on the network
< adiabat>
maybe some kind of udp-port knocking or something? just an idea
< fanquake>
jonatack: I might write up some thoughts and post them here tomorrow. The gist is that 1 ACK is not always equal to another (obviously). It's not always clear what people mean when they say they've reviewed something. Recapping ACKs from some previous head or a concept ACK from a previous implementation isn't always useful. You can just as easily open a buggy PR, as you can introduce a bug in the last rebase.
< ajonas>
Some more feedback would be helpful
< troygiorshev>
>it's not always clear ...
< troygiorshev>
that's the issue, no?
< fanquake>
It's not uncommon for a PR to have a few ACKs, and then an experienced contributor to turn up and start pointing out bugs. There's also no rush to merge things.
< ajonas>
Understood. You don't have to do it now, but maybe we can talk about some of the ones that really stood out as unhelpful.
< fanquake>
troygiorshev: sure; and comments like "I ran the functional tests" generally aren't useful unless you did it one some exotic machine/setup etc. In which case, you should point it out!
< fanquake>
Obviously not referring to personally here.
< troygiorshev>
didn't take it personally :)
< troygiorshev>
certainly is difficult (i imagine) to distinguish between "this is a really clean PR and I really have no comments on it, it's RFM" and "I've done my best but I'm not familiar enough with bitcoin to really review this"
< jonatack>
fanquake: agreed. contributing.md clearly states that all review is not equal. providing ack methodology is also pretty much inversely valuable to review experience, e.g. newer reviewers providing details on what they did and thought about and checked is probably more useful than if i more or less repeat the same process info on each review ack
< aj>
troygiorshev: always seems best to qualify your ack if it's the latter
< sipa>
yes, you are in no way restricted to just the word "ack" when leaving a review :)
< sipa>
a one-line summary of what you did, or how qualified you feel about it, is very useful
< amiti>
fanquake: ofc ACK review comments can't replace the job of the maintainer to make those more subtle judgments, but the reason I find them helpful is to maintain momentum on PRs with lots of comments. and give me (and other reviewers) a quick overview as a starting point. whether I already reviewed the PR or am just starting a review
< amiti>
sipa: earlier you referred to a mention of outbound eviction, what was that in regards to?
< sipa>
08:07:11 < sdaftuar> more improvements to syncing our tips with more peers (possibly including tx-relay-peer rotation, which can help here as well)
< sipa>
and apparently i misread; jonatack was talking about inbound eviction policy
< jonatack>
which made me realise that i didn't know what was going on with my peer conns in enough detail
< jonatack>
i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard
< jonatack>
sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently?
< sipa>
jonatack: nope; i suspect just nobody ever added it
< phantomcircuit>
adiabat, it has to be easy to identify a bitcoin node that's listening, and it's inherently going to be easy to identify a bitcoin node based on the network traffic
< jonatack>
e.g. timestamp of last block and last txn for that peer
< phantomcircuit>
without significantly delaying block relay, it's impossible to hide that you're running a bitcoin node from the network operator
< adiabat>
phantomcircuit: right I guess this would only make sense in the context of bip 324 or similar
< jonatack>
sipa: thanks. will propose.
< phantomcircuit>
adiabat, no even then you can trivially detect that it's a bitcoin node
< nehan>
quick comment on the review nagging stuff from the p2p meeting: i think there is an implicit assumption in there that faster merges are better (by measuring time to merge and suggesting to optimize on it). i don't agree with that and want to point out it might not be great to encourage faster merges. i'd argue a better metric is quantity of high-quality review.
< sdaftuar>
nehan: i think that's a fair point, but for a given amount of review from a given set of reviewers, the quality of that review is likely higher when not excessively drawn out over time. i think this is because people are more thoughtful in responding to other good review comments if their own thinking on how a PR works is fresh in their minds, and because errors can crop up due to rebases as the
< sdaftuar>
underlying code base changes while a PR is open
< sdaftuar>
so maybe it's not reasonable to expect merges to happen in very short periods of time and have that be a good thing... but 6-12 month review cycles strikes me as usually not very good outcomes for a PR
< ajonas>
nehan: Point well taken. To build on what sdaftuar mentioned, I've seen evidence that some are discouraged by month+ gaps in a review cycle or don't feel comfortable asking for review themselves. Each PR is different -- different complexity, different set of reviewers, etc. I don't think any apples to apples comparison on an individual PR basis makes sense.
< ajonas>
In aggregate, however, I think there is the possibility of seeing some signal in those numbers by coordinating reviews or a designated entity to do the nagging. The goal is to allow authors to focus more on their contributions rather than rebasing. Time to merge is a first bad metric that I thought might be a decent proxy, but I'd certainly be open to other ways to measure progress.