19:01:00 #startmeeting 19:01:00 Meeting started Thu Sep 17 19:01:00 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:01:00 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:01:02 hi 19:01:03 vasild: i would really avoid doing anything that's unrelated to addr relay 19:01:18 #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 19:01:20 just risks expanding the scope unboundedly 19:01:21 amiti fjahr jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2 19:01:22 hi 19:01:27 we have a lot of topics for today, so let's start quickly 19:01:27 hi 19:01:29 bonsoir 19:01:30 hi 19:01:31 greetings 19:01:33 #topic High priority for review 19:01:34 hi 19:01:35 sipa: wumpus: ok 19:01:42 hi 19:01:46 hi 19:01:48 hi 19:01:51 https://github.com/bitcoin/bitcoin/projects/8 12 blockers, 1 bugfix, 2 chasing concept ACK 19:02:10 can i have #19953 ? 19:02:13 https://github.com/bitcoin/bitcoin/issues/19953 | Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa · Pull Request #19953 · bitcoin/bitcoin · GitHub 19:02:24 signet should be close to mergable, I hope we can get that one at least in for 0.21 19:03:00 PSA: the release schedule deadlines for 0.21 start october 1: https://github.com/bitcoin/bitcoin/issues/18947 19:03:01 #16546 is next in line for hardware wallet support 19:03:04 sipa: sure 19:03:05 https://github.com/bitcoin/bitcoin/issues/16546 | External signer support - Wallet Box edition by Sjors · Pull Request #16546 · bitcoin/bitcoin · GitHub 19:04:07 provoostenator: sipa: added 19:04:18 wumpus: let's put #11082 back in? 19:04:21 https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub 19:05:00 although it looks like I already have 2 there, the ramifications of missing 0.21 with this is pretty annoying 19:05:27 I'm still not sure about the writable configuration files (didn't we have two conflicting systems now?) but in any case, added 19:05:40 not the time to have that discussion now 19:05:59 #topic Endomorphism optimization in libsecp256k1 (sipa) 19:06:05 I have never seen such dual configs in any other software... 19:06:06 hi! 19:06:25 this is mostly a short announcement so it doesn't cause any surprise 19:07:05 libsecp256k1 started out as an experiment to see how much secp256k1 EC operations could be made by using the GLV endomorphism optimization, which it was specifically designed to support, but not actually implemented anywhere 19:07:09 vasild: that's kinda the point; it reduces to one config format 19:07:24 hi 19:07:45 as it turned out that there is some risk it is encumbered by a patent, the GLV optimization was made optional, and defaults to off (and has been off in every bitcoin core release) 19:08:14 it looks like that patent is expiring on september 25th, and blockstream had a patent attorney verify that (i'm happy to share their findings, if anyone cares) 19:08:27 yay! 19:08:31 wooo! 19:08:36 sipa: how sure can we be that it can't break consensus? 19:08:56 so the plan is to switch it to default on after that date, or even rip out the non-GLV code 19:08:58 luke-jr: good question 19:09:13 is it provable? :x 19:09:16 libsecp256k1' CI has always tested with both endomorphism enabled and disabled 19:09:42 including our exhaustive tests, which are probably as close to a mathemetical proof we can get for real software - at least for some aspects 19:10:24 fwiw, that's a test where the library is compiled with a slightly different curve equation that makes it trivially insecure, and only leaves 12 valid private/public keys 19:10:45 and in that mode, we can test literally every combination of signature/pubkey/private key 19:11:28 nice 19:11:31 so i think that given that, it shouldn't be any more invasive than regular code changes to libsecp256k1 19:11:35 has it been proven on a mathematical level? (not saying it's a problem if not, just asking) 19:12:19 luke-jr: for some parts of the code we have actual proofs (some hand-written, some automatic); though admittedly not the part touched by the endomorphism 19:12:35 I think he means in mathetmatical theory, not so much the specific code 19:12:40 right 19:13:12 wumpus: for the group arithmetic, there is a transliteration of the C code to python, which is then symbolically executed and can be automatically proven correct 19:13:37 Very cool, somewhat scary, but how much of a speedup is this? 19:13:37 the conversion from C to Python (or its semantics) of course aren't, but the algorithms at a slightly higher level are proven 19:13:39 sipa: that's a very interesting approach 19:13:47 provoostenator: 27% for ecdsa verification 19:13:59 Ok, that's worth some code review time alright. 19:14:00 very hard to say no to that :) 19:14:16 well 19:14:24 arguably, libsecp256k1 originally _only_ had the GLV mode 19:14:43 the mode where GLV was disabled (which is now default) was added later 19:14:56 yes it was added out of patent concerns 19:15:02 yes 19:15:22 but all changes since 2013 have always kept both GLV and non-GLV working, and tested 19:15:27 Interesting, I didn't know that 19:15:34 was the GLV mode ever released in Core? 19:15:47 no 19:15:59 luke-jr: it's a compile time option, and it was never enabled in (default) builds of core 19:16:10 you could always enable it yourself with --enable-endomorphism 19:16:25 right, not sure how many people actually did that tho :p 19:16:43 some people did it for benchmarking at times 19:16:50 apart from that, dunno 19:16:54 luke-jr: i assume nobody 19:16:57 If there are concernts, what about doing all calculus in both and comparing they produce the same result? 19:17:12 we do have a USE flag for it in Gentoo, but no metrics on usage 19:17:16 vasild: that's arguably what the unit tests are doing 19:17:26 if they differed, at least one would fail 19:17:43 vasild: you mean in real-world use? what's the point? 19:17:46 sipa: why not just test it out on all secp256k1 keys to make sure ;) 19:17:57 meshcollider: that's what the exhaustive test mode does 19:17:58 meshcollider: :D 19:18:01 I mean, in real life, in production, for e.g. 6 months. But I am not suggesting that, just saying "if there are concerns" :) 19:18:07 vasild: i believe that is pointless 19:18:21 ok, I can't judge 19:18:33 due to the cryptographic nature of things, actual correct _random_ usage is never going to trigger an edge case if one existed 19:18:34 I think you could just build with it enabled, and do a full sync 19:18:41 trying completely random input is very likely not going to find anything 19:18:44 right 19:18:45 if anything deviates, the sync should fail, right? 19:19:07 luke-jr: in theory it could be accepting invalid signatures, which wouldn't be caught by such a test 19:19:17 though again, this is true for every change to the cryptographic code 19:19:24 sipa: but vasild's suggestion wouldn't detect that either 19:19:30 luke-jr: indeed 19:19:38 the exhaustive test likely would though 19:19:58 or at least, has a reasonable chance to - it depends on the nature of the hypothetical bug 19:20:53 https://patents.google.com/patent/US7110538B2/en 19:20:59 I'd assume you have 100% code coverage of that code in the test? (not that that proves anything, of course, but at least all paths are being exercised) 19:21:40 wumpus: i believe we have code coverage of everything that isn't impossible to reach, but i'll go verify that 19:22:01 sipa: thanks! 19:22:29 so, expect a libsecp256k1 update shortly after september 25th 19:22:49 thanks for the announcement sipa, let's move to next topic 19:22:51 discussion on testing and whatnot can still happen in the PR 19:22:55 that's all from me 19:23:00 that's great news sipa! 19:23:07 #topic How should signet params be prefixed? (kallewoof) 19:23:38 basically my comment here https://github.com/bitcoin/bitcoin/pull/18267#discussion_r488814952 19:23:48 i suppose signetchallenge, as that's how all the others are, except reindex-chainstate that uses lispy kebab-case 19:24:13 I didn't like _ in command line parameters, - would be ok-ish with me (because it matches the - symbol at the beginning), but our convention seems to be to just concatentate 19:24:16 it looks like all 3 styles are used; there is -rpcpport, there is -reindex-chainstate, there is -output_csv (to bench) 19:24:39 my preference is the first (just squeeze things), but apart from that i don't care, and i don't think it's worth much discussion time :) 19:24:43 _ is definitely the worst to me at least, it's harder to type too 19:25:22 why not -signet= 19:25:22 I do think it is good to be consistent and come up with a standard way also for future arguments 19:25:44 traditionally we just stick them together without any separator, so just do that? 19:25:47 luke-jr: because there may be other signet arguments in the future 19:26:10 there are several alredy 19:26:26 and it's more consistent with -regtest -testnet to have it as a boolean anyhow 19:26:41 yes 19:26:52 achow101: +1 19:27:08 ACK squeezecase 19:27:45 okay, the sentiment here seems to be clear, if no one else is going to weigh in, we're going to next topic 19:28:14 #topic Size limit for data-driven unit tests (sipa) 19:28:27 hi! 19:29:26 o hai thar sipa? 19:29:27 in #19953 i've recently added a unit test with randomly-generated transaction validation success/failure cases, minimized using the fuzzing framework (it's not an actual fuzzer, all input is generated by a python script, but just minimized using the fuzz build) 19:29:29 https://github.com/bitcoin/bitcoin/issues/19953 | Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa · Pull Request #19953 · bitcoin/bitcoin · GitHub 19:29:54 which i think is an interesting approach as it permits getting the kind of coverage you get by running the python functional test for days... weeks... 19:30:10 it's 250 kB now, which seems in line with some of the other tests we have 19:30:26 but i could extend this to test more things, and in particular more validation flags 19:30:33 does that create a dep on fuzzing stuff for normal tests? :x 19:30:41 luke-jr: nope, just a json file 19:30:51 with the output of the whole fuzzing procedure 19:31:00 aha 19:31:22 anyway, trying to extend this, using the same approach, leaves me with things in the 1-2 MB range 19:31:25 I think we should be careful to not add too much data for tests to the repository, git is not that great for bulk data storage, though 250kB seems fine to me 19:31:28 and i was wondering if that's acceptable 19:31:54 there are many more tradeoffs possible, which can reduce that - or extend it - in exchange for coverage/development time 19:32:07 but if people are like "1 MB is just fine", that would simplify things 19:32:21 another drawback of large files is that it generates huge diffs, and this isn't really reviewable 19:32:23 I guess the runtime memory requirements are unchanged for that? 19:32:35 jonasschnelli: yes, it's just a (very simple) unit test 19:32:40 jonasschnelli: yes, it's only used at test time 19:32:41 it's also 1 MB of json which is presumably compressed quite a bit by git 19:32:42 sipa: does it compress at all in git? 19:32:47 the json contains ascii hex, what if we save it in binary? would be 2x reduce 19:32:49 hah 19:33:22 but as it's part of a unit test it also can't easily be moved to another repository like the fuzz dataset one 19:33:48 vasild: yes, possible - but if git compresses it already in a similar degree, leaving it in a more readable form has advantages 19:33:59 is there a reason not to just make this part of the fuzzer build? 19:34:09 luke-jr: it's not a fuzzer 19:34:16 you can't run it as a fuzzer 19:34:34 (it would immediately fail, as it's not testing random inputs) 19:34:42 sipa: right, I guess disk space when checked out is irrelevant for such sizes 19:34:57 sipa: but can't the 250k be generated at test-time? 19:35:02 luke-jr: it took me days 19:35:11 hmm 19:35:15 (of CPU time) 19:35:23 a special make target? 19:35:29 it's nondeterministic 19:35:36 is https://github.com/bitcoin-core/qa-assets used for test assets? 19:35:42 +1 19:35:54 jnewbery: yes, that's what I meant, the only thing is that it takes an extra step then 19:35:54 luke-jr: to be clear, this is a test that _already_ runs as a functional test, but only for 1 minute 19:36:17 someone who wants to read the test also needs that erpository checked out -- not a problem for the CI at leat 19:36:28 luke-jr: the approach to extract a very-good-coverage unit test from it makes it a bit more accessible and reusable, and gives the same coverage as running the functional test 1000s of times 19:37:05 wumpus: that's reasonable, i guess 19:37:22 skip the test if the json file isn't found 19:37:28 or something like that 19:37:35 sipa: yes, sounds fair to me 19:38:02 * luke-jr glares at boost for not supporting skips still (last I checked) 19:38:24 though there are already some ~250kB json files in the repo, for the tests, I don't think one more is that bad... but let's not make a habit out of it, and also, you're planning to add more data in the future 19:38:35 I like the qa assetts idea. Skipping the test if input not found is also similar to what we do for backwards compatibility tests 19:38:39 luke-jr: "return true;" works great 19:38:44 yes 19:38:55 sipa: except it gives the impression it passed? 19:39:00 No objection to 250kb either, though 19:39:38 luke-jr: "make check" also doesn't run the fuzz tests; does that give the impression they passed? :) 19:39:55 there could be a "notice: qa-assets not found, so large data-driven tests are skipped" 19:39:55 sipa: boost explicitly says the tests pass.. 19:40:14 luke-jr: in aggregate 19:40:20 sipa: individually 19:40:25 sipa: this is a problem for another test already IIRC 19:40:29 ok, I think we've given sipa quite some input on this for now, decision doesn't need to be made in the meeting, only ~20 minutes left, and 1 or 2 topics 19:40:38 #topic AssertLockHeld PRs (ryanofsky) 19:40:51 Debug lockerorder test is a test that passes if skipped, but minor one 19:41:10 For AssertLockHeld PRs, I just wanted to advertise wiki page https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs 19:41:29 If anyone is interested in AssertLockHeld improvements but confused by the multiple PRs, page summarizes them 19:41:57 ryanofsky: thanks for that 19:42:04 ryanofsky: v nice 19:42:06 * sipa is quite overwhelmed by it 19:42:11 yes good to have an overview 19:42:24 Sure, that's all I have for the topic 19:42:57 what would "WeaklyAssertLockHeld" do compared to the normal assert? 19:43:20 indeed! (confusing name) 19:43:49 They both do the same thing at runtime, which is why my preference is to have one assert instead of two 19:44:23 and the difference is that one also does a compile-check (if supported) and the other doesn't? 19:44:25 it sounds really weird to me a lock is held or not :) 19:44:25 But if we can't have one assert, there are cases where the stronger assert isn't accepted at compile time and you need to use the weaker one 19:44:27 btw, one of the clang people suggested that we don't annotate AssertLockHeld() with any compile time attributes and leave it pure run time. 19:44:43 oh, like that 19:45:28 I don't get why one would use Assert* instead of EXCLUSIVE_LOCKS_REQUIRED 19:45:41 outside of perhaps a conditional 19:45:53 it's an assertion that existed way before the lock annotations did 19:46:04 lock annotations also only work in clang 19:46:09 ouch 19:46:11 oh :/ 19:46:16 and can't be used for some of the more complex cases, i assume 19:46:29 luke-jr: runtime asserts work always, compile time _warnings_ - only for clang and if you compile with --enable-werror and if clang does not have bugs etc 19:46:32 luke-jr, infrequently there are cases where the compile can't know EXCLUSIVE_LOCKS_REQUIRED is satisfied and you have to tell it that 19:46:46 vasild: well, they only work in -DDEBUG_LOCKORDER mode, which you don't want to use for production :) 19:46:50 that is what assert is useful for 19:47:15 so they're overlapping but one is not a subset of the other, in terms of what they can detect 19:47:16 the other thing assert is useful for (i don't think very useful) is when compile time checks are unavailable or disabled or buggy 19:47:51 apparently they're clang only so they're often not available 19:48:15 i'm fairly sure most people compile with gcc, at least on linux 19:48:21 right but they run on CI 19:49:05 and if you are compiling with gcc, you have to enable run time checks and execute the code and hope an assert is hit to see any benefit from it 19:49:26 yes 19:49:43 having CI as the sole protection seems uncomfortable to me - it does not run manual tests 19:49:55 developers do on their machines 19:50:14 ideally all tests would exist in the automated form ;) 19:50:37 vasild, compile time checks check correctness at compile time they have no effect on runtime generated code 19:50:45 i wonder how many developers are compiling with DEBUG_LOCKORDER anyway, well probably those that are working on locking changes do 19:50:51 there's 0 benefit to compiling with compiling checks and then running bitcoind locally 19:51:02 wumpus: it is enabled bug --enable-debug 19:51:15 by 19:51:27 yes 19:51:46 "Bitcoin Core developer claims enabling debugging introduces bug" 19:52:08 "fixes the bug by disabling debugging" 19:52:15 src/Makefile:CXXFLAGS = -Wthread-safety-analysis -DDEBUG_LOCKORDER -O1 -ggdb -Wall -Werror=thread-safety-analysis -fsanitize=undefined 19:52:18 apparently I am 19:52:43 I debug-build with clang on some PRs 19:52:55 ok if we still want to discuss torv3, we'll have to switch topics now 19:52:56 mind you, I never use dev code for mainnet 19:53:08 * luke-jr wonders when a 1 hour limit was set in the first place :P 19:53:20 because it's good to keep meetings short 19:53:23 #topic torv2->torv3 transition, schedule, process (jonatack) 19:53:25 Per this Tor ML post https://lists.torproject.org/pipermail/tor-dev/2020-June/014365.html 19:53:43 Tor v2 was deprecated the day before yesterday (September 15, 2020) with 0.4.4.x and will be obsoleted in 0.4.6.x (July 15, 2021) 19:53:52 Tor v2 is expected to be completely disabled in Tor client stable versions on October 15, 2021 19:54:04 previous discussion from the review meeting yesterday: https://bitcoincore.reviews/19845#l-270 19:54:12 is this a network-level change, or just dependency change? 19:54:26 ie, does Tor v2 stop working for old versions too? 19:54:39 a half-dozen of us are running nodes with tor v3 services ATM 19:54:50 luke-jr: i assume it will, due to network infrastructing updating to versions that don't support torv2 anymore 19:54:59 luke-jr: they say torv2 is going to be removed from the source code of Tor 19:54:59 using #19954 / aka PR 19031 19:55:01 https://github.com/bitcoin/bitcoin/issues/19954 | tor: make a TORv3 hidden service instead of TORv2 by vasild · Pull Request #19954 · bitcoin/bitcoin · GitHub 19:55:05 luke-jr: that's not entirely clear to me; but I assume they'll shut down the directory authorities etc for torv2 too 19:55:41 "We will release new Tor client stable versions for all supported series that will disable v2." 19:55:55 on Oct 15 2021 per https://blog.torproject.org/v2-deprecation-timeline 19:56:06 "This effectively means that from today (July 2nd, 2020), the Internet has around 16 months to migrate from v2 to v3 once and for all." 19:56:30 that sounds like torv2 will stop working in oct 2021 19:56:51 they probably realize that if they leave torv2 working, there will be still people using it after 10 years 19:56:54 let's try to get basic (if we can't get all) torv3 support into 0.21.0 19:57:07 wumpus: seems doable 19:57:22 we're ~5 commits away 19:57:26 better to have things prepared in time than to wait for last minute 19:57:34 19845 + 19954 i believe 19:57:42 yea it's not too much anymore 19:58:10 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-16.html#l-243 19:58:24 wumpus: also, ack bip155 changes, as those need to be stable before 0.21 if we want it :) 19:58:34 at first, will nodes gossip both v2 and v3? 19:58:36 sipa: yes 19:58:57 jonatack: i think that would make sense 19:59:04 agree 19:59:22 stopping gossip of torv2 we can consider after Oct 2021 19:59:40 sgtm 19:59:41 maybe it should support the case where tor refuses to create a v2 service, to be future proof 19:59:59 e.g. not make that a fatal error 20:00:36 is anything in torcontrol a fatal error? 20:00:37 in 19954 we only ever try to create torv3 service 20:00:40 atm 19954 only rumours v3? 20:01:08 sipa: not for bitcoind entirely, but any error will stop it from going forward in torcontrol 20:01:26 "atm 19954 only rumours v3?" - no, it would rumor (gossip) both torv2 and torv3 20:01:49 great! 20:02:15 #endmeeting