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