19:00:18 #startmeeting 19:00:18 Meeting started Thu Jun 11 19:00:18 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:18 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:21 hi 19:00:24 hi 19:00:30 hi 19:00:31 hi 19:00:34 #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:34 jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2 19:00:35 hola 19:00:37 hi 19:00:48 hi 19:00:48 hi 19:01:15 hi 19:01:16 hi 19:01:17 hi 19:01:28 hi 19:01:33 hi 19:01:36 hi 19:01:57 hi 19:02:13 hi 19:02:15 hi 19:02:54 hi 19:02:55 one proposed topic: Bump minimum required Clang version to 3.5 (hebasto) 19:03:19 hebasto: links? 19:03:21 As I posted in the thread, debian oldoldstable is on clang-4 19:03:22 (you can propose topics between meetings with #proposedmeetingtopic, or now before the meeting) 19:03:42 Not sure why this is so controversial 19:03:47 please wait with discussion about the topic until the topic 19:03:50 sorry 19:03:56 sry 19:04:10 #proposedmeetingtopic feature detection API 19:04:34 yeah you don't have to use that tag inside the meeting we see it :) 19:04:56 (i think it helps for grepping logs) 19:04:57 #topic High priority for review 19:05:16 Could I nominate #18991 for blockers? I think it's a big improvement for p2p privacy, and it would also unlock me from further work on AddrMan. 19:05:19 https://github.com/bitcoin/bitcoin/issues/18991 | Cache responses to GETADDR to prevent topology leaks by naumenkogs · Pull Request #18991 · bitcoin/bitcoin · GitHub 19:05:24 13(!) blockers, 0 bugfixes, 3 items chasing ACK https://github.com/bitcoin/bitcoin/projects/8 19:05:40 Oh i see.... 19:06:02 if anyone wants to see forward motion on assumeutxo, #18637 is the one to review 19:06:05 https://github.com/bitcoin/bitcoin/issues/18637 | coins: allow cache resize after init by jamesob · Pull Request #18637 · bitcoin/bitcoin · GitHub 19:06:22 I'd like to add #19071 19:06:24 https://github.com/bitcoin/bitcoin/issues/19071 | doc: Separate repository for the gui by MarcoFalke · Pull Request #19071 · bitcoin/bitcoin · GitHub 19:06:26 Not sure what is left there 19:06:40 gleb: added 19:07:25 Mine is not a blocker, so feel free to ignore ;) 19:07:46 thank you! We need couple more eyes on #18991. It's a little code with big impact, not much prior knowledge required but it would expose you to the logic of addr relay! So really nice to review please come :) 19:07:49 https://github.com/bitcoin/bitcoin/issues/18991 | Cache responses to GETADDR to prevent topology leaks by naumenkogs · Pull Request #18991 · bitcoin/bitcoin · GitHub 19:07:56 18637 is already in there 19:08:59 19071 added 19:09:04 wumupus: yep just additional heads-up since people seem to ask about where the project's at from time to time 19:09:05 #proposedmeetingtopic Add Really Really High Priority For Review project 19:09:12 haha 19:09:18 oh noooo 19:09:52 sky high elite priority for review project 19:10:19 Jokes aside, maybe would be useful to re-iterate on the usability/usefulness of the high-prio stuff. Maybe PRs should expire from there or somehting. 19:10:57 that's interesting. probably no way of doing automatic expiry with github though 19:11:04 maybe drahtbot? 19:11:16 I found that putting something in high prio has no effect on review activity 19:11:23 I don't think we need automatic expiry, let's just try to pay attention ourselves 19:11:26 I don't think it makes sense to automate such a low-latency thing. 19:11:38 Or rather low-bandwidth I should say. 19:11:39 MarcoFalke: well at least I pay more attention to it, for what it's worth 19:11:57 I just wanted to make sure wumpus and co are comfortable with evicting things from tht list. 19:11:59 although, the higher the number of things on there the less it matters I'd definiltly agree 19:12:08 jnewbery: good point. when the list is short, i follow it. when it's long, i make my own list. 19:12:29 although review blocked, I am happy having a project 19:12:29 I guess it's good that every contributor can propose something that's most important for them now 19:12:52 So maybe letting more contributors manage a project is better than the high prior for review as it is better to sort work 19:13:02 I'm not sure that needs any bots or automatic expiry to be honest 19:13:09 Agree 19:13:11 Especially if the important-ness is blocking related, projects can help hilight the follow on work 19:13:23 wumpus: I agree. At the very least, it's useful for contributors to signal "this is _my_ highest priority PR right now" 19:13:31 a bit more projects seems good 19:13:31 jnewbery: exactly 19:13:48 jnewbery: only when people look at the list :) 19:13:49 but you shouldn't assume that anyone else cares :) 19:14:04 if no one cares, no one cares, nothing to do about that 19:14:47 I think the length is ok. Reviewers just have to take a pick and often not all of them are relevant for oneself. 19:15:02 it's, sometimes, kind of absurd sometimes how much money sails on this project compared to how little attention review gets, but it's only one of the many mysteries 19:15:45 wumpus: you need to tokenize review for that 19:15:52 For some high prio stuff I am unqualfied to review because I am lacking the background, so even forcing me to review wouldn't yield anything better than a "Concept ACK" 19:16:10 MarcoFalke: +1 19:16:13 yes, I don't have any solutions either, that's what I meant 19:16:41 maybe lists per domain: wallet p2p mining consensus etc with a maintainer would make it efficient, i dunno. 19:16:49 wumpus: I expect there's more effort and time going into review on this project than at any point in the past 19:16:59 jnewbery: I'm happy to hear that 19:17:18 gleb: nah… 19:17:28 you can already sort issues per label, you know 19:17:31 and PRs 19:17:59 Right. 19:18:07 I don't think high priority for review is suppsoed to become that long that it needs to be sorted into categories as well 19:18:13 but who knows :-) 19:18:36 The number of open pull requests keeps growing ... 19:18:40 I think one problem is the huge number of different concerns and aspects in one project 19:18:46 but we've been over that 19:19:11 #topic Bump minimum required Clang version to 3.5 (hebasto) 19:19:15 hi 19:19:18 negative capabilities in clang thread safety analysis are a great tool to prevent deadlocks 19:19:36 I agree with it remaining ad hoc. Ideally people would not put non-blocking, non-completely review-ready PRs in the list, and pull their own PRs off if less high-priority than others. 19:19:40 personally, I think we should hold off doing any compiler version bumps until C++17 19:19:41 #19249 19:19:43 https://github.com/bitcoin/bitcoin/issues/19249 | Add means to handle negative capabilities in the Clang Thread Safety annotations by hebasto · Pull Request #19249 · bitcoin/bitcoin · GitHub 19:19:59 I think I misunderstood what the annotation do, so I will need to re-review the changes, but if they are useful, we shouldn't hold them back for 4 months. 19:20:01 example of usage: #19238 19:20:03 https://github.com/bitcoin/bitcoin/issues/19238 | refactor: Replace RecursiveMutex with Mutex in CAddrMan by hebasto · Pull Request #19238 · bitcoin/bitcoin · GitHub 19:20:31 example of error catching #19251 19:20:33 https://github.com/bitcoin/bitcoin/issues/19251 | [Do Not Merge] ci: Temporary Test Case for negative capabilities in the Clang Thread Safety annotations by hebasto · Pull Request #19251 · bitcoin/bitcoin · GitHub 19:20:34 no distro is using this ancient version of clang, and gcc is used by default to compile anyway 19:20:35 I don't think requiring *everyone* to use a newer compiler just for some annotations and checking is that great 19:20:44 wumpus: i have no strong opinion either way; bumping to (already such an old) new clang seems fairly low friction 19:20:48 it requires clang 3.5+ 19:21:02 wumpus: Is any os using pre-3.5 clang? 19:21:06 if a few peopel compile with those annotations it aleady helps 19:21:08 https://repology.org/project/clang/versions 19:21:10 it doesn't have to be everyone 19:21:14 but if we're going to do a big bump soon anyway, that's ok too 19:21:15 oldoldstable debian is on clang-4 19:21:43 clang 3.5 is from 2015 19:21:44 I'm not necessarily opposed to this specific bump, but I think we're spending too much time doing small bumps 19:22:20 I'd like to make an "ambitious" bump for C++17, it's a good rationale 19:22:30 This is merely a documentation change. It should have no effect in practice 19:22:41 agree 19:23:11 ok... 19:23:18 I don't care strongly just update it then 19:23:32 please don't come back next week that you need another bump though ;) 19:23:32 Personal note that I find the safety annotations really confusing 19:23:47 which way? 19:24:24 EXCLUSIVE_LOCKS_REQUIRED(!mutex) looks good and works well 19:24:33 if the change doesn't make it in because it is not worthwhile, clang won't be bumped 19:25:00 I think it's worthwhile to check 19:25:05 for compilers that support it 19:25:13 I don't quite know how to use them for some of the tasks that I would like to prove safe :) Would be interested to learn more; I have a couple examples where we over-lock and I'm not sure how to use the exclusive locks required negation to accomplish that 19:25:16 I do not think it's a worthwhile reason to drop compilers that don't support it 19:25:17 We can take it offline though 19:25:42 then again I'm not fanatic in supporting ancient stuff either 19:26:03 if oldoldstable is already 4.0, let's require 4.09 19:26:13 4.0* 19:26:39 Anyway, let's first check if the annotations make sense 19:27:02 examples are here #19238 19:27:04 https://github.com/bitcoin/bitcoin/issues/19238 | refactor: Replace RecursiveMutex with Mutex in CAddrMan by hebasto · Pull Request #19238 · bitcoin/bitcoin · GitHub 19:27:24 but my point was, we *know* we're going to drop support for old compilers with C++17, so all time spend discussing bumps before that is probably wasted 19:27:56 so 3.5 or 4.0 ? 19:28:15 MarcoFalke: yes, let's be sure of that first 19:28:32 ok 19:28:55 #topic Feature detection API (jeremyrubin) 19:29:42 Not too much; but there was some discussion on this w.r.t. people building on top of core 19:30:01 That detecting which RPCs are available requires firing off a batch of RPCs and seeing what error codes come back 19:30:08 This is... suboptimal 19:30:17 jeremyrubin: Only with whitelists 19:30:23 MarcoFalke: no, all the time 19:30:40 BTCPayServer does this to detect features across versions on startup 19:30:41 doesn't the help RPC list all methods? 19:30:53 Yeah, what jnewbery said ^ 19:31:29 But there's also a need to know what semantics/arguments are available and if the semantic has changed at all 19:31:37 well you only have to do it once 19:31:52 jeremyrubin: The RPC version number is used for that 19:31:54 it's only suboptimal if you have to do it all the time, in which case, you're probably doing something wrong 19:31:56 or you can parse the help 19:32:18 yes, the RPC version number would be the thing to check for that 19:32:27 Yeah. I think it's more that we should expose some nicer way of doing this and filtering across the whitelists. 19:32:32 i wouldn't object to an RPC that returns all supported RPCs in JSON form 19:32:39 if that's what we're talking about 19:32:40 this seems to be part of integration testing if BTCPayServer or whatever starts using a new version 19:32:47 we already have that information ready anyway 19:33:18 Also the whitelists are non-interpreted, so they aren't that useful because you then need to replicate the whitelist algorithm locally from it 19:33:32 would be nice to have a machine-interpratable version of the RPC help in general 19:33:40 I think there was also a desire to have per-RPC versioning 19:33:44 this is an idea that has been around since 2012 or so 19:33:44 I am working on that 19:33:50 (the machine readable help) 19:33:53 Which IDK if it's super useful, but it was requested 19:34:08 machine-readable help would just be something like an OpenAPI spec? 19:34:14 It is nice to know if theres a breaking RPC change that you are aware on startup that there was a change 19:34:18 per-rpc versioning? oh no, please don't make RPC versioning any more of a hassle as it is already 19:34:50 wumpus the rpc is versioned on the major version of Bitcoin Core https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md 19:34:55 don't shoot the messenger. If it's an issue I'd rather just rename RPCs rpc_name_v1 when theres a change 19:35:04 I don't get it, what does it pprovide on top of the global version number 19:35:20 oh, it is the same as the global version number 19:35:25 in general, only major bitcoin core versions introduce breaking RPC changes 19:35:57 (that's how it's supposed to be, at least, if not, I guess that's a bug in itself) 19:35:58 dongcarl: It can be anything, even just `help(format="json")` as an RPC 19:36:08 I think it's that a major version doesn't break *everything* 19:36:12 wumpus: Correct, that is what the doc says 19:36:16 So it's if you need to upgrade all your logic or not 19:36:34 before upgrading to a new major version you need to check the release notes for all the calls you're using in your software 19:36:51 Well the issue is that's not a developer issue 19:36:55 that's a userland issue 19:37:02 unless the user is bundling the node 19:37:06 *developer 19:37:14 same for client software I guess 19:38:09 Yeah so if a user starts with an older node, BTCPayServer tries to be compatible. And every developer could write their own maps of breaking changes and what works or doesn't 19:38:16 what would per-RPC versioning look like in any case? like, instead of calling sendtoaddress, you'd call sendtoaddress/v4 ? 19:38:22 But i think the request is to handle it inside of core 19:38:32 Ah, I think it would be in the get_avail_rpcs 19:38:47 You would get a current version, and a broken at version 19:38:55 there was an idea like that at one point I guess 19:39:16 so if you're expecting something v3 compliant, but it tells you broken at v4, then you can alert the user to upgrade 19:39:22 e.g. have an endpoint /v2 that would expose calls with new arguments 19:39:37 yeah. I don't have a proposal for this BTW. 19:39:47 but, to be honest, nothing is that organized, you really need to pay attention to the major bitcoin core release in practice 19:40:09 That's why I think it makes a good meeting topic as it's something downstream users are complaining about, and would be good to make life easier for them. 19:40:46 I think a JSON of rpcs and available args would be good. 19:41:07 I still don't see how that is better than just looking at the major version. We don't need to expose release notes information on the RPC interface. 19:41:38 i don't think the current_version and broken_at_version idea is crazy 19:41:38 I think we've generally been pretty responsible with maintaining RPC compatibility. The deprecatedrpc functionality gives clients plenty of warning when anything important changes. 19:41:56 yes 19:42:13 I'm not sure for what RPCs this is a practical problem 19:42:59 I think the reason for putting thought into it is if we're going to do *some* feature detection, we want it to be sufficiently future proof as then future feature detection would be broken 19:43:06 if we change the feature detection API 19:43:22 having some examples of what RPCs have caused issues in the past would be good to know, indeed 19:43:31 Sounds like if this is a downstream complaint we need to look at the specific cases and see what could be done that would solve a majority of them. jeremyrubin Are there links? 19:43:37 perhaps all we need to do is be more careful about breaking compatbility 19:43:49 https://github.com/bitcoin/bitcoin/issues/12248 19:43:57 sipa: especially in cases that can pass silently 19:44:14 https://github.com/bitcoin/bitcoin/pull/19117 19:44:18 maybe renaming a RPC if its fields change isn't that bad an idea 19:44:51 jeremyrubin: Are there links to the downstream convo? 19:44:55 though in general we've been careful to keep the same patern for arguments, e.g. we *still* have a dummy argument for getbalance 19:45:12 I think the main thing I broke is batching API when using whitelists. unaware of links for their internal issue on that 19:45:52 Because if you're using whitelists, the method of feature discovery (calling all required RPCS) fails 19:45:59 I'm still confused about whitelisting tbh 19:46:12 Which calling all RPCs to discover the features is... bad. Because RPCs can do things 19:46:28 I don't think RPC whitelisting should have been something that ended up in bitcoind 19:46:31 been anyhow 19:47:18 some very application-specific security requirements are finding their way in 19:47:41 all of this stuff seems like it should be the responsibility of a proxy 19:47:43 and this has nothing to do with differences between versions 19:47:46 jnewbery: yes 19:47:46 dongcarl: here's the issue https://github.com/bitcoin/bitcoin/issues/19087 19:47:55 But then different rpcusers shouldn't be in bitcoind either? 19:48:21 I do think that we could rip out all authentication for RPCusers 19:48:35 Just have one authpair and the proxy can deal with users 19:48:53 It's a lot of complexity and if the answer is to proxy it just make Bitcoind not listen by default & require SSH auth'd proxy to connect 19:49:01 MarcoFalke: I'm not for reverting it, just, it's not a no-holds-barred pretext to introduce all kinds of per-RPC auth mechanisms into bitcoind 19:49:47 I think that it gives the wrong message, that unprivileged users are an expected thing. there's already a large number of people running with RPC bound to public interfaces as it is. 19:50:02 wait, no, don't bind RPC to public interfaces 19:50:08 I am not for reverting it either. Just saying that we already have more than the minimal required functionality in core 19:50:21 this kind of people that want this are enterprises I guess, with *very specific* security and auditing requirements 19:50:40 by all means it allows you to have granular control of the permissions from your service, but it is easily misinterpreted. 19:51:01 exposing things on the public internet is just plain dumb 19:51:25 i think public probably means internal-public 19:51:31 the only interface that bitcoind provides that (hopefully) is internet-hardened is P2P 19:51:39 anyhow, any other topics? 19:51:42 It looks to me like 19087 is the result of two features interacting badly (batching and whitelists). Adding more complexity and another feature (versioning) doesn't seem like the correct remedy. 19:52:04 So the need for versioning exists outside of the conflict there 19:52:16 The core issue is that people are struggling to do feature detection 19:52:23 And are calling a list of all RPCs to do that 19:52:30 So we should have a better paradigm for that 19:52:45 imo: look at the major version number of bitcoin core 19:52:51 What about the pull request that returns the RPC methods for the current user? 19:52:52 if you're not sure, warn the user 19:53:07 MarcoFalke: it returns the whitelist, not the list of RPC methods 19:53:14 nothing will be enough to machine-represent the subtleties of differences between versions 19:53:25 And the whitelist can contain things that aren't currently defined RPCs 19:53:46 whitelist has nothing to do with this 19:53:46 Hence the issue being, what people want is a detect-avaialble-features API 19:53:53 why can't the whitelist be sanitized before returning it? 19:54:07 Then it's a detect available features API 19:54:23 Which is what the issue is about; but if we're doing that it makes sense to think it through 19:54:37 And listen to users on what sort of feature detection things they need/want 19:54:46 And RPC versioning came up as a request 19:54:48 jnewbery: it definitely seems like digging in deeper after making a mistake 19:55:23 I'm OK with just returning a filtered RPC whitelist tho, it would work, but maybe we need a v2 feature detection later which I'd want to avoid 19:55:33 to be clear I applaud attempts to have machine-readable documentation for RPCs like MarcoFalke is working on 19:55:38 jeremyrubin: wasn't there a really good external site once that documented all the RPC API versions in detail? 19:56:07 and if that gives a list of available RPC calls, sure, that could be useful 19:56:08 jonatack: bitcoincore.org :) 19:56:20 https://chainquery.com/bitcoin-cli probably. 19:56:31 https://bitcoincore.org/en/doc/ 19:57:36 Anyways I agree largely it doesn't belong in core; things like the wallet also probably shouldn't be there 19:57:52 yes 19:57:55 yes, and I knew another one that was outstanding, but don't recall what it was 19:57:58 too much score creep 19:58:04 scope creep 19:58:17 But it's just making due with helping users/devs write secure software 19:58:22 *making do 19:58:45 If someone has a good proxy implementation that can be put into a sub-process maybe that would help 19:58:49 I think a layered approach is a better way to make secure software 20:00:02 *DONG* 20:00:06 #endmeeting