19:00:14 #startmeeting 19:00:14 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 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:17 hi 19:00:19 hi 19:00:30 hi 19:00:37 #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 jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2 19:00:44 hi 19:00:46 hi 19:00:54 hi 19:00:59 Hi 19:01:24 hi 19:01:46 hi 19:01:51 one proposed meeting topic for today: taproot implementation plan (moneyball) 19:01:56 any last minute topics? 19:02:11 hi 19:02:29 hi 19:03:01 hi 19:03:05 #topic High priority for review 19:03:15 hola 19:03:22 https://github.com/bitcoin/bitcoin/projects/8 currently: 12 blockers, 3 chasing concept ACK 19:03:31 anything to add/remove? 19:03:33 I'd like to add #19028 19:03:35 https://github.com/bitcoin/bitcoin/issues/19028 | test: Set -logthreadnames in unit tests by MarcoFalke · Pull Request #19028 · bitcoin/bitcoin · GitHub 19:03:53 wumpus: #18818 should probably be under Bugfixes, not Blockers? not sure on categorisation 19:03:56 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 MarcoFalke: added 19:04:33 hi 19:04:35 thx 19:04:41 luke-jr: ok moved 19:05:02 luke-jr: gah, sorry, forgot about that one. Will take a look regardless of how it's tagged. 19:05:10 cfields: thanks 19:05:19 hi 19:05:20 tangential to 19028, maybe we should set logthreadnames=1 by default if we can show there isn't a performance hit 19:06:05 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 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 no strong opinion, but I'm not sure the thread names are useful to most non-developers 19:06:51 for running the tests it makes sense though 19:07:46 I'm thinking it could be useful when we ask for debug.logs in bug reports 19:07:57 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 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 dongcarl: GNU tar can do it too, but I'm not sure it's worth the complexity 19:08:56 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 (especially of those enabled by default) 19:09:16 wumpus: fair point, will raise again if I can 19:09:54 jamesob: thanks 19:10:22 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 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 (iirc, git-archive is using a timestamp potentially after the gitian reference timestamp, so the source files would appear "newer" possibly) 19:11:33 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 jnewbery: do you have any specific examples where it wouldh elp or would have helped? 19:12:48 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 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 so I'm more concerned about making the log unneceessarily spammy/verbose than performance hit here 19:13:55 thread names are non-unique, right? it might be nice to log thread IDs in addition or instead. 19:14:15 otherwise all the worker threads will log the same name and it still won't help to tell them apart. 19:14:29 all the names are unique 19:14:32 gwillen: I think they're unique at the moment; those that aren't have an increasing suffix 19:14:45 ah okay great, nevermind 19:15:08 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 i just git grep the log message :) 19:15:43 sipa: same, it's the only way to be sure :) log messages tend to be unique enough 19:15:46 (no objection to logging thread names by default if it has no performance impact, though) 19:17:05 (hi) 19:17:07 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 let's first benchmark? 19:18:00 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 if looking up a thread name really causes a performance hit something is really wrong btw 19:18:34 if logging is on the performance critical path (with the default amount of logging) in the first place 19:18:38 agree, but haven't we had surprising experiencing with this in the past? 19:18:42 wumpus: yeah I reckon you're right. couldn't hurt to see if something's really wrong though 19:19:03 MarcoFalke has proposed to add a logging benchmark re: the previous issue, which I think is still open? 19:19:05 make it it isn't looking up the thread name for log categories that are disbaled 19:19:23 sipa: there was some worry about enabling TLS causing a performance hit (independent of whether it was actually used frequently) 19:19:28 sipa: but this turned out not to be the case 19:19:53 (TLS as in Thread Local Storage, not the other thing) 19:20:07 jup the log bench is still open 19:20:19 imo it can be merged. The risk should be zero 19:20:22 I implemented the thread name lookup using a map but then it turned out to not be the problem at all 19:20:30 MarcoFalke: agree, think I've acked it 19:20:31 ok 19:20:32 in any case please do not log on the critical path 19:20:41 definitely not by default (with debug flags is fine) 19:21:19 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 it's why I find a logging benchmark kind of weird, we're not trying to optimize logging throughput 19:23:08 it's less for optimization and more just an assurance we're not doing anything totally dumb 19:23:24 jamesob: agree 19:23:32 the benchmark also checks that *disabled* logs don't affect performance 19:23:45 maybe I should call is nolog bench 19:23:50 *it 19:23:52 that's a good idea 19:24:50 logprintf arguments shouldn't even be evaluated in that case 19:25:06 jup (I broke that once) 19:25:09 * MarcoFalke hides 19:25:24 heh 19:26:31 #topic Taproot implementation plan (moneyball) 19:26:48 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 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 (feel free to comment in the doc) 19:28:26 or here 19:28:41 moneyball: activation isn't required 19:29:05 ok 19:29:09 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 associated PR would be #17977 I guess 19:29:25 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 signet isn't required 19:29:47 yeah I think we don't usually aim for major releases to have forks, should be a minor 19:30:08 So there really isn't any release window time crunch to push for 19:30:29 But I agree in principal with trying to figure out a path for activation 19:30:34 luke-jr, sipa: well there is including the activation code and then separately configuring the activation parameters. right? 19:30:50 moneyball: depends on the activation mechanism 19:31:02 if a new activation mechanism needs code, then yes that needs code :) 19:31:08 ha :) 19:31:41 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 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 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 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 ariard: more testing is absolutely welcome of course 19:33:05 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 moneyball: that sounds like it needs ML discussion first 19:33:13 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 sipa: yes 19:33:57 it's hard to ask people here what they think about an approach that hasn't even been published yet 19:35:03 my hope was to focus on non-activation method work needed in Core 19:35:11 yeah, that makes sense 19:35:13 perhaps it was a mistake having line item one in that doc 19:35:41 Although not required, it would be really nice to have a Taproot Signet. 19:36:41 i think implementation wise that list pretty much covers it 19:36:55 ok i just deleted that line item from the doc :) 19:37:32 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 I think that running on signet doesn't really do anything by itself 19:39:40 provoostenator: sure, I was just answering moneyball's request for things not required :P 19:39:48 The real challenge is to get integration tests somewhere 19:39:58 moneyball: having a signet explorer somewhere can help with testing too 19:40:03 E.g., people attempting to integrate it and acutally use taproot 19:40:31 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 jeremyrubin: that would be great, but i fear that it's a bit of a chicken and egg problem 19:40:39 segnet worked okay AFAIR 19:40:40 testing should be before merge anyway 19:41:06 luke-jr: signet could be in a release and completely changed in the next release though 19:41:20 sipa: does feature_taproot.py attempt any coverage-guided like a fuzzer? 19:41:22 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 ariard: no 19:41:27 Or we could release a taproot signet binary seperately 19:41:40 sipa: indeed it's hard. I think if signet comes out then people will integrate test against it 19:41:54 provoostenator: that seems like a given 19:42:00 Just more noting that just getting signet out doesn't do anything in terms of progress alone 19:42:19 provoostenator: otherwise we'd be merging taproot before testing it 19:42:19 ariard: fuzzing definitely makes sense to test for things like memory violations and UB 19:42:36 well if it helps getting more attention to testing taproot, that's progress 19:42:44 luke-jr: i think there are different stages of testing, and different stages of getting attention to it 19:43:04 sipa: right you may have nast edges cases we wide trees and oversized tapscripts? 19:43:22 [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 ariard: there is a test for the max depth of the tree, if that's what you're asking for 19:43:37 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 okay great 19:44:02 it can't be testnet/signet tested before being merged - but different kinds of testing are obviously necessary before that point 19:44:15 I think the issue with signet is it doesn't add a new message type/storage place for the signatures 19:44:24 I understand why kallewoof did it that way and it makes sense 19:44:36 But it just makes it difficult for people to want to merge it 19:44:40 jeremyrubin: this seems orthogonal 19:44:54 slightly 19:45:12 i don't think taproot should be blocked by signet in any case 19:45:16 sipa: but I think we want tapnet before merging? 19:45:18 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 luke-jr: perhaps 19:45:31 luke-jr: i think that may make sense 19:45:39 What about just flag daying testnet? 19:45:54 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 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 wumpus: doesn't need to be.. 19:47:43 do we expect to introduce new standard rules on taproot witness? 19:47:45 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 lol, wasn't testnet hardforked for segwit? 19:48:21 I mean silent hardfork. "hardfork" is probably the wrong word 19:48:22 wumpus: ah I see. I thought we can just reset testnet if we want. Does anyone care? 19:48:41 wumpus: you can also make a soft fork flag day that a rule is enforced for N blocks only 19:48:43 jeremyrubin: it's possible but should probably be avoided 19:48:58 ariard: yes, though they're pretty weak; upgradability (annex, new leaf versions, ...), and max stack item size 19:49:03 ariard: https://github.com/bitcoin/bitcoin/pull/17977/commits/fa2b4fded614ef2424404b22a07bfbdb2d77ea6c 19:49:18 doing things like 'reset testnet' isn't going to make changes more popular 19:49:36 wumpus: what about flag day testnet and rule only valid for 6 mos of blocks? 19:50:30 I find it hard to believe a non-trivial change to testnet is more difficult than signet. 19:50:47 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 meh, we can just create specialized testnets 19:51:03 before or after merge 19:51:14 contention to resetting testnet, is a reason to reset testnet :P 19:51:24 testnet has compatbility requirements 19:51:29 specialized things don't 19:52:03 right, a specialized testnet would make sense, that's basically what signet is anyway (except the mining part) 19:52:14 indeed 19:52:22 do we consider this PR required for taproot? https://github.com/bitcoin/bitcoin/pull/18044 19:52:50 and this one? https://github.com/bitcoin/bitcoin/pull/19184 19:52:50 moneyball: i believe sdaftuar has some thoughts on that 19:53:05 sipa: indeed all of them are constraints on new data structure so no risk to tamper with network/break existent applications 19:54:02 network stuff isn't required 19:54:06 It can be done after 19:54:18 jeremyrubin: read the wtxid relay PR 19:54:24 it gives a justification 19:55:23 right because v0 segwit nodes are going to waste bandwidth constantly redownloading taproot txn they can't verify 19:55:38 indeed 19:55:57 this depends on how upgraded nodes are at the time of activation of course 19:56:15 so it may not be a big issue, but having a way to reduce that impact beforehand sounds like an improvement 19:56:31 ofc how long it takes to get 80% of segwit nodes ? or similar number based on previous forks? 19:56:39 yeah I am familiar. It's not great, but I personally don't think it's blocking 19:56:57 ok 19:57:18 I could be wrong on that though 19:57:50 moneyball: i think wtxid could be done before 19184 19:58:10 (but i'm obviously biased in liking 19184 to get in) 19:58:20 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 meeting time about to end 19:58:50 ariard: well, it's linear, but with a possibly big constant factor 19:59:09 ok thank you for the feedback. this has been valuable. lots to follow-up on though! 20:00:18 #endmeeting