< fanquake>
Flat out confusing myself with the GH UI, but managed to get it sorted
< luke-jr>
more like it managed to get you sorted :P
< fanquake>
heh. Managed to re-request a review from myself
< luke-jr>
XD
< sipa>
fanquake is now known as aaefknqu
< luke-jr>
lol
< bitcoin-git>
[bitcoin] luke-jr opened pull request #19242: Add -uaappend option to append a literal string to user agent (master...uaappend) https://github.com/bitcoin/bitcoin/pull/19242
< aj>
luke-jr: shouldn't uaappends be separated by "; " or something?
< luke-jr>
aj: '/', but I'm assuming the using application knows that
< luke-jr>
-uaappend 'foo:1.0/'
< luke-jr>
-uaappend 'foo:1.0/bar:9999/'
< aj>
luke-jr: i would have expected use more like -uappend=foo:1.0 -uappend=bar:9999
< aj>
apparently the double-a is hard for me
< luke-jr>
aj: but then you can't reliably predict order
< aj>
luke-jr: sure, but don't see why order matters much? (and if it does, then allowing many uaappend's doesn't seem right?)
< luke-jr>
aj: maybe it does, maybe not *shrug*
< luke-jr>
we don't act on it, but others might
< luke-jr>
probably shouldn't I guess, but it's also nice for consistency :x
< luke-jr>
I guess always ensuring there's a / at the end makes sense
< bitcoin-git>
[bitcoin] hebasto closed pull request #19238: refactor: Replace RecursiveMutex with Mutex in CAddrMan (master...200610-addrman-mx) https://github.com/bitcoin/bitcoin/pull/19238
< bitcoin-git>
[bitcoin] hebasto reopened pull request #19238: refactor: Replace RecursiveMutex with Mutex in CAddrMan (master...200610-addrman-mx) https://github.com/bitcoin/bitcoin/pull/19238
< bitcoin-git>
[bitcoin] luke-jr opened pull request #19243: banman: Limit resources consumed by misbehaving node deprioitisation (master...misbehaving_limit) https://github.com/bitcoin/bitcoin/pull/19243
< luke-jr>
is it just me, or do we currently have a bug where a misbehaving node makes itself immune to a real/user-set ban less than 24 hours long?
< luke-jr>
also, is there any realistic way to write functional tests for many misbehaving nodes? XD
< bitcoin-git>
[bitcoin] hebasto opened pull request #19249: Add means to handle negative capabilities in the Clang Thread Safety annotations (master...200611-nc) https://github.com/bitcoin/bitcoin/pull/19249
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19250: wallet: [move-only bugfix] Make RPC help compile-time static (master...2006-rpcWalletHelp) https://github.com/bitcoin/bitcoin/pull/19250
< hebasto>
please remind how to propose a meeting topic in advance?
< achow101>
hebasto: using #proposedmeetingtopic
< hebasto>
achow101: thanks
< hebasto>
#proposedmeetingtopic Bump minimum required Clang version to 3.5
< hebasto>
^ in the context of #19249
< gribble>
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
< bitcoin-git>
[bitcoin] hebasto opened pull request #19251: [Do Not Merge] ci: Temporary Test Case for negative capabilities in the Clang Thread Safety annotations (master...200611-dnm-test) https://github.com/bitcoin/bitcoin/pull/19251
< wumpus>
I think we should bump compiler versions after C++17, not bother witht it sooner
< wumpus>
no need to complicate this with multiple bumps
< wumpus>
but note that #16684 has no mention of clang versions
< hebasto>
wumpus: yes, C++17 is near. Do you mean 19249 should be dropped for now, or could move on without clang version bumping?
< wumpus>
hebasto: if you can make it optional
< wumpus>
I think we should let the minimum version also depend on what is generally available in Linux distros' to make sure we can test against it easily
< hebasto>
that is the problem: make it optional will clutter the code
< wumpus>
hebasto: then I'd say delay it until a bump is needed for c++17
< wumpus>
one proposed topic: Bump minimum required Clang version to 3.5 (hebasto)
< dongcarl>
hebasto: links?
< MarcoFalke>
As I posted in the thread, debian oldoldstable is on clang-4
< wumpus>
(you can propose topics between meetings with #proposedmeetingtopic, or now before the meeting)
< MarcoFalke>
Not sure why this is so controversial
< wumpus>
please wait with discussion about the topic until the topic
< MarcoFalke>
sorry
< dongcarl>
sry
< jeremyrubin>
#proposedmeetingtopic feature detection API
< wumpus>
yeah you don't have to use that tag inside the meeting we see it :)
< jeremyrubin>
(i think it helps for grepping logs)
< wumpus>
#topic High priority for review
< gleb>
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.
< MarcoFalke>
Mine is not a blocker, so feel free to ignore ;)
< gleb>
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 :)
< jamesob>
wumupus: yep just additional heads-up since people seem to ask about where the project's at from time to time
< jnewbery>
#proposedmeetingtopic Add Really Really High Priority For Review project
< jamesob>
haha
< wumpus>
oh noooo
< wumpus>
sky high elite priority for review project
< gleb>
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.
< jamesob>
that's interesting. probably no way of doing automatic expiry with github though
< jamesob>
maybe drahtbot?
< MarcoFalke>
I found that putting something in high prio has no effect on review activity
< wumpus>
I don't think we need automatic expiry, let's just try to pay attention ourselves
< gleb>
I don't think it makes sense to automate such a low-latency thing.
< gleb>
Or rather low-bandwidth I should say.
< wumpus>
MarcoFalke: well at least I pay more attention to it, for what it's worth
< gleb>
I just wanted to make sure wumpus and co are comfortable with evicting things from tht list.
< wumpus>
although, the higher the number of things on there the less it matters I'd definiltly agree
< jonatack>
jnewbery: good point. when the list is short, i follow it. when it's long, i make my own list.
< jeremyrubin>
although review blocked, I am happy having a project
< wumpus>
I guess it's good that every contributor can propose something that's most important for them now
< jeremyrubin>
So maybe letting more contributors manage a project is better than the high prior for review as it is better to sort work
< wumpus>
I'm not sure that needs any bots or automatic expiry to be honest
< MarcoFalke>
Agree
< jeremyrubin>
Especially if the important-ness is blocking related, projects can help hilight the follow on work
< jnewbery>
wumpus: I agree. At the very least, it's useful for contributors to signal "this is _my_ highest priority PR right now"
< hebasto>
a bit more projects seems good
< wumpus>
jnewbery: exactly
< gleb>
jnewbery: only when people look at the list :)
< jnewbery>
but you shouldn't assume that anyone else cares :)
< wumpus>
if no one cares, no one cares, nothing to do about that
< fjahr>
I think the length is ok. Reviewers just have to take a pick and often not all of them are relevant for oneself.
< wumpus>
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
< jeremyrubin>
wumpus: you need to tokenize review for that
< MarcoFalke>
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"
< jeremyrubin>
MarcoFalke: +1
< wumpus>
yes, I don't have any solutions either, that's what I meant
< gleb>
maybe lists per domain: wallet p2p mining consensus etc with a maintainer would make it efficient, i dunno.
< jnewbery>
wumpus: I expect there's more effort and time going into review on this project than at any point in the past
< wumpus>
jnewbery: I'm happy to hear that
< wumpus>
gleb: nah…
< wumpus>
you can already sort issues per label, you know
< wumpus>
and PRs
< gleb>
Right.
< wumpus>
I don't think high priority for review is suppsoed to become that long that it needs to be sorted into categories as well
< wumpus>
but who knows :-)
< MarcoFalke>
The number of open pull requests keeps growing ...
< wumpus>
I think one problem is the huge number of different concerns and aspects in one project
< wumpus>
but we've been over that
< wumpus>
#topic Bump minimum required Clang version to 3.5 (hebasto)
< hebasto>
hi
< hebasto>
negative capabilities in clang thread safety analysis are a great tool to prevent deadlocks
< jonatack>
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.
< wumpus>
personally, I think we should hold off doing any compiler version bumps until C++17
< hebasto>
#19249
< gribble>
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
< MarcoFalke>
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.
< gribble>
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
< MarcoFalke>
no distro is using this ancient version of clang, and gcc is used by default to compile anyway
< wumpus>
I don't think requiring *everyone* to use a newer compiler just for some annotations and checking is that great
< sipa>
wumpus: i have no strong opinion either way; bumping to (already such an old) new clang seems fairly low friction
< hebasto>
it requires clang 3.5+
< MarcoFalke>
wumpus: Is any os using pre-3.5 clang?
< wumpus>
if a few peopel compile with those annotations it aleady helps
< sipa>
but if we're going to do a big bump soon anyway, that's ok too
< MarcoFalke>
oldoldstable debian is on clang-4
< sipa>
clang 3.5 is from 2015
< wumpus>
I'm not necessarily opposed to this specific bump, but I think we're spending too much time doing small bumps
< wumpus>
I'd like to make an "ambitious" bump for C++17, it's a good rationale
< MarcoFalke>
This is merely a documentation change. It should have no effect in practice
< hebasto>
agree
< wumpus>
ok...
< wumpus>
I don't care strongly just update it then
< wumpus>
please don't come back next week that you need another bump though ;)
< jeremyrubin>
Personal note that I find the safety annotations really confusing
< hebasto>
which way?
< hebasto>
EXCLUSIVE_LOCKS_REQUIRED(!mutex) looks good and works well
< MarcoFalke>
if the change doesn't make it in because it is not worthwhile, clang won't be bumped
< wumpus>
I think it's worthwhile to check
< wumpus>
for compilers that support it
< jeremyrubin>
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
< wumpus>
I do not think it's a worthwhile reason to drop compilers that don't support it
< jeremyrubin>
We can take it offline though
< wumpus>
then again I'm not fanatic in supporting ancient stuff either
< wumpus>
if oldoldstable is already 4.0, let's require 4.09
< wumpus>
4.0*
< MarcoFalke>
Anyway, let's first check if the annotations make sense
< wumpus>
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
< hebasto>
so 3.5 or 4.0 ?
< wumpus>
MarcoFalke: yes, let's be sure of that first
< hebasto>
ok
< wumpus>
#topic Feature detection API (jeremyrubin)
< jeremyrubin>
Not too much; but there was some discussion on this w.r.t. people building on top of core
< jeremyrubin>
That detecting which RPCs are available requires firing off a batch of RPCs and seeing what error codes come back
< jeremyrubin>
This is... suboptimal
< MarcoFalke>
jeremyrubin: Only with whitelists
< jeremyrubin>
MarcoFalke: no, all the time
< jeremyrubin>
BTCPayServer does this to detect features across versions on startup
< jnewbery>
doesn't the help RPC list all methods?
< MarcoFalke>
Yeah, what jnewbery said ^
< jeremyrubin>
But there's also a need to know what semantics/arguments are available and if the semantic has changed at all
< wumpus>
well you only have to do it once
< MarcoFalke>
jeremyrubin: The RPC version number is used for that
< wumpus>
it's only suboptimal if you have to do it all the time, in which case, you're probably doing something wrong
< MarcoFalke>
or you can parse the help
< wumpus>
yes, the RPC version number would be the thing to check for that
< jeremyrubin>
Yeah. I think it's more that we should expose some nicer way of doing this and filtering across the whitelists.
< sipa>
i wouldn't object to an RPC that returns all supported RPCs in JSON form
< sipa>
if that's what we're talking about
< jnewbery>
this seems to be part of integration testing if BTCPayServer or whatever starts using a new version
< sipa>
we already have that information ready anyway
< jeremyrubin>
Also the whitelists are non-interpreted, so they aren't that useful because you then need to replicate the whitelist algorithm locally from it
< wumpus>
would be nice to have a machine-interpratable version of the RPC help in general
< jeremyrubin>
I think there was also a desire to have per-RPC versioning
< wumpus>
this is an idea that has been around since 2012 or so
< MarcoFalke>
I am working on that
< MarcoFalke>
(the machine readable help)
< jeremyrubin>
Which IDK if it's super useful, but it was requested
< dongcarl>
machine-readable help would just be something like an OpenAPI spec?
< jeremyrubin>
It is nice to know if theres a breaking RPC change that you are aware on startup that there was a change
< wumpus>
per-rpc versioning? oh no, please don't make RPC versioning any more of a hassle as it is already
< jeremyrubin>
don't shoot the messenger. If it's an issue I'd rather just rename RPCs rpc_name_v1 when theres a change
< wumpus>
I don't get it, what does it pprovide on top of the global version number
< MarcoFalke>
oh, it is the same as the global version number
< wumpus>
in general, only major bitcoin core versions introduce breaking RPC changes
< wumpus>
(that's how it's supposed to be, at least, if not, I guess that's a bug in itself)
< MarcoFalke>
dongcarl: It can be anything, even just `help(format="json")` as an RPC
< jeremyrubin>
I think it's that a major version doesn't break *everything*
< MarcoFalke>
wumpus: Correct, that is what the doc says
< jeremyrubin>
So it's if you need to upgrade all your logic or not
< wumpus>
before upgrading to a new major version you need to check the release notes for all the calls you're using in your software
< jeremyrubin>
Well the issue is that's not a developer issue
< jeremyrubin>
that's a userland issue
< jeremyrubin>
unless the user is bundling the node
< jeremyrubin>
*developer
< wumpus>
same for client software I guess
< jeremyrubin>
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
< wumpus>
what would per-RPC versioning look like in any case? like, instead of calling sendtoaddress, you'd call sendtoaddress/v4 ?
< jeremyrubin>
But i think the request is to handle it inside of core
< jeremyrubin>
Ah, I think it would be in the get_avail_rpcs
< jeremyrubin>
You would get a current version, and a broken at version
< wumpus>
there was an idea like that at one point I guess
< jeremyrubin>
so if you're expecting something v3 compliant, but it tells you broken at v4, then you can alert the user to upgrade
< wumpus>
e.g. have an endpoint /v2 that would expose calls with new arguments
< jeremyrubin>
yeah. I don't have a proposal for this BTW.
< wumpus>
but, to be honest, nothing is that organized, you really need to pay attention to the major bitcoin core release in practice
< jeremyrubin>
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.
< jeremyrubin>
I think a JSON of rpcs and available args would be good.
< wumpus>
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.
< sipa>
i don't think the current_version and broken_at_version idea is crazy
< jnewbery>
I think we've generally been pretty responsible with maintaining RPC compatibility. The deprecatedrpc functionality gives clients plenty of warning when anything important changes.
< wumpus>
yes
< wumpus>
I'm not sure for what RPCs this is a practical problem
< jeremyrubin>
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
< jeremyrubin>
if we change the feature detection API
< sipa>
having some examples of what RPCs have caused issues in the past would be good to know, indeed
< dongcarl>
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?
< sipa>
perhaps all we need to do is be more careful about breaking compatbility
< MarcoFalke>
But then different rpcusers shouldn't be in bitcoind either?
< jeremyrubin>
I do think that we could rip out all authentication for RPCusers
< MarcoFalke>
Just have one authpair and the proxy can deal with users
< jeremyrubin>
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
< wumpus>
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
< yevaud>
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.
< wumpus>
wait, no, don't bind RPC to public interfaces
< MarcoFalke>
I am not for reverting it either. Just saying that we already have more than the minimal required functionality in core
< wumpus>
this kind of people that want this are enterprises I guess, with *very specific* security and auditing requirements
< yevaud>
by all means it allows you to have granular control of the permissions from your service, but it is easily misinterpreted.
< wumpus>
exposing things on the public internet is just plain dumb
< jeremyrubin>
i think public probably means internal-public
< wumpus>
the only interface that bitcoind provides that (hopefully) is internet-hardened is P2P
< wumpus>
anyhow, any other topics?
< jnewbery>
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.
< jeremyrubin>
So the need for versioning exists outside of the conflict there
< jeremyrubin>
The core issue is that people are struggling to do feature detection
< jeremyrubin>
And are calling a list of all RPCs to do that
< jeremyrubin>
So we should have a better paradigm for that
< wumpus>
imo: look at the major version number of bitcoin core
< MarcoFalke>
What about the pull request that returns the RPC methods for the current user?
< wumpus>
if you're not sure, warn the user
< jeremyrubin>
MarcoFalke: it returns the whitelist, not the list of RPC methods
< wumpus>
nothing will be enough to machine-represent the subtleties of differences between versions
< jeremyrubin>
And the whitelist can contain things that aren't currently defined RPCs
< wumpus>
whitelist has nothing to do with this
< jeremyrubin>
Hence the issue being, what people want is a detect-avaialble-features API
< MarcoFalke>
why can't the whitelist be sanitized before returning it?
< jeremyrubin>
Then it's a detect available features API
< jeremyrubin>
Which is what the issue is about; but if we're doing that it makes sense to think it through
< jeremyrubin>
And listen to users on what sort of feature detection things they need/want
< jeremyrubin>
And RPC versioning came up as a request
< wumpus>
jnewbery: it definitely seems like digging in deeper after making a mistake
< jeremyrubin>
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
< wumpus>
to be clear I applaud attempts to have machine-readable documentation for RPCs like MarcoFalke is working on
< jonatack>
jeremyrubin: wasn't there a really good external site once that documented all the RPC API versions in detail?
< wumpus>
and if that gives a list of available RPC calls, sure, that could be useful
< dongcarl>
Perhaps a good topic for some day where there aren't many topics is: what things can we remove from bitcoin-core that would have a net-positive impact?
< dongcarl>
sipa: yes? :P
< hebasto>
jeremyrubin: "I have a couple examples where we over-lock..." mind sharing?\
< jnewbery>
please don't remove sipa
< wumpus>
dongcarl: I'm just trying to prevent adding too many things
< aj>
jnewbery: but sipa's something that has a net-positive impact, so fits dongcarl's requirements
< jeremyrubin>
dongcarl: russ has a great way of doing that but it requires adding something big :p
< dongcarl>
:brainfried:
< sipa>
*floink*
< wumpus>
we have a kitchen_sink.cpp now, that should be enough, stop adding things please xD
< sipa>
wut?
< wumpus>
./src/test/fuzz/kitchen_sink.cpp
< achow101>
dongcarl: deep fried brains?
< jeremyrubin>
you've been removed sipa
< jamesob>
> *DONG*
< jamesob>
didn't know there was a gong in here
< sipa>
hmm does that not need some kind of voting algorithm?
< jeremyrubin>
I like MarcoFalke's suggestion of completely getting rid of credentials in core
< sipa>
"you are the weakest link, bye bye" style
< dongcarl>
jeremyrubin: A proxy could be maintained in a separate repo
< wumpus>
I don't think it's worth getting rid of now
< jeremyrubin>
I think you probably do want it to be... shipped with and started by 'bitcoind' tho
< MarcoFalke>
Make it opt in for now -use_rpcusers_that_is_going_to_be_removed_in_2025=1
< MarcoFalke>
then remove
< wumpus>
checking against 3 credentials instead of 1, is not that much overhead
< jeremyrubin>
I kind of like a model where bitcoind is a compatible set of processes that run
< sipa>
i think getting rid of the auth mechanism is too big a breaking change
< wumpus>
yes
< yevaud>
"a proxy" can just be nginx, except for the rpccookie bit.
< jeremyrubin>
and bitcoin-node is the just network process
< jeremyrubin>
I think it doesn't have to be breaking at all
< sipa>
yes, dreaming is nice
< wumpus>
you can argue it shouldn't have been introduced in the first place, but it makes no sense to remove it now
< wumpus>
you should be careful what you PR
< wumpus>
and what you merge
< wumpus>
pay attention and review, also conceptually
< wumpus>
and with regard to maintenance burden
< yevaud>
jeremyrubin: it turns out that breaking up bitcoin into different daemons needs a comical amount of state exposed, sadly.
< jeremyrubin>
I don't think so for RPC credentials
< jeremyrubin>
I think it's relatively small
< wumpus>
I'm trying to be careful but too many people just want to kitchen sink everything
< jeremyrubin>
Is anyone opposed to work that separates out RPC credentials to a different thread, makes sure there's no state leak? Then when Russ pushes through the experimental capnproto stuff it can be forked out?
< wumpus>
it shouldn't be a different thread, no
< yevaud>
capnproto?
< sipa>
i don't see how any of these things are related
< wumpus>
agree
< wumpus>
it sounds like complicating things even more (at least inthe repository)
< jeremyrubin>
The idea being to make the credentialing system removed from core. Doesn't need to be a thread, just an object that can pass things to core through a proxy-like interface
< sipa>
define "removed from core"
< sipa>
from the codebase?
< sipa>
from the process?
< sipa>
from the interface?
< wumpus>
some people are actually filtering RPC and using some external proxy to filter allowed/disallowed RPC calls
< wumpus>
no changes to bitcoin coren eeded, at all
< jeremyrubin>
Ah
< jeremyrubin>
I'm not just talking about the lists
< jeremyrubin>
I'm talking about the identities as well
< wumpus>
they can do that as well
< jeremyrubin>
Yep
< wumpus>
and logging
< wumpus>
et
< jeremyrubin>
And by removed I mean that it could be a separate binary
< wumpus>
yes, or a script
< wumpus>
there's nothing we need to do for this
< wumpus>
it's socially scalable :)
< jeremyrubin>
wumpus: it's more if you want to 'remove the crap' from Core, you can actually get rid of the code that's there
< yevaud>
the panacea really is getting the wallet and networking into their own daemons, but that's not going to happen any time soon.
< wumpus>
I'm not trying to remove anything, just to prevent adding it
< wumpus>
once added it's pretty much too late
< jeremyrubin>
Ah ok. So one thing that is in favor of feature discovery in that context
< jeremyrubin>
Is that you can write a generic proxy that learns the features from core and not have to ever touch that program
< jeremyrubin>
So maybe there's some minimal set of features that make writing these proxies a bit better.
< wumpus>
the proxy should already know what is allowd and what not, that shouldn't determine on the target
< jeremyrubin>
Allowed/disallowed differs from exists/doesn't exist and versions
< jeremyrubin>
E.g., if I configure a proxy for v1 to be safe, but v2 adds an argument which is unsafe
< jeremyrubin>
That's a bad situation
< wumpus>
yes before upgrading bitcoin core (definitely the major version) you need to make sure your other software is compatible
< jeremyrubin>
Yeah i think that's just asking a bit much of the user...
< jeremyrubin>
but I guess they'll lose their funds sooner or later if that's the case so maybe we don't care to help them?
< wumpus>
I don't think any general mechanism can prevent that unless you can describe *every* detail of the RPC interface programatically ,and even then
< wumpus>
wait, I don't think we should do RPC changes that can result in losing funds in the first place?!?
< wumpus>
we've been very careful with this, even getbalance still has a dummy argument
< jeremyrubin>
I think that's inevitable that new flags can do stupid things
< jeremyrubin>
E.g., adding a maxFeeRate flag that previously was set to a fixed constant
< wumpus>
if you see any RPC change that can result in old software suddenly doing destructive things please flag it on review
< jeremyrubin>
Herculean task :p
< wumpus>
well if it is too much of a task to pay attention to review of things like that
< wumpus>
it's definitely too much to expect people to update some featur detection mechanism
< jeremyrubin>
I mean to say I'm not even aware of all of the downstream software, let alone how it handles these things
< wumpus>
be realistic here
< jeremyrubin>
Well I think it's sort of a case of helping the developers do the right thing.
< jeremyrubin>
I'm growing in fondness for explicit aliases for all rpcs
< jeremyrubin>
I think swift's mangler does something where A(foo: int, bar:String?) becomes A() and A_with_bar()
< wumpus>
you could achieve something like that by forbidding default arguments
< wumpus>
if some new argument is added, it needs to be specified ... then again, that completely breaks backwards compatibility, which is for most people even worse I think
< jeremyrubin>
Well... maybe not?
< wumpus>
there's an implicit expectation already that calling the same RPCs with the same arguments keeps doing the same
< wumpus>
if any arguments are added their default is the same as what was done before
< wumpus>
in any case, it would help if you have specific examples of where this went wrong in the past
< wumpus>
where people lost funds due to RPCs changing
< jeremyrubin>
Not off the top of my head. But I can imagine someone sanitizing a JSON for arguments by checking what's there against the old arguments, but not checking for presence of new ones
< wumpus>
if not, this is kind of an empty discussion, something that is already avoided at all ocsts
< jeremyrubin>
and then an attacker being able to sneak in an argument that changes the behavior.
< wumpus>
where did the attacker come from? this was about accidental version incompatiblities right
< jeremyrubin>
Would be prevented by explicitly destructing/rebuilding the JSON with only the desired args. Don't know how common that is tho
< * wumpus>
is confused, logging off for now
< jeremyrubin>
Yeah sorry not trying to cause a kerfuffle over it
< jeremyrubin>
I don't know what to tell end-users other than to ship their applications with a specific bitcoind version
< jeremyrubin>
which i think is an OK answerrt
< phantomcircuit>
meshcollider, i've been working on the "rescanblockchain" rpc and noticed a few oddities in the wallet logic; for example #19216
< gribble>
https://github.com/bitcoin/bitcoin/issues/19216 | wallet: Remove first parameter to ScanForWalletTransactions start_hash by pstratem · Pull Request #19216 · bitcoin/bitcoin · GitHub
< sipa>
jeremyrubin: i think that wumpus' point is that no matter how well we try to encode compatibilities electronically, things may be too subtle and in the end people/client developers will for production use still need to refer to the release notes
< jeremyrubin>
sipa: agree 100%.
< sipa>
jeremyrubin: but at the same time, solving compatibility is a much more widely scoped problem than the security side
< sipa>
RPC changes should not impact security
< phantomcircuit>
meshcollider, im going through other parameters looking for oddities as well
< wumpus>
yes, I think that's another reason I'm confused about things like whitelists and credentials, the RPC interface was never supposed to be hardened against attackers like that, it was supposed to be a trusted interface for applications
< jeremyrubin>
sipa: in the example I was giving above I was imagining some frontend app passes a JSON of a desired RPC call. The server checks all the arguments it knows about for that rpc, and sends to the server. But a new RPC was added in the new version of core running. Then the frontend-hacker adds that parameter. And a bad RPC call accidentally gets passed.
< sipa>
jeremyrubin: if the frontend that does the security check is compromised, i don't see how problems can be avoided?
< jeremyrubin>
No the backend server is doing the check
< sipa>
ok, then i don't understand the problem
< jeremyrubin>
Ok. So imagine v1 RPC A has arg 'good'. V2 adds arg 'bad' default = 0
< MarcoFalke>
I plan to write a feature detection in 5 lines of python and add it to the test framework. No changes to core required.
< sipa>
jeremyrubin: that should never happen
< MarcoFalke>
We might be over-engineering this a bit
< jeremyrubin>
sipa: why not? We add default args all the time.
< sipa>
and they shouldn't impact security
< jeremyrubin>
and the 'bad' one by default does nothing
< jeremyrubin>
But we add ones that could impact security if set to a bad value?
< wumpus>
sipa: exactly, that should already never happen, so there shouldn't be need to describe it
< jeremyrubin>
things like feerates
< wumpus>
which was my point with catching these things at review time
< sipa>
jeremyrubin: i'm still missing part of your context
< sipa>
"the server checks ... and sends to the server"
< jeremyrubin>
Ah yeah I was still explaining but you told me that I was already off
< sipa>
there are multiple servers involved?
< sipa>
ok
< jeremyrubin>
There's a frontend, a backend, and a core node
< jeremyrubin>
(I'll tell you when I'm done)
< jeremyrubin>
Frontend generates and RPC, passes it to the backend. Backend sanitizes, passes to core
< jeremyrubin>
v1: A good. v2: A good, bad=0
< sipa>
sounds like the backends needs to disallow parameters it does not know about
< jeremyrubin>
the backend initially checks that A.good is 0 or 1
< jeremyrubin>
Yes
< jeremyrubin>
My point being that originally Core was doing this check
< jeremyrubin>
And then when the default arg gets introduced core stops doing this
< wumpus>
exactly, the security checking thing should be careful to only pass through what it knows about
< wumpus>
this was always the case
< jeremyrubin>
So when core goes V1 -> V2 there's a potential for a security issue
< sipa>
jeremyrubin: that's why a security-enforcing proxy needs to work with whitelisting
< sipa>
per RPC, per parameter
< jeremyrubin>
I don't think we're disagreeing on any of this
< sipa>
then i don't see the problem
< jeremyrubin>
I'm just saying I don't think it's that common that this happens, and there's an opportunity to introduce some stuff that helps prevent these bugs.
< jeremyrubin>
I don't think it eliminates them
< jeremyrubin>
But I think it could help reasonable app developers not have these sorts of mistake
< sipa>
ok, i agree there - but i don't think it's worth the complexity, especially as it risks giving a false sense of security
< jeremyrubin>
fwiw my proposal is essentially an API where you do feature detect and fail to start if it doesn't match your expected feature set
< sipa>
changes to the RPC interface should be safe in such a way that if an RPC that worked in the past was fine, if it still exists in the new version, is still fine
< sipa>
jeremyrubin: but how do you define feature? is a slightly different way that ping messages are scheduled a feature change? because it may be observable through the minping field
< sipa>
and if feature changes are restricted to things that could have a security impact... the probably just shouldn't happen
< jeremyrubin>
You know it's a good question to ask more of the app developers (e.g., LN btcpay)
< jeremyrubin>
I think it's also an interesting question if there were a change we had to make for security, how would we make sure old software didn't do the broken thing on new nodes (hopefully never happens but you never know)
< jeremyrubin>
Again I don't really have a concrete proposal here
< sipa>
we could rename RPCs if we're somehow forced to change their semantics very substantially
< jeremyrubin>
I just know that calling all the RPCs in a batch to see what's on sounds like a very wrong idea
< jeremyrubin>
And would like to help BTCPayServer not do that
< sipa>
yes, they should check version numbers - i agree
< sipa>
with wumpus
< jeremyrubin>
Maybe a good one is just an RPC where you submit a list of RPCs an arguments intended to the node
< sipa>
and just fail to start if it's not a known one
< jeremyrubin>
Yeah that could be the answer
< meshcollider>
phantomcircuit: sounds good :)
< jeremyrubin>
I do know that RPC whitelisting did expose this problem by changing what batching returns when something isn't whitelisted in the batch. So I regret that. But seems like there should be a good answer of what BTCPayServer should implement instead of that for detection
< jeremyrubin>
NicolasDorier: ^^^ stop it :)
< sipa>
and we could automate some things, which would mean that generally client versions will break slightly less often when facing an unknown server... but if semantics are super critical, that still risks breaking things, and they still really should just check the version number
< jeremyrubin>
Ah!
< jeremyrubin>
Here's a great reason to do this; makes people more likely to upgrade their supported node version :p
< jeremyrubin>
But I'm basically at my depth on this issue; I don't have a current major core-dependent app dealing with compatibility issues so it is really a matter of e.g. roasbeef, NicolasDorier to chime in on it.
< jeremyrubin>
I think in terms of the actual Bug I'm OK with wontfix if you're using batching + whitelist with non whitelisted rpcs
< sipa>
it sounds like the batching/whitelist implementation was just broken?
< jeremyrubin>
Not really?
< sipa>
i'm not familiar with the issue
< jeremyrubin>
So if you call a batch
< jeremyrubin>
and one of the RPCs is not whitelisted
< jeremyrubin>
It will call the prefix
< jeremyrubin>
and then just return the RPC not found for the whole thing
< jeremyrubin>
rather than RPC not found for that specific RPC
< jeremyrubin>
I mean the bigger picture issue is that this *should* have come up in review and integration testing
< jeremyrubin>
as it seems to break btcpayserver on startup
< jeremyrubin>
And whitelistrpc has been merged for a while
< sipa>
and it should definitely have come up during rc testing...
< jeremyrubin>
It's not like some corner case issue
< sipa>
but i don't see why we can't just fix this in 0.20.1
< jeremyrubin>
It can be fixed, but permanently 0.20 will be hard to start against for BTCPayServer if using rpcwhitelist
< sipa>
sure, they should have a better way of doing feature detection, but it sounds like this just gratutiously broke compatibility, so we should restore it?
< sipa>
well that's what you have bugfix releases for
< sipa>
bugs happen, and sometimes they're detected too late
< jeremyrubin>
Well it didn't break it thaaat gratuitously. If you don't use RPC Whitelist OR you detect features differently, it would not come up
< sipa>
by gratuitously i don't mean that this was trivial to find; just that it comes at no benefit
< sipa>
as in: i don't expect anyone to (want to) rely on the new behavior
< jeremyrubin>
Ah. Yes. Agree 100%
< sipa>
so we should fix it
< jeremyrubin>
The new behavior is obviously a defect
< sipa>
the feature discovery (or version checking) discussion is orthogonal
< jeremyrubin>
It is, which is what I've been trying to get across
< jeremyrubin>
There's a defect. The reason we found it was doing X. X is shocking. We should fix the defect and provide a better way to X
< sipa>
or tell people that instead of X, they should be doing Y which is already possible instead :)
< sipa>
but we should still fix the defect
< sipa>
is there an issue (or PR) for the fix?
< jeremyrubin>
Sure... which is sort of the question that started this. What is Y? Is just parsing the help page sufficient? Should we JSON expose it?
< jeremyrubin>
sipa: no, not yet.
< sipa>
Y is checking the version number
< jeremyrubin>
I think that it's sort of a 'draw the fucking owl' answer though. So check the version number and hardcode all RPCs and the arguments they can take?
< wumpus>
also, as said, there is work underway in making the help more programatically parsable, so if that's what you want to do it's only going to become easier
< sipa>
jeremyrubin: i mean... what are they doing with the information about which RPCs are available? presumably pretty much something like that already?
< wumpus>
of course it's never going to be a perfect representation fully covering all changes and all subtleties, it's not there to give a false sense of security
< jeremyrubin>
sipa: good question? Not sure. I would imagine some kind of start with assuming a base version with X features and a dispatch table of functionality that implemented inefficiently client side. See what comes back and replace the client versions with RPC versions?
< sipa>
yes
< jeremyrubin>
So the code is never aware of what version at all, just aware of what's allowed and adds the on-node versions as applicable.
< jeremyrubin>
There's no mapping of version 18 has Y and version 19 has Y + Z
< jeremyrubin>
sipa: wumpus: apologies for the drawn out conversation here. It's not a particularly major thing I'm trying to get in for Core, but I just feel somewhat responsible for having introduced it to advocate for the users even if I don't really know what they want/need on this one.
< jnewbery>
promag: done. Thank you for you persistance on multiwallet!