19:00:22 <wumpus> #startmeeting
19:00:23 <lightningbot> Meeting started Thu Jun  4 19:00:22 2020 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:23 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:25 <jnewbery> hi
19:00:26 <kanzure> hi
19:00:29 <MarcoFalke> hoy
19:00:29 <sipa> hi
19:00:43 <troygiorshev> hi
19:00:46 <wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball kvaciral ariard digi_james amiti fjahr
19:00:48 <ariard> hi
19:00:49 <wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2
19:00:52 <fjahr> hi
19:00:53 <elichai2> Hi
19:00:54 <jonasschnelli> hi
19:00:57 <achow101> hi
19:01:11 <luke-jr> hi
19:01:29 <jamesob> hi
19:01:34 <real_or_random> hi
19:01:39 <wumpus> hi
19:02:00 <wumpus> #topic High priority for review
19:02:17 <wumpus> https://github.com/bitcoin/bitcoin/projects/8  currently 12 blockers, 3 chasing concept ACK
19:02:22 <jonatack> hi
19:02:39 <wumpus> this is a record high number :)
19:03:03 <luke-jr> please add #18818
19:03:06 <gribble> https://github.com/bitcoin/bitcoin/issues/18818 | Fix release tarball generated by gitian by luke-jr · Pull Request #18818 · bitcoin/bitcoin · GitHub
19:04:15 <amiti> hi
19:04:18 <ariard> if we can add #18797 too plz ? it's pretty simple to review and added a lengthy motivation
19:04:20 <gribble> https://github.com/bitcoin/bitcoin/issues/18797 | Export standard Script flags in bitcoinconsensus by ariard · Pull Request #18797 · bitcoin/bitcoin · GitHub
19:04:26 <wumpus> luke-jr: added
19:04:30 <luke-jr> thx
19:04:39 <wumpus> I think we need more review before we add even more PRs but okay
19:04:44 <luke-jr> ariard: it's conceptually non-simple
19:05:10 <MarcoFalke> I looked at some of the high prio ones, but no one looked at mine :(
19:05:12 <wumpus> at some point if we keep adding high priority PRs then it's no more of a help than github's PR list
19:05:17 <ariard> luke-jr: I added a motivation since last discussion, I invite you to express your reply on the PR
19:05:46 <wumpus> anyhow, added
19:05:56 <jamesob> MarcoFalke: will try to take a look this weekend
19:05:57 <jonatack> wumpus: I agree... too many blockers means no blockers
19:06:05 <sipsorcery> hi
19:06:25 <wumpus> I mean everyone is entitled to having one high prio PR and there's a lot of contributors so it's not entirely unexpected
19:06:32 <MarcoFalke> jamesob: thx
19:06:38 <wumpus> I kind of hope reviewing will go back to pre-corona activity at some point
19:08:50 <ariard> it sounds also all hi prio prs affect different parts of the codebase, which may be not a bad signal to have a lot of them?
19:09:01 <wumpus> ok, let's go over the rest of the proposed topics, there's a lot for today
19:09:04 <luke-jr> hmm
19:09:49 <wumpus> #topic Peer logging (troygiorshev)
19:10:10 <troygiorshev> Hey everyone
19:10:11 <troygiorshev> I have a proposal for extending the per-peer message capture and resource tracking capabilities.
19:10:13 <jeremyrubin> MarcoFalke: fwiw I'm more likely to look at something when explicitly asked to review it
19:10:38 <troygiorshev> It's here
19:10:39 <troygiorshev> https://gist.github.com/troygiorshev/f2fedca9b2ab8a2c6449c92d92e17678
19:10:49 <troygiorshev> If anyone's given it a look I'm happy to answer any questions
19:12:08 <wumpus> is there a PR or issue for this?
19:12:16 <jeremyrubin> troygiorshev: I like this kind of stuff. I'm curious if you have intuition on how much overhead it is to track
19:12:31 <wumpus> if not, I don't think yo ucan expect anyone to have had a look at this
19:12:43 <elichai2> troygiorshev: the general idea sounds interesting, especially if there will be a defined structure to it, so that external tools can analyze/aggregate the logs
19:12:50 <jamesob> Concept ACK, would love more comprehensive per-peer statistics for the for-now-fictional monitoring framework I've been noodling on
19:12:53 <sipa> concept ack, but it's very high level
19:12:54 <troygiorshev> wumpus: not yet, thanks for the suggestion
19:13:12 <sipa> things will depend on how invasive it is, what performance impact it has, ...
19:13:28 <sipa> but in general, gather more of these statistics is definitely useful
19:13:32 <jamesob> sipa: presumably it would be opt-in, but your questions are still relevant
19:13:42 <wumpus> also how complex the added code is and how much extra maintence work it is
19:14:09 <troygiorshev> jeremyrubin: I don't expect it will be run all of the time, but that will be something I keep a close eye on.  With it being toggleable, when people are using it they probably won't care about the performace impact (as I don't expect it will be massive my any means)
19:14:16 <sipa> right, but even if it's opt-in, if it's so invasive that it alters the data it's measuring... it may be a problem :)
19:14:31 <jamesob> true!
19:14:50 <troygiorshev> elichai2: absolutely.  I'll also be building a bit of an analysis tool
19:15:04 <jamesob> maybe there's a validation-queue-style decoupling you could do (or resurrect the logging thread PR and extend that)
19:15:11 <jnewbery> jamesob: for stats tracking it might not be opt-in. Eventually we might want to use those stats to do peer throttling/eviction
19:15:12 <wumpus> it reminds me of the statoshi changes (which IIRC never got integrated, nor even proposed, beause they're too invasive)
19:15:34 <sipa> jnewbery: indeed
19:15:39 <phantomcircuit> hi
19:15:47 <jamesob> jnewbery: makes sense, but could start opt-in and make various measurements mandatory on a case-by-case basis
19:15:54 <sipa> of course
19:15:59 <jonatack> troygiorshev: i read it. i think it addresses a real issue but needs much more detail and also describe much more any previous work on this
19:16:19 <luke-jr> one thing to keep in mind is we don't want detailed logging by default
19:16:20 <jamesob> wumpus: and they introduce a dependency on a statsd library iirc
19:16:22 <jnewbery> wumpus: statoshi is global statistics for the node. This is more targetted towards troubleshooting/investigating interactions with individual peers
19:16:29 <wumpus> in which case you might want to talk with Jameson Lopp
19:16:33 <luke-jr> last thing we need is people getting subpoena'd to find out which peer relayed what
19:16:48 <wumpus> jnewbery: which sounds even more involved
19:17:01 <sipa> luke-jr: logging to an external file should definitely remain opt-in, of course
19:17:02 <wumpus> luke-jr: that's also a serious risk
19:17:20 <wumpus> detailed logging should *definitely* not be enabled by default
19:17:47 <jamesob> is there a near costless way of "teeing" each P2P message received to some separate analysis thread?
19:17:52 <troygiorshev> jamesob, sipa: I hope that, in its initial form, it will have no impact whatsover when disabled, and that it won't modify messages in any way when enabled.  imo it wouldn't be an effetive monitoring tool if it changed things
19:18:04 <troygiorshev> (We're not quanum physics here :))
19:18:09 <sipa> sure
19:18:27 <jamesob> troygiorshev: I think the concern is not message modification but introducing some delay that materially affects p2p behavior as a second-order thing
19:18:33 <wumpus> jamesob: tcpdump? :)
19:18:49 <jeremyrubin> TBH I think the best thing is tcpdump-like
19:18:50 <jamesob> wumpus: sounds good to me! let's not do this in core if that's practical!
19:19:11 <sipa> i think there are two separate goals, and they may need different solutions
19:19:13 <jeremyrubin> You might want to do it in core for encryption reasons
19:19:16 <wumpus> wireshark has a bitcoin P2P protocol dissector, for example
19:19:23 <sipa> dumping logs may be doable with tcpdump or other external tools
19:19:27 <jnewbery> I don't think there's a huge amount to discuss yet. I just suggested that troy raise this in a meeting so people are aware of the project and know where to leave feedback/suggestions
19:19:39 <sipa> but tracking of statistics that we may want to one day use to influence p2p behavior can't be done that way
19:19:44 <wumpus> if your goal is really to capture all P2P data, that approach sounds better than adding yet another layer in bitcoind
19:19:57 <MarcoFalke> I think it is hard to give feedback when the exact goal is a bit vague right now. Is this for internal node stats? Is this for logging plaintext to a file? Is this for exporting stats to a serialized file?
19:19:59 <wumpus> yes it's useless for statistics
19:20:09 <wumpus> agree there's mixed goals here
19:20:16 <jamesob> an OOB process could then feed back into core via an RPC interface, if desired
19:21:01 <wumpus> we already have some light statistics, like a P2P message type histogram in core, doesn't hurt to add more oft hose things esp. if they're opt-in and don't add to memory use of the CNode struct too much
19:21:09 <phantomcircuit> wireguard even has a dissector for this
19:21:11 <sipa> yeah
19:21:22 <wumpus> but if you want full traffic intercept, that kind of thing doesn't belong in bitcoind imo
19:21:40 <wumpus> phantomcircuit: yup
19:22:32 <jamesob> troygiorshev: anyway, looking forward to some details!
19:22:33 <jnewbery> wumpus: tcpdump isn't aware of application details. It'd be impossible to log messages from peers with subversion="thing" for example
19:22:51 <jnewbery> unless you dumped all traffic all the time and then filtered after the fact
19:22:53 <troygiorshev> lots of good points, thanks.  Certainly I agree, it's not for statistics.  The immediate benefit would be for debugging, one of the long term benefits could be intelligent peer-selection
19:22:56 <sipa> yeah, let's wait for some details
19:22:57 <jb55> troygiorshev: I have been adding usdt bpf traces to my node for exactly this use case. it adds low-overhead nop instructions that linux can plug into at runtime. and with bpftrace you can write scripts to gather node stats
19:23:03 <luke-jr> not impossible w wireshark
19:23:03 <phantomcircuit> jnewbery, the wireguard dissector actually means you can do exactly that
19:23:15 <luke-jr> though maybe not implemented
19:23:35 <wumpus> jnewbery: dunno about tcpdump but wireshark can do some quite complex context/application-specific filters
19:23:39 <jnewbery> yes, I understand you can filter afterwards. The point is that you end up with enormous pcap files because you have to capture all traffic
19:23:53 <wumpus> yes, but also it is a pretty rare case for most people
19:23:56 <phantomcircuit> luke-jr, it's literally implemented bitcoin.version.user_agent
19:24:02 <wumpus> some things should really be external tooling
19:24:20 <luke-jr> phantomcircuit: that just matches the version packet tho?
19:24:41 <phantomcircuit> luke-jr, you can select tcp streams based on it though
19:24:47 <wumpus> yupp
19:24:48 <phantomcircuit> i cant remember how but i know you can
19:24:50 <sipa> fwiw, i'm more interested in gathering statistics (possibly some day gathered by default) than just protocol dumps
19:25:03 <jonasschnelli> would ne no longer fun for post BIP324 connections
19:25:16 <jonasschnelli> (outside core)
19:25:17 <sipa> jonasschnelli: also a good point
19:25:29 <jamesob> would be nice to make it easy for people to voluntarily give a detailed dump of p2p stats
19:25:34 <luke-jr> cool
19:25:40 <troygiorshev> i'm personally a huge fan of BIP324 and AltNet and related, so I'
19:25:53 <troygiorshev> I'm keeping those in the back of my mind as I think about htis
19:25:57 <wumpus> so: statistics yes, protocol dump controversial
19:26:16 <jnewbery> right, it's impossible to do any kind of dynamic packet filtering outside the application if it's encrypted
19:26:18 <wumpus> let's regard thes as separate topics
19:26:35 <luke-jr> jnewbery: NOT IF YOU PROVIDE KEYS
19:26:42 <luke-jr> oops sorry for caps
19:27:05 <sipa> shower thought: are pcap files easy? if so, maybe it's useful post-BIP324 to permit dumping the decrypted stream in pcap format
19:27:06 <jonasschnelli> luke-jr: would probably be complicated to impossible in wireshark
19:27:20 <wumpus> but I'd suggest first writing some document detailing more what you want to do, or make a example implementation
19:27:30 <luke-jr> jonasschnelli: it does for https already
19:27:31 <MarcoFalke> For logging, one could disable bip324
19:27:32 <sipa> indeed, seems there are many partially overlapping ideas here
19:27:33 <wumpus> as said this is quite vague and abstract right now
19:27:52 <jnewbery> sipa: I don't think you'd want pcap. By the time you've got to the application, you've lost all the lower protocol layers
19:28:11 <sipa> perhaps
19:29:08 <wumpus> you could add these zero-cost linux abstraction hooks to bitcoind (forgot the name)
19:29:16 <jb55> ebpf
19:29:20 <wumpus> yess
19:29:22 <jb55> I have a branch for it
19:29:57 <wumpus> that's extremely flexible and allows for all kinds of diagnostics and experiments without impact on the code itself
19:30:15 <jb55> I've been using it to time ibd and gathering histograms. it just inserts NOP instructions that linux hooks into
19:30:21 <jb55> so only had overhead when you are plugged in
19:30:29 <jb55> but you could also just compile it with traces disabled
19:31:23 <troygiorshev> well I'll definitely be checking out everything that was said here, thanks everyone!
19:31:32 <wumpus> someone even did a presentation at a bitcoin coredev meetup about this once, I forgot who
19:31:50 <luke-jr> iirc gcc can inject tracing stuff auto
19:32:08 <jb55> here's an example bpftrace script that I am using + a connect_block usdt tracepoint https://jb55.com/s/ibd.bt.txt
19:32:28 <wumpus> jb55: thanks!
19:32:39 <sipa> seeing USDT in this channel scares me
19:32:39 <troygiorshev> jb55: amazing!
19:32:41 <jb55> so you can share these bpftrace scripts that gather stats without hardcoding anything into core itself, you just have to insert the tracepoints in interesting places
19:32:59 <jb55> and you can extract values as well from the tracepoints, so you can log structs, etc
19:33:00 <jnewbery> wumpus: are you thinking of evan klitzke's work on flame graphs and probing?
19:33:20 <wumpus> eklitzke was that
19:33:22 <jamesob> jnewbery: I thought same but I think those are kernel probs; this looks like userland stuff
19:33:22 <wumpus> yes
19:33:29 <jamesob> *probes
19:33:38 <wumpus> they work in userland too
19:33:43 <wumpus> jnewbery: yes!
19:33:44 <jb55> yeah the perf tool uses the same underlying tech, linux tracepoints, ebpf a way of executing these scripts securely within the kernel as bytecode
19:34:17 <wumpus> could add a probe that is called on every P2P packet, for ex.
19:34:20 <jb55> that's what the script I linked is doing. that is compiled and run within the kernel, and taps into those usdt tracepoints
19:34:47 <jb55> it can also access low level io/net kernel traces within the same script which is nice
19:35:12 <jb55> might be overkill but its super powerful
19:35:17 <wumpus> that's really awesome
19:35:49 <wumpus> well it's overkill but also the range of things you miht need for diagnosing or testing specific things calls for something flexible which is why it is that way
19:36:03 <jb55> here's the patch I'm using to experiment with it https://jb55.com/s/tracepoints.patch.txt
19:36:21 <wumpus> the alternative is generally to recompile with all kinds of instrumentation but that requires a recompile and patching every time
19:36:54 <jamesob> seems like it'd make a lot of sense to build in ./configure-able dtrace taps...
19:36:56 <jb55> if there's interest I would PR it, just wasn't sure
19:37:17 <wumpus> there definitely is interest!
19:37:20 <jb55> kk
19:37:31 <jonatack> jb55: yes
19:37:42 <jamesob> jb55: cool work
19:38:18 <sipa> indeed
19:38:56 <wumpus> we need to cover some other topics today so moving on for now, maybe discuss this further out of meeting
19:39:04 <wumpus> #topic Merging of Schnorr in libsecp256k1 (sipa)
19:39:14 <sipa> hi
19:39:23 <troygiorshev> yep thanks again everyone I'll make an issue with this more formalized and many questions answered!
19:39:38 <wumpus> troygiorshev: thanks
19:39:50 <sipa> so with the prospect of having BIP340-342 one day merged, there will need to be a time to merge BIP340 support in libsecp256k1
19:40:02 <sipa> and i was wondering if we have a good feel for when the right time is
19:40:36 <wumpus> I don't see any drawbacks to merging it as an optionally enabled feature
19:40:45 <sipa> yeah
19:41:05 <sipa> i think the code is pretty much done; it was recently stripped of some future feature (batch validation) and cleaned up
19:41:11 <nickler> it's an experimental module right now, so it needs to be explicitly enabled
19:41:19 <sipa> of course it'd be experimental for now
19:41:27 <sipa> (thanks to nickler)
19:41:48 <sipa> do we want to update libsecp256k1 in master before adding the schnorr code first?
19:41:54 <sipa> so that the diff is minimized?
19:41:54 <wumpus> I guess before exposing it publicly there needs to be some agreement with regard to the interface, though not 100%, it's expected for there to be some drift over the initial releases with it
19:42:17 <wumpus> sipa: I don't think that can hurt in any case
19:42:26 <sipa> to me, it feels that there needs to be some confidence that this is the functionality that will eventually make it into bitcoin
19:42:36 <wumpus> last secp256k1 subtree update has been a while
19:42:43 <sipa> we wouldn't just merge support for anything in libsecp256k1, being a subproject of bitcoin core
19:42:57 <wumpus> there's a lot of expectations around it ending up in bitcoin eventually at least
19:43:08 <sipa> but i think that confidence can be (significantly) lower than full consensus rules implemented in core
19:43:16 <sipa> *than what is needed for
19:43:17 <nickler> fwiw the PR looks good to me right now and many people looked at it already but it has changed quite a bit over time. If these people would be interested in having another look, that would be helpful.
19:43:42 <elichai2> I'd like to review the new keypair direction but I do hope that BIP340 will actually end up in bitcoin in the end so I think it's ok to move toward merging schnorr to libsecp
19:44:17 <wumpus> I think the most important thing to be confident about is that the algorithm, and the code, is secure
19:44:18 <sipa> link: https://github.com/bitcoin-core/secp256k1/pull/558
19:44:28 <real_or_random> elichai2: I mean we don't need to merge it in the current state.
19:45:47 <real_or_random> but it's good to know when we feel confident enough that we want it in the codebase at all
19:45:59 <sipa> yeah
19:46:08 <sipa> that's why i'm bringing it up here
19:46:56 <sipa> unless there are other comments, that's all from me
19:46:57 <nickler> Afaik we've addressed all comments on the BIPS on the mailing list in some form of another
19:47:30 <sipa> some TODOs left for clarifications/rationales, but no semantics changes
19:47:38 <jamesob> who are the people who have spent enough time on this to give meaningful ACKs?
19:48:47 <nickler> for the libsecp PR there's a few that could review it relatively quickly (elichai, real-or-random, sipa)
19:49:26 <jamesob> I'm curious if there's anyone here who *doesn't* think schnorr will or should eventually be a part of bitcoin
19:49:34 <elichai2> I think the big "political" question in terms of merging is if anyone believes that BIP340 doesn't have a good chance of landing in bitcoin core
19:49:40 <elichai2> jamesob: exactly :)
19:49:41 <jamesob> right
19:49:47 <wumpus> FWIW: this was what eklitzke wrote about tracepoints at the time: https://eklitzke.org/how-sytemtap-userspace-probes-work
19:49:47 <sipa> jamesob: i think the right question is whether it will be part of bitcoin *in this form*
19:49:55 <jamesob> sipa: ah
19:50:09 <troygiorshev> wumpus: thanks
19:50:35 <elichai2> sipa: by *this form* I assume you mean the algorithm and not the API?
19:50:45 <sipa> elichai2: yes
19:51:33 <elichai2> sipa: did you get any comments on the curve mailinglist?
19:51:38 <wumpus> we have 10 minutes left and two (small) topics left
19:51:43 <jamesob> I think anyone who feels they're capable of evaluation should speak up, because I think a whole lot of us are not qualified...
19:51:50 <real_or_random> also not everything is in the current PR
19:51:59 <real_or_random> e.g., no batch verification at the moment
19:52:53 <wumpus> but I guess it's better to postpone those to next week
19:52:54 <real_or_random> we (the BIPauthors) feel it's in a good state, I think otherwise sipa wouldn't bring this up
19:53:24 <elichai2> The last time I reviewed it it was in a pretty good state, I assume it's even better now
19:53:25 <jeremyrubin> jamesob: it's also useful to have people out themselves as useless
19:53:42 <jamesob> jeremyrubin: /me raises hand
19:53:47 <jeremyrubin> jamesob: rather than seeming passive ack. I'm not opposed but am unqualified to comment
19:54:23 <jamesob> I think I would not be wrong in characterizing most people at this meeting as "generally favorable towards schnorr -> bitcoin but unable to provide meaningful commentary on the specifics"
19:55:16 <sipa> i think part of the question is here are about measuring community expectations, and not necessarily technical review itself
19:55:17 <jamesob> IMO PR it. Best way to get the qualified commentary out.
19:55:47 <wumpus> definitely favorable enough to merge it into secp256k1, which is a small step and easily reverted
19:56:18 <MarcoFalke> Well, a bump to the current libsecp wouldn't hurt
19:56:28 <MarcoFalke> The overhead is just another two commits
19:56:31 <sipa> MarcoFalke: I'll PR a bump (pre-schnorr) soon
19:56:39 <MarcoFalke> thx
19:56:51 <sipa> there have been some nice improvements in master too
19:56:53 <wumpus> I don't think this is a point where Schnorr progress should be held up on
19:57:41 <wumpus> ofc. there will be a long stuggle to get it into bitcoin itself and hopefully a lot of people that will review the cryptography and code in detail
19:58:04 <MarcoFalke> Btw, my short topic was to delete the 0.14 and 0.15 branches because they won't be pushed to anymore and are EOL https://github.com/bitcoin-core/bitcoincore.org/pull/704
19:58:10 <MarcoFalke> Any objections?
19:58:30 <jeremyrubin> Keeping the tags tho?
19:58:31 <sipa> delete the branches, keep the tags?
19:58:35 <wumpus> ACK: if there have been no commits since the last version tag
19:58:36 <MarcoFalke> jeremyrubin: Sure
19:58:44 <jeremyrubin> sipa: jinx
19:58:48 <MarcoFalke> wumpus: The only commit is deleting the release notes
19:58:51 <wumpus> if there have been commits since please create a -final tag or so
19:58:56 <wumpus> fine w/ me
19:59:08 <wumpus> yes, the tags should *definitely* be kept
19:59:11 <wumpus> never delete tags
19:59:22 <MarcoFalke> I don't think we need another tag when just the release notes have been cleared
19:59:30 <sipa> agree
19:59:41 <wumpus> (e.g. see v0.12-final etc)
19:59:51 <wumpus> MarcoFalke: I agree if that's the only thing
20:00:36 <MarcoFalke> I think for 0.12 we had some code change backports
20:00:38 <wumpus> the other branches has unreleased bugfixes ported to them I think
20:00:40 <wumpus> yes
20:00:53 <MarcoFalke> Jup, 0.16 has a CVE fix xD
20:00:54 <wumpus> #endmeeting