19:00:14 <wumpus> #startmeeting
19:00:14 <lightningbot> Meeting started Thu Jun 18 19:00:14 2020 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:14 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:17 <jnewbery> hi
19:00:19 <moneyball> hi
19:00:30 <cfields> hi
19:00:37 <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:39 <wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2
19:00:44 <sipa> hi
19:00:46 <MarcoFalke> hi
19:00:54 <achow101> hi
19:00:59 <fjahr> Hi
19:01:24 <ajonas> hi
19:01:46 <meshcollider> hi
19:01:51 <wumpus> one proposed meeting topic for today: taproot implementation plan (moneyball)
19:01:56 <wumpus> any last minute topics?
19:02:11 <ariard> hi
19:02:29 <luke-jr> hi
19:03:01 <kanzure> hi
19:03:05 <wumpus> #topic High priority for review
19:03:15 <jeremyrubin> hola
19:03:22 <wumpus> https://github.com/bitcoin/bitcoin/projects/8  currently: 12 blockers, 3 chasing concept ACK
19:03:31 <wumpus> anything to add/remove?
19:03:33 <MarcoFalke> I'd like to add #19028
19:03:35 <gribble> https://github.com/bitcoin/bitcoin/issues/19028 | test: Set -logthreadnames in unit tests by MarcoFalke · Pull Request #19028 · bitcoin/bitcoin · GitHub
19:03:53 <luke-jr> wumpus: #18818 should probably be under Bugfixes, not Blockers? not sure on categorisation
19:03:56 <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:30 <wumpus> MarcoFalke: added
19:04:33 <jamesob> hi
19:04:35 <MarcoFalke> thx
19:04:41 <wumpus> luke-jr: ok moved
19:05:02 <cfields> luke-jr: gah, sorry, forgot about that one. Will take a look regardless of how it's tagged.
19:05:10 <luke-jr> cfields: thanks
19:05:19 <jonasschnelli> hi
19:05:20 <jamesob> tangential to 19028, maybe we should set logthreadnames=1 by default if we can show there isn't a performance hit
19:06:05 <luke-jr> I did notice #18818 was insufficient for Knots, but only because Knots is distributing files rendered from SVGs at dist-time - shouldn't affect Core's needs
19:06:07 <gribble> https://github.com/bitcoin/bitcoin/issues/18818 | Fix release tarball generated by gitian by luke-jr · Pull Request #18818 · bitcoin/bitcoin · GitHub
19:06:30 <wumpus> no strong opinion, but I'm not sure the thread names are useful to most non-developers
19:06:51 <wumpus> for running the tests it makes sense though
19:07:46 <jamesob> I'm thinking it could be useful when we ask for debug.logs in bug reports
19:07:57 <wumpus> but we had a similar discussion at the time about adding microseconds by default to the log - like okay, for some things it might be useful, but it just widens the log messages
19:07:59 <dongcarl> luke-jr: I think after exploring a bit in the `tarfiles` python module (quite powerful, and shipped with python by default), we can use it to "union" the `make dist` archive, and the `git-archive` archives, happy for yours to be merged in first, just wanted to mention it here.
19:08:50 <luke-jr> dongcarl: GNU tar can do it too, but I'm not sure it's worth the complexity
19:08:56 <wumpus> jamesob: can you show a specific example where it helps? are there many log messages that are ambigious as to where they originate?
19:09:09 <wumpus> (especially of those enabled by default)
19:09:16 <jamesob> wumpus: fair point, will raise again if I can
19:09:54 <wumpus> jamesob: thanks
19:10:22 <dongcarl> luke-jr: last time I tried it with GNU tar I remember it had weird behaviour, anyway, I'll shut up until I have code :-)
19:10:45 <luke-jr> dongcarl: also, I had originally made 18818 to do that, but I was uncertain of the ramifications of having different timestamps for the modified files
19:11:30 <luke-jr> (iirc, git-archive is using a timestamp potentially after the gitian reference timestamp, so the source files would appear "newer" possibly)
19:11:33 <jnewbery> I don't see any downside to logging thread names by default (unless there's a performance hit). It does make tracing what's going on in the log files a lot easier
19:12:13 <wumpus> jnewbery: do you have any specific examples where it wouldh elp or would have helped?
19:12:48 <dongcarl> luke-jr: true, but with the `tarfiles` python module we can make decisions on that programmatically ourselves, instead of GNU tar doing what its default conflict resolution is
19:12:56 <wumpus> the default amount of logging shouldn't be a performance hit in any way, adding a field isn't going to make it noticably worse
19:13:31 <wumpus> so I'm more concerned about making the log unneceessarily spammy/verbose than performance hit here
19:13:55 <gwillen> thread names are non-unique, right? it might be nice to log thread IDs in addition or instead.
19:14:15 <gwillen> otherwise all the worker threads will log the same name and it still won't help to tell them apart.
19:14:29 <wumpus> all the names are unique
19:14:32 <jamesob> gwillen: I think they're unique at the moment; those that aren't have an increasing suffix
19:14:45 <gwillen> ah okay great, nevermind
19:15:08 <jnewbery> wumpus: it's just easier when you're eyeballing the log to get an idea of what's going on. Obviously if you know or look up the location of every log call in the source, you can work it out.
19:15:25 <sipa> i just git grep the log message :)
19:15:43 <wumpus> sipa: same, it's the only way to be sure :) log messages tend to be unique enough
19:15:46 <sipa> (no objection to logging thread names by default if it has no performance impact, though)
19:17:05 <provoostenator> (hi)
19:17:07 <wumpus> but if everyone wants to add tghat field and I'm the only one slightly sceptical about it, just add it, no strong opinion
19:17:59 <sipa> let's first benchmark?
19:18:00 <jnewbery> validation interface logging is also super helpful if you haven't played around with it. It's nice to see when the signals being added to the callback queue and when they're serviced by the scheduler thread
19:18:10 <wumpus> if looking up a thread name really causes a performance hit something is really wrong btw
19:18:34 <wumpus> if logging is on the performance critical path (with the default amount of logging) in the first place
19:18:38 <sipa> agree, but haven't we had surprising experiencing with this in the past?
19:18:42 <jamesob> wumpus: yeah I reckon you're right. couldn't hurt to see if something's really wrong though
19:19:03 <jamesob> MarcoFalke has proposed to add a logging benchmark re: the previous issue, which I think is still open?
19:19:05 <sipa> make it it isn't looking up the thread name for log categories that are disbaled
19:19:23 <wumpus> sipa: there was some worry about enabling TLS causing a performance hit (independent of whether it was actually used frequently)
19:19:28 <wumpus> sipa: but this turned out not to be the case
19:19:53 <wumpus> (TLS as in Thread Local Storage, not the other thing)
19:20:07 <MarcoFalke> jup the log bench is still open
19:20:19 <MarcoFalke> imo it can be merged. The risk should be zero
19:20:22 <wumpus> I implemented the thread name lookup using a map but then it turned out to not be the problem at all
19:20:30 <jamesob> MarcoFalke: agree, think I've acked it
19:20:31 <sipa> ok
19:20:32 <wumpus> in any case please do not log on the critical path
19:20:41 <wumpus> definitely not by default (with debug flags is fine)
19:21:19 <wumpus> but if it's logging in an inner loop or something that really affects, say, validation performance, that's not how logigng should be used
19:22:07 <wumpus> it's why I find a logging benchmark kind of weird, we're not trying to optimize logging throughput
19:23:08 <jamesob> it's less for optimization and more just an assurance we're not doing anything totally dumb
19:23:24 <jnewbery> jamesob: agree
19:23:32 <MarcoFalke> the benchmark also checks that *disabled* logs don't affect performance
19:23:45 <MarcoFalke> maybe I should call is nolog bench
19:23:50 <MarcoFalke> *it
19:23:52 <wumpus> that's a good idea
19:24:50 <wumpus> logprintf arguments shouldn't even be evaluated in that case
19:25:06 <MarcoFalke> jup (I broke that once)
19:25:09 * MarcoFalke hides
19:25:24 <wumpus> heh
19:26:31 <wumpus> #topic Taproot implementation plan (moneyball)
19:26:48 <moneyball> Hi everyone, I wanted to check in here to get a sense for whether contributors are imagining the taproot implementation making it into v.21 or not. If yes, then it is likely the case that there needs to be pretty extreme focus in order to make it in time.
19:27:02 <moneyball> Here is a draft document that compiles a list of things that (arguably) need to be done for taproot to be complete. Would love feedback on this: is everything listed required? Is anything missing? https://docs.google.com/document/d/1DAMV9csW9k7vDh118_e65-IPJd4AcCImkvsB0b3gbNw/edit
19:28:21 <moneyball> (feel free to comment in the doc)
19:28:26 <moneyball> or here
19:28:41 <luke-jr> moneyball: activation isn't required
19:29:05 <moneyball> ok
19:29:09 <sipa> typically our process would be merging in a 0.x.0 release, and then implementating activation whenever in a 0.x.1
19:29:22 <wumpus> associated PR would be #17977 I guess
19:29:25 <gribble> https://github.com/bitcoin/bitcoin/issues/17977 | [WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa · Pull Request #17977 · bitcoin/bitcoin · GitHub
19:29:35 <luke-jr> signet isn't required
19:29:47 <jeremyrubin> yeah I think we don't usually aim for major releases to have forks, should be a minor
19:30:08 <jeremyrubin> So there really isn't any release window time crunch to push for
19:30:29 <jeremyrubin> But I agree in principal with trying to figure out a path for activation
19:30:34 <moneyball> luke-jr, sipa: well there is including the activation code and then separately configuring the activation parameters. right?
19:30:50 <sipa> moneyball: depends on the activation mechanism
19:31:02 <sipa> if a new activation mechanism needs code, then yes that needs code :)
19:31:08 <moneyball> ha :)
19:31:41 <jeremyrubin> One thing I'm curious to look at is if the recent changes to the sighash & the recent hardware wallet issues are informing or suggesting any other sighash changes we should be doing concurrently.
19:31:49 <moneyball> ok my understanding of that is that a new activation method is planned to be proposed to the mailing list, and IF there is consensus around that, then yes, that code would need to be added to Core
19:31:59 <ariard> haven't reviewed yet 17977 yet, what's the test coverage of it ? do we need to add more support for testing taproot tree composition in fuzzing or test framework?
19:32:50 <sipa> ariard: the python code is effectively an extensive generate-random cases with lots of edge cases, and compare the python-created signatures against block/tx validation
19:32:57 <sipa> ariard: more testing is absolutely welcome of course
19:33:05 <jeremyrubin> moneyball: fwiw I've talked with a bunch of contributors and I think Modern Soft Fork Activation is far from a universally loved approach. That conversation should probably be had more exentsively before you hitch taproot onto that wagon
19:33:11 <sipa> moneyball: that sounds like it needs ML discussion first
19:33:13 <moneyball> luke-jr: ok i guess i understand that signet is not required per se, but, some kind of test plan would be. has there been much discussion and consensus for how to test this in Core?
19:33:35 <moneyball> sipa: yes
19:33:57 <sipa> it's hard to ask people here what they think about an approach that hasn't even been published yet
19:35:03 <moneyball> my hope was to focus on non-activation method work needed in Core
19:35:11 <sipa> yeah, that makes sense
19:35:13 <moneyball> perhaps it was a mistake having line item one in that doc
19:35:41 <provoostenator> Although not required, it would be really nice to have a Taproot Signet.
19:36:41 <sipa> i think implementation wise that list pretty much covers it
19:36:55 <moneyball> ok i just deleted that line item from the doc :)
19:37:32 <moneyball> i would love to hear more discussion about testing approach. what is there general agreement on? what are open questions that need to be discussed?
19:39:39 <jeremyrubin> I think that running on signet doesn't really do anything by itself
19:39:40 <luke-jr> provoostenator: sure, I was just answering moneyball's request for things not required :P
19:39:48 <jeremyrubin> The real challenge is to get integration tests somewhere
19:39:58 <provoostenator> moneyball: having a signet explorer somewhere can help with testing too
19:40:03 <jeremyrubin> E.g., people attempting to integrate it and acutally use taproot
19:40:31 <jeremyrubin> I would put more stock in, e.g., an LND fork with taproot support against regtest than signet (but signet would be great too)
19:40:35 <sipa> jeremyrubin: that would be great, but i fear that it's a bit of a chicken and egg problem
19:40:39 <luke-jr> segnet worked okay AFAIR
19:40:40 <luke-jr> testing should be before merge anyway
19:41:06 <provoostenator> luke-jr: signet could be in a release and completely changed in the next release though
19:41:20 <ariard> sipa: does feature_taproot.py attempt any coverage-guided like a fuzzer?
19:41:22 <sipa> yeah, if not signet we can create a (normal) testnet with it activated too (i think signet would be preferable, but if it somehow doesn't make it in time, i don't think that would be a blocker)
19:41:25 <sipa> ariard: no
19:41:27 <provoostenator> Or we could release a taproot signet binary seperately
19:41:40 <jeremyrubin> sipa: indeed it's hard. I think if signet comes out then people will integrate test against it
19:41:54 <luke-jr> provoostenator: that seems like a given
19:42:00 <jeremyrubin> Just more noting that just getting signet out doesn't do anything in terms of progress alone
19:42:19 <luke-jr> provoostenator: otherwise we'd be merging taproot before testing it
19:42:19 <sipa> ariard: fuzzing definitely makes sense to test for things like memory violations and UB
19:42:36 <wumpus> well if it helps getting more attention to testing taproot, that's progress
19:42:44 <sipa> luke-jr: i think there are different stages of testing, and different stages of getting attention to it
19:43:04 <ariard> sipa: right you may have nast edges cases we wide trees and oversized tapscripts?
19:43:22 <bitcoin-git> [13bitcoin] 15jnewbery opened pull request #19322: [net] split PushInventory() (06master...062020-06-split-push-inventory) 02https://github.com/bitcoin/bitcoin/pull/19322
19:43:26 <sipa> ariard: there is a test for the max depth of the tree, if that's what you're asking for
19:43:37 <sipa> luke-jr: and at some point it will need to be merged for people to test against a kind of testnet, which hopefully informs discussions on activation
19:43:43 <ariard> okay great
19:44:02 <sipa> it can't be testnet/signet tested before being merged - but different kinds of testing are obviously necessary before that point
19:44:15 <jeremyrubin> I think the issue with signet is it doesn't add a new message type/storage place for the signatures
19:44:24 <jeremyrubin> I understand why kallewoof did it that way and it makes sense
19:44:36 <jeremyrubin> But it just makes it difficult for people to want to merge it
19:44:40 <sipa> jeremyrubin: this seems orthogonal
19:44:54 <jeremyrubin> slightly
19:45:12 <sipa> i don't think taproot should be blocked by signet in any case
19:45:16 <luke-jr> sipa: but I think we want tapnet before merging?
19:45:18 <jonatack> hi... fwiw MarcoFalke, fjahr, brakmic and I were testing signet for some time and going back and forth with kallewoof on improvements... iirc it's the PR is in pretty good shape
19:45:23 <sipa> luke-jr: perhaps
19:45:31 <sipa> luke-jr: i think that may make sense
19:45:39 <jeremyrubin> What about just flag daying testnet?
19:45:54 <sipa> there aren't any associated P2P changes, so i think the need for that level of testing may be lower than with segwit
19:46:19 <wumpus> as said, testnet needs to be compatible between releases, so there's not much scope for experimentation there
19:46:29 * luke-jr glad to hear brakmic hasn't given up on us completely :x
19:46:36 <luke-jr> wumpus: doesn't need to be..
19:47:43 <ariard> do we expect to introduce new standard rules on taproot witness?
19:47:45 <wumpus> I mean, I think there should be a flag day on testnet before considering activation on mainnet, but only after the protocol and implementation is virtually finalized
19:47:49 <MarcoFalke> lol, wasn't testnet hardforked for segwit?
19:48:21 <MarcoFalke> I mean silent hardfork. "hardfork" is probably the wrong word
19:48:22 <jeremyrubin> wumpus: ah I see. I thought we can just reset testnet if we want. Does anyone care?
19:48:41 <jeremyrubin> wumpus: you can also make a soft fork flag day that a rule is enforced for N blocks only
19:48:43 <wumpus> jeremyrubin: it's possible but should probably be avoided
19:48:58 <sipa> ariard: yes, though they're pretty weak; upgradability (annex, new leaf versions, ...), and max stack item size
19:49:03 <sipa> ariard: https://github.com/bitcoin/bitcoin/pull/17977/commits/fa2b4fded614ef2424404b22a07bfbdb2d77ea6c
19:49:18 <wumpus> doing things like 'reset testnet' isn't going to make changes more popular
19:49:36 <jeremyrubin> wumpus: what about flag day testnet and rule only valid for 6 mos of blocks?
19:50:30 <provoostenator> I find it hard to believe a non-trivial change to testnet is more difficult than signet.
19:50:47 <jeremyrubin> Would make it easier to do sort of rolling releases on testnet if there's a worry about wanting to permanently be in step with core
19:50:58 <sipa> meh, we can just create specialized testnets
19:51:03 <sipa> before or after merge
19:51:14 <luke-jr> contention to resetting testnet, is a reason to reset testnet :P
19:51:24 <sipa> testnet has compatbility requirements
19:51:29 <sipa> specialized things don't
19:52:03 <wumpus> right, a specialized testnet would make sense, that's basically what signet is anyway (except the mining part)
19:52:14 <sipa> indeed
19:52:22 <moneyball> do we consider this PR required for taproot? https://github.com/bitcoin/bitcoin/pull/18044
19:52:50 <moneyball> and this one? https://github.com/bitcoin/bitcoin/pull/19184
19:52:50 <sipa> moneyball: i believe sdaftuar has some thoughts on that
19:53:05 <ariard> sipa: indeed all of them are constraints on new data structure so no risk to tamper with network/break existent applications
19:54:02 <jeremyrubin> network stuff isn't required
19:54:06 <jeremyrubin> It can be done after
19:54:18 <sipa> jeremyrubin: read the wtxid relay PR
19:54:24 <sipa> it gives a justification
19:55:23 <ariard> right because v0 segwit nodes are going to waste bandwidth constantly redownloading taproot txn they can't verify
19:55:38 <sipa> indeed
19:55:57 <sipa> this depends on how upgraded nodes are at the time of activation of course
19:56:15 <sipa> so it may not be a big issue, but having a way to reduce that impact beforehand sounds like an improvement
19:56:31 <ariard> ofc how long it takes to get 80% of segwit nodes ? or similar number based on previous forks?
19:56:39 <jeremyrubin> yeah I am familiar. It's not great, but I personally don't think it's blocking
19:56:57 <sipa> ok
19:57:18 <jeremyrubin> I could be wrong on that though
19:57:50 <sipa> moneyball: i think wtxid could be done before 19184
19:58:10 <sipa> (but i'm obviously biased in liking 19184 to get in)
19:58:20 <ariard> jeremyrubin: don't you have a bad effect as we see more taproot txn ande nodes relaying them the cost is increasing non-linearly for non-upgraded nodes?
19:58:34 <wumpus> meeting time about to end
19:58:50 <sipa> ariard: well, it's linear, but with a possibly big constant factor
19:59:09 <moneyball> ok thank you for the feedback. this has been valuable. lots to follow-up on though!
20:00:18 <wumpus> #endmeeting