19:00:29 <wumpus> #startmeeting
19:00:29 <lightningbot> Meeting started Thu Nov  7 19:00:29 2019 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:29 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:33 <kanzure> hi
19:00:39 <gleb> hi
19:00:41 <jonatack> bonsoir
19:00:51 <jnewbery> hi
19:00:56 <dongcarl> saluton
19:01:07 <digi_james> hi
19:01:07 <fanquake> hi
19:01:17 <provoostenator> hi
19:01:40 <wumpus> #topic 0.19.0 final
19:01:42 <meshcollider> hi
19:01:51 <sipa> hi
19:02:07 <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:02:09 <wumpus> jeremyrubin lightlike
19:02:17 <jamesob> hi
19:02:23 <fanquake> #17079 #16996
19:02:24 <gribble> https://github.com/bitcoin/bitcoin/issues/17079 | v0.19.0 testing · Issue #17079 · bitcoin/bitcoin · GitHub
19:02:25 <gribble> https://github.com/bitcoin/bitcoin/issues/16996 | Release process for 0.19.0 · Issue #16996 · bitcoin/bitcoin · GitHub
19:02:42 <fanquake> Does anyone have new issues to report after testing rc3?
19:02:49 <wumpus> I haven't heard of any new problems coming up with rc3, so it's probably time to tag final?
19:02:51 <instagibbs> hi
19:03:16 <fanquake> ACK. I haven't seen anything new either.
19:03:26 <achow101> hi
19:03:40 <fanquake> Also: https://github.com/bitcoin/bitcoin/milestone/37
19:03:52 <wumpus> well, first moving the release notes back to the branch from the wiki
19:04:10 <fanquake> and adding the macOS catalina note
19:04:17 <wumpus> so if anyone wants to do any last minute edits in the release notes, now would be the time
19:05:34 <MarcoFalke> ship it
19:05:49 <wumpus> yess
19:05:56 <fanquake> 🚀
19:06:01 <provoostenator> Ship it!
19:06:07 <arik_> 🛳
19:06:33 <wumpus> #topic High priority for review
19:06:36 <fanquake> I guess we could shift topics then?
19:06:38 <wumpus> https://github.com/bitcoin/bitcoin/projects/8
19:06:50 <wumpus> 7 blockers, 7 things chasing ACK
19:06:58 <wumpus> I think that's already a perfect number so let's move on
19:07:14 <BlueMatt> proposed topic: one last question blocking rust progress
19:07:33 <wumpus> (anything to add/remove?)
19:07:36 <achow101> add #17373 pls
19:07:38 <gribble> https://github.com/bitcoin/bitcoin/issues/17373 | wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet by achow101 · Pull Request #17373 · bitcoin/bitcoin · GitHub
19:08:19 <wumpus> achow101: ok, added
19:08:24 <fanquake> Only PR I'd suggest adding is #17270. I have some new comments to make there, and think it could be split up to aid review, but could use more eyes regardless.
19:08:26 <gribble> https://github.com/bitcoin/bitcoin/issues/17270 | Feed environment data into RNG initializers by sipa · Pull Request #17270 · bitcoin/bitcoin · GitHub
19:08:34 <gleb> I think there is some valuable discussion is going on here: #17326. Not sure it's a particularly high-prio, but I think it's worth looking at for whoever missed. It's about whether we should keep (some) p2p connections on a node restart or connect to new peers.
19:08:36 <gribble> https://github.com/bitcoin/bitcoin/issues/17326 | rfc, p2p: Eclipse attack mitigation · Issue #17326 · bitcoin/bitcoin · GitHub
19:08:58 <gleb> It's not even mine :P
19:09:09 <wumpus> it doesn't have to be to propose it
19:10:19 <wumpus> but yea, that would be more like a proposed meeting topic than something for review
19:10:54 <fanquake> Should we move onto a proposed topic? This might be a quick meeting for once.
19:11:18 <fanquake> is nickler or instagibbs here?
19:11:18 <gleb> I don't think at this point we need real-time discussion, as long as the conversation is happening there on github. Just wanted to bring a bit attention of everyone.
19:11:21 <MarcoFalke> fanquake: https://gist.github.com/moneyball/071d608fdae217c2a6d7c35955881d8a
19:11:25 <wumpus> fanquake: I think everyone is already looking at #17270
19:11:28 <gribble> https://github.com/bitcoin/bitcoin/issues/17270 | Feed environment data into RNG initializers by sipa · Pull Request #17270 · bitcoin/bitcoin · GitHub
19:11:41 <wumpus> happy with adding that to high prio, though
19:11:48 <MarcoFalke> Also, wumpus had another topic
19:11:57 <wumpus> yes
19:12:04 <fanquake> wumpus: if everyone is looking at it, I'm surprised there hasn't been more commentary
19:12:15 <wumpus> quite a lot of topics this week, let's move on
19:12:35 <wumpus> #topic security email address for bitcoin-core/secp256k1 (nickler)
19:12:46 <sipa> real_or_random, nickler: here?
19:13:06 <wumpus> https://github.com/bitcoin-core/secp256k1/pull/679
19:13:23 <nickler> hi
19:13:56 <BlueMatt> seems like an easy solution is proposed there: add a list for secp-securiy@bitcoincore.org and add relevant folks to it?
19:14:00 <nickler> We're adding a SECURITY.md file to the bitcoin-core/secp256k1 repo and considered secp256k1-security@bitcoincore.org as security email address.Would it be possible to add such an email address? How are we making sure that security contacts will actually get emails forwarded to them? Who can see the content of a vulnerability report if it's sent unencrypted?
19:14:01 <BlueMatt> I presume wumpus can do that?
19:14:20 <wumpus> yes, I can make a forward address on bitcoin-core.org
19:14:27 <BlueMatt> dig +short bitcoincore.org mx
19:14:27 <BlueMatt> 10 spool.mail.gandi.net.
19:14:27 <wumpus> just tell me who to include
19:14:50 <wumpus> doesn't have to be in the meeting :p
19:14:53 <nickler> wumpus: cool, will do in private
19:15:00 <sipa> we'd need to put it on the bitcoincore.org website as well, i think
19:15:09 <sipa> together with gpg keys of some/all of the people involved
19:15:19 <wumpus> sure
19:15:22 <BlueMatt> sounds good, next topic?
19:15:28 <bitcoin-git> [13bitcoin] 15elichai opened pull request #17402: Travis support for PowerPC64 (06master...062019-11-powerpc64) 02https://github.com/bitcoin/bitcoin/pull/17402
19:15:49 <wumpus> #topic opt-in SRD (instagibbs)
19:16:04 <wumpus> #17331
19:16:05 <gribble> https://github.com/bitcoin/bitcoin/issues/17331 | Use effective values throughout coin selection by achow101 · Pull Request #17331 · bitcoin/bitcoin · GitHub
19:17:04 <achow101> instagibbs wanted to ask whether we should consider making SRD something that is opt in instead of outright replacing the current knapsacksolver fallback
19:17:30 <wumpus> could at least initially do that I guess
19:17:38 <gleb> Is it assumed that everybody knows what does SRD stand for?
19:17:43 <achow101> so people who care could set some switch to use srd as the fallback so that we aren't breaking things too much
19:17:54 <sipa> gleb: single random draw
19:18:03 <wumpus> replacing it is goig to be much more controversial than adding a new method
19:18:15 <achow101> but I don't think anyone would opt in because what user knows that srd is or really cares about the coin selection algo details?
19:18:45 <achow101> My current pr for srd is to actually use both knapsack and srd, then choose the one that produces the "better" solution
19:18:48 <wumpus> it's something that needs to be tested/evaluate over a long period
19:18:50 <achow101> although defining "better" is non-trivial
19:18:54 <wumpus> people that want to test it can enable it
19:19:39 <achow101> i'm not convinced that people are going to test it
19:19:57 <wumpus> you mean, no one?
19:20:07 <meshcollider> Probably lol
19:20:18 <wumpus> that's a dangerous statement, if you think no one cares, why work on it?
19:20:43 <sipa> a possibility could be to run both, and add a debug log that tells you what SRD would have done instead
19:21:10 <achow101> I would guess that the people who would test it are not the same people who make enough transactions for it to matter
19:21:22 <wumpus> that doesn't matter
19:21:36 <sipa> there's a big difference between people in general being interested in a feature ("better coin selection" i think certainly qualified) and people with both the technical means and in a situation where they can provide good feedback
19:21:48 <jonatack> perhaps it needs awareness raised about it? via bitcoin optech, forums, etc.
19:21:49 <achow101> then there's no meaningful results or feedback
19:21:51 <wumpus> though it might mean you need to convince people that make enough transactions to test it, maybe
19:22:01 <wumpus> that's easier if it's an option
19:22:56 <wumpus> I don't think we can have this discussion without anyone arguing to add it
19:23:29 <wumpus> let's move to next topic
19:23:51 <wumpus> #topic Subtree organization and linters (wumpus)
19:24:08 <fanquake> linters 😺
19:24:27 <wumpus> so while trying to add crc32c to the tree (because the new leveldb uses that), I've noticed that the exceptions to linting are spread all over the place
19:24:56 <wumpus> I think it'd be useful to move subtrees to one place in the tree
19:25:13 <sipa> yes!
19:25:16 <MarcoFalke> Linters shouldn't be a reason to move source code around
19:25:17 <BlueMatt> ack
19:25:18 <wumpus> so that doxygen knows to avoid it, linters know to avoid it, etc
19:25:23 <gleb> broke: minting; woke: linting
19:25:39 <MarcoFalke> But I agree it makes sense even absent of linters
19:25:45 <BlueMatt> mostly cause putting them in one place is clean from a "this isnt our code, you should know that" human pov
19:25:57 <BlueMatt> but, yea, my preference for anything where linters come up is to remove them :)
19:26:00 <wumpus> soo
19:26:08 <wumpus> look at this commit: https://github.com/bitcoin/bitcoin/pull/17398/commits/50c86bfc0bfb44c3962b681320aee8d54fc8614d
19:26:09 <MarcoFalke> wumpus: Any reason to not run doxygen on it?
19:26:20 <wumpus> I had to change 10 places (and more) just to add one subtree
19:26:28 <wumpus> MarcoFalke: because it's not our code
19:26:39 <wumpus> I don't want general google documentation in our doxygen
19:26:47 <jonasschnelli> Can't attend the meeting today...
19:27:12 <wumpus> it's meant to document bitcoin core, not leveldb, or a crc32 library
19:27:36 <bitcoin-git> [13bitcoin] 15fanquake opened pull request #17403: doc: reintegrate 0.19.0 release notes (060.19...06reintegrate_relnotes_019) 02https://github.com/bitcoin/bitcoin/pull/17403
19:27:38 <MarcoFalke> ok, fine
19:27:58 <wumpus> do you think it would not only be clutter?
19:28:37 <wumpus> in any case that leaves the linters and everything
19:28:39 <wumpus> I think a mess now
19:29:29 <wumpus> it took me an hour or so to find them all
19:29:31 <MarcoFalke> #action Move subtrees to one place
19:29:49 <sipa> it seems ctaes and secp256k1 are included in doxygen, though
19:30:10 <wumpus> that's probably accidental
19:30:20 <jnewbery> an alternative would be to have an exclude.txt file in the linter directory and have all linters read from there
19:30:24 <wumpus> it's also easy to forget one
19:31:15 <jnewbery> (not saying we shouldn't change code organization structure, just agree with Marco that it shouldn't be driven by satisfying the linters)
19:31:17 <wumpus> yes, that's also possible
19:31:44 <wumpus> oh I agree
19:31:50 <sipa> or have a directory subtrees with all the subtrees? :)
19:31:52 <fanquake> another alternative would be to just remove all the linters, as they seem to take up a disproportionate amount of everyones time 🦆
19:32:22 <MarcoFalke> fanquake: Not having them also takes up time, unfortunately (as we recently saw)
19:32:22 <jnewbery> Concept ACK directory subtrees with subtrees
19:32:24 <jamesob> something something baby bathwater
19:32:40 <wumpus> FWIW doxygen input is already generated using automake so it'd be trivial to generate it from a central list
19:32:59 <wumpus> the linters are shell scripts all over the place
19:33:11 <BlueMatt> ignoring linters, I think its a good idea to put them all in one place, and it looks like no one materially disagreed?
19:33:12 <wumpus> using different ways to scan the tree
19:33:15 <wumpus> so that's more work...
19:33:16 <BlueMatt> so...next topic?
19:33:42 <fanquake> I have a real quick topic if there aren't any others left
19:33:48 <BlueMatt> i haz topic
19:33:49 <MarcoFalke> It is going to change all includes. Incoming merge conflicts
19:33:58 <wumpus> #topic rust in bitcoin core (BlueMatt)
19:34:10 <wumpus> MarcoFalke: yes, it's a mess to clean up a mess
19:34:20 <MarcoFalke> lol
19:34:23 <BlueMatt> one question about the rust stuff that wasn't answered in the last meeting (#16834)...in the last meeting the conclusion was "why merge if not shipped by default", so I set it as default, and this added a new question...if you dont have rustc installed, should configure fail, or just warn and not build rust.
19:34:23 <BlueMatt> cfields made the point that this results in silently building binaries with significantly different feature sets than release ones, which feels super strange, but I figure folks will complain if suddenly master stops building for them cause rustc is required unless you say --disable-rust. thoughts?
19:34:26 <gribble> https://github.com/bitcoin/bitcoin/issues/16834 | Fetch Headers over DNS by TheBlueMatt · Pull Request #16834 · bitcoin/bitcoin · GitHub
19:35:07 <BlueMatt> other than this it seems that pr is Ready to Go (tm)
19:35:24 <jamesob> seems pretty aggressive to me to require rustc by default
19:35:25 <wumpus> but if someone wants to fix the linters to use a central exclude list that's fine with me too ,anyhow, I really don't know what's the best solution here, but I think something needs to change
19:36:19 <fanquake> I don't think rustc should be a requirement to build master, I also agree that silently failing to build new feature sets feels like the wrong behaviour.
19:36:20 <wumpus> also I didn't really like including crc32c in our main src directory, it's kind of indirect
19:36:35 <wumpus> something like /src/external would be better
19:36:36 <sipa> why not just leave the rust stuff off by default?
19:36:49 <wumpus> yes, leave the rust stuff off by default, at least initially
19:36:50 <BlueMatt> sipa: you're three meetings late on that one :p
19:37:13 <sipa> yeah, haven't paid much attention to that
19:37:17 <sipa> but i don't see why that isn
19:37:24 <sipa> 't the obvious solution
19:37:34 <jnewbery> Sorry, I also missed the 'on by default' decision
19:37:35 <BlueMatt> well I mean we could also leave it off by default, but there was strong agreement to have it on by default in 0.20
19:37:46 <BlueMatt> I dont think there were any voices that disagreed with that
19:37:48 <promag> hi
19:37:55 <jnewbery> BlueMatt: do you have a link? Struggling to find it in my scrollback
19:37:58 <wumpus> I haven't seen that
19:38:05 <wumpus> I like to merge *some* rust code by 0.20
19:38:23 <BlueMatt> (built by default, not, like, making new network calls by default at runtime, that is)
19:38:24 <wumpus> I think it's best to have it disabled by default initally
19:38:25 <meshcollider> Maybe the "strong agreement" was just everyone missing the discussion :p
19:38:25 <jamesob> and just to clarify, there's no way that a failure in any of the rust subsystems can cascade over to the other processes, right?
19:38:27 <BlueMatt> (in case that was the confusion)
19:38:40 <wumpus> it's always possible to change a default
19:38:51 <wumpus> but you're going to have a merge harder time getting it merged it you want to require it
19:38:54 <BlueMatt> yes, jamesob, that is (to my knowledge) true.
19:39:06 <fanquake> #17090 some default discussion in here
19:39:08 <gribble> https://github.com/bitcoin/bitcoin/issues/17090 | RFC: Rust code integration · Issue #17090 · bitcoin/bitcoin · GitHub
19:39:10 <BlueMatt> wumpus: I personally dont want to require it, my preference was just "on if you have rustc available"
19:39:18 <BlueMatt> but cfields pointed out this felt....strange
19:39:30 <BlueMatt> cause you would just silently get things that had different feature sets from release
19:39:36 <wumpus> I like explicit configuration flags, but anyhow
19:39:56 <promag> BlueMatt: isnt that the same as with qt?
19:40:04 <jamesob> I think in general whenever we release major new features they should be opt-in (if possible) for a few releases to iron out kinks
19:40:05 <BlueMatt> you mean ./configure flags, anything written so far has to be opt-ed in at runtime
19:40:18 <sipa> promag: but Qt is actually built and enabled by default in release builds
19:40:19 <BlueMatt> promag: maybe zmq is a better example, at least with qt you just totally miss the bitcoin-qt binary
19:40:26 <meshcollider> why not off unless you have --enable-rust then?
19:40:40 <BlueMatt> jamesob: you mean opt-in, or not built-in, cause those are different
19:40:52 <wumpus> I'm not sure why this is such a big issue, just leave it experimental for the first release and only activated with a flag
19:41:05 <jamesob> BlueMatt: I mean available but disabled by default
19:41:05 <wumpus> this can always be chagned later
19:41:06 <fanquake> I feel like an explicit opt into rust stuff for 0.20.0 is ok. That would be the same as all this new android stuff. None of that is going to be anything other than explicitly opt in for a while
19:41:21 <promag> +1 fanquake
19:41:40 <wumpus> fanquake: exactly
19:41:41 <BlueMatt> wumpus: I'm confused, you mean on by default at runtime or built by default
19:41:52 <sipa> by default don't compile it in
19:41:53 <wumpus> it's much *easier* to merge things if they're introduced that way
19:41:53 <BlueMatt> same goes for fanquake...
19:42:09 <wumpus> disabled in the compile by default
19:42:15 <jnewbery> wumpus: +1
19:42:20 <jamesob> +1
19:42:22 <fanquake> BlueMatt I mean if you want to do rust things, do ./configure --enable-rust, and that's the only time it's used
19:42:30 <BlueMatt> lol wumpus i distinctly recall the opposite thought last meeting
19:42:35 <BlueMatt> but, ok, I can undo all those changes...
19:42:39 <BlueMatt> fanquake: lol you too...
19:42:51 <jnewbery> BlueMatt: no-one else remembers. I can't see any agreement in the Oct 10 meeting
19:43:29 <promag> BlueMatt: just to be clear, you suggest to enable if rustc is available?
19:44:25 <wumpus> I'm not sure having rustc in your path should change anything automatically to bitcoin configuration
19:44:40 <BlueMatt> yea, that was cfields' objection, which seems strange
19:45:09 <wumpus> but than again, I like explicit configuration options, I hate 'intelligent' software trying to second-guess me in general, so :)
19:45:23 <wumpus> any other topics?
19:45:38 <promag> I think it's fair to expect a first-time thing to be opt-in
19:45:59 <fanquake> I have one in regards to #15847
19:46:02 <gribble> https://github.com/bitcoin/bitcoin/issues/15847 | Feedback for GitHub CEO · Issue #15847 · bitcoin/bitcoin · GitHub
19:46:16 <wumpus> #topic github feedback (fanquake)
19:46:40 <fanquake> Basically just for everyone to dump any more thoughts they have into that thread, as the (in person) discussions with GitHub are happening early next week.
19:46:45 <wumpus> BlueMatt: let' sjust aim to get some rust code in, the default discussion is really something that can be left for later and only confuses the intial merge I think
19:47:05 <fanquake> There have already been some discussions in repos in GitHub in which I've started bringing up some of our suggestions.
19:47:08 <BlueMatt> right, ok, I'll just turn it off by default and we can have this conversation again post-merge.
19:47:23 <wumpus> yes, maybe in a major release or two
19:47:27 <promag> wumpus: I have one topic: cmd notify queue
19:47:30 <fanquake> Seems that some of the maintainers from other projects, which include some people from Rust core, agree with some of our concerns/issues.
19:48:01 <fanquake> That was all though. If you have any thoughts you don't wont to post in the issue, feel free to get in touch with me directly.
19:48:02 <sipa> fanquake: come say hi when you're in the bay area
19:48:37 <fanquake> sipa: will do
19:49:42 <wumpus> #topic cmd notify queue (promag)
19:49:48 <promag> so -walletnottify and -blocknotify spawn a thread which in turn call system()
19:50:17 <promag> and sometimes that can lead to some load - but that's not the issue now
19:50:22 <wumpus> you want an unbounded queue instead of a fork bomb :)
19:50:51 <bitcoin-git> [13bitcoin] 15sipsorcery opened pull request #17404: Remove redundant class file include from test_bitcoin msvc project (06master...06msvc_test) 02https://github.com/bitcoin/bitcoin/pull/17404
19:50:54 <wumpus> launching processes for notifications was always a bad idea
19:51:09 <promag> well, that can't be changed I guess
19:51:11 <wumpus> definitely if they happen often enough to consider things like that
19:51:31 <wumpus> well if it's just every 10 minutes no one complains
19:51:41 <promag> while replacing with boost::process I've noticed that the pace of process spawning is shorter
19:51:50 <wumpus> if it's a high frequency noticication you should use another mechanism
19:52:24 <promag> so I wonder wdyt about adding some queue
19:52:38 <promag> which has de bonus of guaranteeing order
19:53:14 <promag> also, the advantage is to avoid command line placeholders
19:53:23 <wumpus> that adds latency, also people expect the thread to run free (so keeping a command running doesn't block bitcoind), so  you can't guarantee much
19:53:24 <promag> and pass vars via env
19:54:18 <promag> wumpus: I think that if a command does that now I will have problems anyway
19:54:26 <promag> *it will
19:54:43 <wumpus> right now, it creates a new thread an starts the command in that
19:55:18 <promag> right, but if the command doesn't quit you end up with lots of threads?
19:55:31 <wumpus> if you want to create say, a single-threaded worker queue for notifications, that does change how it works
19:55:48 <wumpus> I'm sure it needs to quit some time
19:56:13 <promag> would you nack that?
19:56:24 <wumpus> I think it's a unnecessary change
19:56:38 <wumpus> how it is now works for the people that use the current mechanism
19:56:45 <wumpus> if you want more, use zmq
19:57:46 <wumpus> you're not going to make a system that supports high frequency notifications with process spawning
19:58:01 <promag> my problem is that in #13339 if I use boost::process notifications are slowly handled
19:58:06 <gribble> https://github.com/bitcoin/bitcoin/issues/13339 | wallet: Replace %w by wallet name in -walletnotify script by promag · Pull Request #13339 · bitcoin/bitcoin · GitHub
19:58:14 <wumpus> why?
19:58:41 <wumpus> is boost process so inefficient? what does it do differently?
19:58:44 <promag> my guess is that because internally it has to guard getenv/send etc
19:58:55 <promag> setenv
19:58:57 <sipa> how does a queue help?
19:59:02 <wumpus> promag: why?
19:59:09 <wumpus> you can provide an environment when you exec
19:59:14 <wumpus> there's no need to ever call setenv
19:59:41 <wumpus> in a multi-threaded process you definitely don't want to use that
19:59:44 <promag> at a lower level those calls aren't thread safe
20:00:01 <wumpus> there are thread safe calls at a low leven
20:00:03 <wumpus> l
20:00:26 <wumpus> oh, it's time
20:00:28 <wumpus> #endmeeting