< instagibbs>
We should think long and hard about use-cases that don't involve importing addresses. I think importing "mixed" descriptors where you own some privkeys but not all seems like something to look at?
< achow101>
instagibbs: sure
< instagibbs>
i dont think the "mutation" is as bad provided the user is asking for those sigs
< instagibbs>
fwiw
< instagibbs>
the ismine cleanup is much more important, making sure you don't think you got paid at an address you gave to someone
< achow101>
i guess we want the wallet to behave in two different ways: 1) deal with scripts and track transactions related to those scripts; and 2) be a signer that has keys and just signs things ignoring the scripts
< instagibbs>
sign loosely(provided user knows what they are signing), accept strictly
< instagibbs>
So I think it's probably ok to eagerly sign whatever you can when asked, and separately support importing descriptors where you don't have all the keys(this should just make signing easier?)
< achow101>
right
< instagibbs>
Hm, something to mull over.
< achow101>
there's also a bunch of places in the tests that use dumpprivkey to just get a private key for imports and stuff
< instagibbs>
Is it too asinine to only allow xpubs, then multiwallet to have the xprv in another wallet?
< instagibbs>
but yeah no export
< instagibbs>
that's the rub eh
< achow101>
but no export means those don't work
< achow101>
i suppose we could add something to the test framework to generate private keys
< instagibbs>
It seems like a very reasonable use-case to support.
< instagibbs>
Core-generated key being in a multisig :)
< achow101>
my original thoughts about the multisig stuff would be you have 2 wallets, one generated normally, and another watch only one for the multisig. so you then use psbt and multiwallet
< instagibbs>
what's the hold-up on export? feature creep?
< achow101>
security questions about unhardened derivation
< instagibbs>
oh im overthinking, use-case is supported, *if* you can get the account-level xpub
< instagibbs>
i can't recall what your PR supports
< achow101>
currently no exports at all whatoever
< instagibbs>
ok so I think public descriptor export is a thing we'd want to support
< achow101>
yeah
< achow101>
that'll be next
< sipa>
achow101: i don't think being able to sign for non-"ismine" things is a problem
< sipa>
as long as they're not counted
< fanquake>
wumpus / sipa: can you block Angelika1920
< bitcoin-git>
[bitcoin] uzyn opened pull request #18502: doc: Update docs for getbalance (default minconf should be 0) (master...doc-getbalance) https://github.com/bitcoin/bitcoin/pull/18502
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18503: init: Replace URL_WEBSITE with PACKAGE_URL (master...2004-initRebrand) https://github.com/bitcoin/bitcoin/pull/18503
< luke-jr>
maybe we should just remove getbalance if it isn't going to get fixed :x
< instagibbs>
I thought getbalances was going to replace it anyways
< wumpus>
hebasto: if we don't want that in master, then we need to cherry-pick it *after* the branch off right?
< instagibbs>
i guess i don't see any deprecation on that tho
< luke-jr>
instagibbs: yeah, that was my understanding as well
< wumpus>
I don't think we should be deprecating wallet RPCs left and right tbh
< wumpus>
every change requires changes to all software using bitcoin core, sure, sometimes it's really unavoidable (APIs that don't make sense anymore given our implementation, or are a lot of bother to maintain), but I don't think `getbalance` is that
< luke-jr>
there's already autoconf stuff to handle this afaik
< jonatack__>
but no hurry and they need to go through a deprecation process
< wumpus>
well on the plus side, at some point we might not need boost anymore and can close the issue :')
< wumpus>
in any case I don't have a strong opinion about it, if the build system ppl thing it's ok to merge our own version into master now and risk overwriting it later that's fine with me too
< aj>
wumpus: (we need boost for multi_index_container in txmempool; seems hard to do away with)
< luke-jr>
is there a reason we need the boost lib path at all?
< luke-jr>
why can't it just use -lboost_*
< luke-jr>
?
< wumpus>
aj: sure but I mean if that's the only thing left, we can probably find a replacment
< wumpus>
luke-jr: because -lboost_ doesn't work if the right -L is not specified
< wumpus>
boost libraries are not in the linker default library path
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18507: test: Check that calling walletpasshprase does not freeze the node (master...2004-qaWalletFreeze) https://github.com/bitcoin/bitcoin/pull/18507
< luke-jr>
bleh, my comma removal screwed up other formatting
< promag>
MarcoFalke: I can pick your commit if you want
< promag>
well no need to hold them, you can backport a revert \m/
< MarcoFalke>
There will always be bugs and bugfixes, which can be backported after sufficient review. But at some point we need to open master again.
< achow101>
instagibbs: sipa: it turns out that signing with all spkmans still doesn't work for traditional signrawtransaction* workflows. It should work with PSBTs though. Could we just tell everyone who wants to use descriptor wallets to use PSBT and just disable *rawtransaction RPCs for it as well?
< sipa>
achow101: hmm, why not?
< sipa>
(i don't actually understand the issue)
< achow101>
We need to know the reddenScript but that would worker not be in the wallet, or be in a different spkman
< achow101>
So the spkman currently attending to sign doesn't know the redeemScript
< achow101>
*attempting
< sipa>
can't you import it?
< sipa>
oh, i see
< sipa>
because the private key will be in a different spkman than the one that knows the redeemscript?
< achow101>
Yes
< achow101>
i suppose one method could be to convert everything into PSBTs internally and just pass around PSBTs between the spkmans
< sipa>
or make private keys global for the entire wallet
< sipa>
but converting everything to PSBTs internally seems like a good idea in any case
< achow101>
since descriptor wallets is currently an optional feature, I would be fine with requiring PSBTs for now
< achow101>
then change the wallet to use PSBTs internally everywhere to solve this particular issue
< achow101>
I do have a long term goal of making the wallet use PSBTs everywhere anyways
< sipa>
but the level of review necessary for those things already far outweighs the (imho) minimal complication of just splitting variables up into a bool + other var
< jeremyrubin>
issue exists outside of taproot
< sipa>
sure
< sipa>
i wasn't speaking about that specifically
< jeremyrubin>
at large, not just in this context -- if we need to gate development of features which touch consensus to not use c++17 features in consensus
< jeremyrubin>
because it will interfere with backports
< luke-jr>
IMO we should just not allow C++17 for consensus code where it would make this problem
< jeremyrubin>
So there's three "solutions"
< sipa>
right, until those are outside of backport window
< jeremyrubin>
1) No c++17 stuff for a while because of backports
< jeremyrubin>
2) Allow linking in boost c++17 API fill-ins that we already have until out of support window
< wumpus>
I think we should either switch to C++17 for the entire codebase, or not at all
< luke-jr>
jeremyrubin: we don't have in libconsensus
< sipa>
i think that once master is c++17, we can switch backports to c++17 as well, actually
< wumpus>
moderating which parts of the codebase C++17 features are allowed and where not makes things very complicated for reviewers and maintainers
< luke-jr>
sipa: defeats one big point of backports?
< jeremyrubin>
3) upgrade backports to c++17 :)
< wumpus>
so I'd prefer to not use C++17 at all then
< hebasto>
Not switching from c++11 for the entire codebase could be a problem for Qt stuff on macOS
< luke-jr>
IIRC backports only delays us an extra year, right?
< wumpus>
it's not like we really need it
< sipa>
wumpus: well that's a deadlock, because there will always be some backport to support
< wumpus>
it'[s always 'it would be nice to use this new c++ standard'
< wumpus>
which it would be, sure
< sipa>
so independent of the urgency of c++17 or when it is introduced, it's a good question to address
< sipa>
luke-jr: backports exist because introduced features introduce compatibility issues for people who want a safer upgrade path
< jeremyrubin>
It's also the right time to start thinking about it IMO -- if 0.21 will be c++17 release
< jeremyrubin>
Which I've seen wumpus say is feasible
< wumpus>
yes, I just think using C++17 for only parts of the codebase is impractical
< luke-jr>
sipa: one of those issues is C++ version compat
< sipa>
but if there are a significant amount of people who wouldn't be able to upgrade to a new major version because of c++ language compatibility issues, we simply shouldn't update to c++17 (yet)
< luke-jr>
how hard would it be to compile some of the codebase with C++17 enabled, and not others?
< sipa>
wumpus: that's fair
< cfields>
luke-jr: I really don't like that idea.
< wumpus>
luke-jr: I'm sure it's *possible* but I fear all kinds of API conflicts
< jeremyrubin>
luke-jr: would probably make binaries bigger
< luke-jr>
hmm
< cfields>
luke-jr: I think the threat of something like an uncaught exception is very real in that scenario.
< jeremyrubin>
wumpus: yeah
< luke-jr>
cfields: right, I see
< wumpus>
cfields: yes, it's probably an unacceptable risk in our case
< jeremyrubin>
How was this handled historically?
< wumpus>
of course, if it's only for checking
< sipa>
so maybe we can settle on: we don't update to c++17 until we're in a position where no new backport releases are expected
< wumpus>
you could compile the consensus files with c++11 *as well*
< jeremyrubin>
Or was the switch to 11 widely supported enough at the time
< wumpus>
and throw away the result
< sipa>
if then an emergency happens that causes us to revert on that - so be it
< jeremyrubin>
Or we can turn on c++17 for 0.20, but compile both for checking as wumpus suggests
< jeremyrubin>
That way we're "priming" our backport window
< wumpus>
I'd still prefer not to do this (also because what is 'consensus code' is not very well isolated in our code base)
< jeremyrubin>
I don't think c++17 has any *breaking* changes?
< jeremyrubin>
wumpus: I'm saying the whole codebase not just consensus
< cfields>
jeremyrubin: iirc ^^ is what we did for c++11.
< wumpus>
compile the entire codebase with c++17 and c++11? that's a lot of cverhead
< jeremyrubin>
We can upgrade to c++17 today but not accept code changes that break c++11 compat.
< sipa>
wumpus: we did that for a while with c++11 i think, i actually?
< sipa>
though i'm not sure this is the right time; our focus now is 0.20
< jeremyrubin>
Releases etc can be c++17, but anyone can build for c++11. We probably need to do this for 1-2 releases
< jeremyrubin>
sipa: it is the right time to start worrying about this
< sipa>
and i think the discussion of when c++17 is a different one
< wumpus>
sipa: oh you mean more like a travis run that compiles with c++17 instead of c++11?
< cfields>
sipa: yea, that's what we did.
< sipa>
wumpus: right
< jeremyrubin>
Because if we do it for 0.20 it means 0.21 and 0.22 can use c++17
< cfields>
sipa: we had a release that was "technically" c++11, but iirc we didn't force the flag on.
< cfields>
Then we forced it on in master.
< sipa>
cfields: right
< wumpus>
makes sense to do that again then
< wumpus>
for 0.21
< cfields>
Don't remember for backports, I guess we were just kinda careful/reasonable about it?
< sipa>
i guess if someone does the work for adding a c++17 travis build, and it passes with effectively no code changes... why not
< wumpus>
I don't expect that to take a lot of changes
< sipa>
i think so
< jeremyrubin>
sounds like a plan to me?
< cfields>
Also, we banned certain features when we started with c++11. thread_local is the obvious one that comes to mind, but IIRC there were others as well.
< dongcarl>
if there's consensus, can someone summarize the game plan?
< jeremyrubin>
I can try...
< jeremyrubin>
1) Make 0.20 c++17 and c++11 buildable if easily doable
< wumpus>
dongcarl: the idea is to add a travis run to compile the current code with c++17, do the minimum changes to be compatible with c++17 as well as c++11, for 0.21
< sipa>
trying a C++17 build now
< sipa>
with gcc 9.3
< jeremyrubin>
I thought we'd do it for 0.20 if it's a small patchset and we haven't branched it yet?
< wumpus>
and delay the real switch to c++17 (like actually using new features) to 0.22
< wumpus>
no, we're not going to do anything like that for 0.20
< jeremyrubin>
Why not do it in 0.20 if it can be done?
< jeremyrubin>
It has basically 0 functional changes?
< wumpus>
0.20.0rc1 should have been tagged yesterday
< wumpus>
if anything is going in it's bugfixes
< cfields>
fwiw, I'd like to do at least a small audit on the c++17/c++20 errata to see if there are any obvious implementation minefields to look out for.
< jeremyrubin>
I mean it sort of doesn't really matter if it's in 0.20 or not as c++17 compat can be backported into a minor anyways...
< dongcarl>
Okay, so what will gitian builds use? v0.20: c++11, v0.21: c++11, v0.22: c++17?
< cfields>
(c++20 because presumably several c++17 issues were fixed there)
< dongcarl>
cfields: Right, good call
< wumpus>
dongcarl: gitian builds could use c++17 from 0.21 on
< wumpus>
it just still has to be compatible with c++1
< sipa>
wumpus: ah, so we can easily back out back to c++11 if issues are discovered?
< wumpus>
so that things can be backported
< wumpus>
sipa: that too
< sipa>
right
< sipa>
that seems reasonable
< cfields>
Not sure about g++, but clang++ definitely has a switch for enforcing c++11 syntax/features for newer standards.
< cfields>
So that'd be easy enough to lint/automate.
< sipa>
oh the irony
< jeremyrubin>
cfields: we only need one compiler to have that feature fortunately
< sipa>
the only compile error i get with c++17 is Span related
< cfields>
jeremyrubin: right
< dongcarl>
Got it: v0.20: COMPAT with c++11 + c++17 only if easily doable, GITIAN with c++11; v0.21: COMPAT with c++11 + c++17, GITIAN with c++17; v0.22: COMPAT with c++17, GITIAN with c++17. Is this the plan?
< wumpus>
sgtm
< sipa>
dongcarl: ack
< dongcarl>
Great. Will post on the c++17 issue
< jeremyrubin>
Loks right. And backports from 0.22 only go to 0.21?
< jeremyrubin>
Or to 0.20 too if we can get those builds up easily
< jeremyrubin>
(depending on the feature using APIs that make backport non-trivial)
< wumpus>
jeremyrubin: yes
< cfields>
I think as long as we don't race to replace every line of code with a c++17ism we'll be fine, generally.
< wumpus>
it's pretty rare in the first place to backport things two releases
< wumpus>
cfields: also that
< jeremyrubin>
cfields: noooo you can't just refactor the whole.... hahahah find and replace go refactorrrrrrrrrr
< sipa>
refactorrr̅
< wumpus>
hehe I think we had a rule for that for C++11 as well, don't C++11-ize things for the sake of doing so
< sipa>
right
< wumpus>
is someone going to post this into the c++17 issue?
< jeremyrubin>
dongcarl:
< sipa>
FWIW, with #18468 + updating ax_cxx_compile_stdcxx.m4, everything compiles in c++17
< sipa>
c++14 is a very minor change; c++17 is somewhat bigger, but not nearly as big as c++03->c++11
< dongcarl>
jeremyrubin: Perhaps you can write a little about the backporting situation
< cncr04s>
mostly due to compilers and distros taking forever to update to them
< cfields>
sipa: good point.
< sipa>
also, all functional/unit tests run with c++17 (after 18468 )
< dongcarl>
cfields: I think libstdc++ errata is the important one, yes?
< cfields>
dongcarl: hmm?
< cfields>
dongcarl: Sorry, I meant errata from the standard itself. Somewhere there's a collection of "oops, stuff we clearly got wrong in the spec".
< dongcarl>
Ah okay I understand now
< cfields>
dongcarl: but looking at implementation notes makes sense too.
< sipa>
also compiles with clang++ 6.0 under c++17
< elichai2>
So the time to start slowly replacing boost with c++17 is after 0.21 is branched off?
< jeremyrubin>
elichai2: maybe; it kinda depends. If it takes 6+ months for your contribution to get reviewed and stuff to be merged you can do that... now! But you'd then possibly be merge gated until 0.21 is branched.
< elichai2>
(I belive this isn't considered C++17-ize things just for the sake of doing so)
< jeremyrubin>
It also depends on if your stuff has obvious backport shims
< jeremyrubin>
We're already using option types for instance
< elichai2>
Right. option should be easy to replace. I'm more thinking std::fs and the harder parts
< sipa>
elichai2: after 0.22 is branched off
< sipa>
eh, i guess that's ambiguous
< sipa>
as soon as master branch is on the path to become 0.22, a year from now approximately
< jeremyrubin>
sipa: there's a difference between the review window and the merge window
< sipa>
oh no
< sipa>
i'm confusing myself
< elichai2>
Sipa, right so when 0.21 is branched off and master is 0.22 ;)
< sipa>
so half a year from now, at the very earliest
< sipa>
this is funny in the list of deprecated c++17 features:
< sipa>
P0618R0 Deprecate <codecvt> The entire header <codecvt> (which does not contain the class codecvt!) is deprecated, as are the utilities wstring_convert and wbuffer_convert. These features are hard to use correctly, and there are doubts whether they are even specified correctly. Users should use dedicated text-processing libraries instead.
< sipa>
"this turns out to be hard; let someone else do it"
< elichai2>
Loool
< elichai2>
I think the new standards are cool on one hand, but on the other it seems like stl is just getting messier and messier
< jeremyrubin>
messy stl > boost tho
< jeremyrubin>
ugh github is kinda down
< sipa>
elichai2: i think modern stl is pretty nice
< sipa>
it's of course growing is features
< sipa>
but they do seem to put a lot of work in making sure things work together nicely
< elichai2>
jeremyrubin: oh that's a give. Boost is the kind of messy
< elichai2>
sipa: right, Altough I'm not sure how I feel about their ADL overuse
< sipa>
boost is really the STL sandbox :p
< sipa>
ADL?
< elichai2>
Argument dependent lookup
< elichai2>
That you can have `void my_func(A& a);` and then you can just call `a.my_func()`
< sipa>
elichai2: reading up on it
< jeremyrubin>
Oh so you can patch in stuff to a class?
< sipa>
it doesn't seem to be what you're saying
< jeremyrubin>
Is it a syntax change?
< elichai2>
Yeah I might be confusing 2 terms
< elichai2>
Hard to see on my phone
< sipa>
just that if you havea function A in namespace B, you can call `A(B::something)`, and it will attempt resolving it as "B::A(B::something)"
< sipa>
without being in namespace B
< elichai2>
what I remembered also doesn't seem to work hmm, maybe I'm confusing a c++2x proposal I've read, let me check
< promag>
sorry, so when rc1?
< sipa>
elichai2: it's hard to claim there is ADL overuse, when what you imagined ADL was isn't even possible :p
< elichai2>
sipa: you're right hehe, I'm now doubting me memory lol
< elichai2>
seeing way too many cppcon talks
< sipa>
STL does use ADL for some things, for example the idion for calling swap on generic types is {using namespace std; swap(a, b); }
< sipa>
which will use std::swap if possible, but also consider the namespaces a and b are to find a definition for a swap
< sipa>
otherwise you'd need to inject swap into the std namespace for user-defined types
< sipa>
arguably, it's saying that anything, in whatever namespace, with the name swap should have swap-like semantics, which is pretty polluting conceptually
< jonatack_>
luke-jr: 18192 lgtm... github killed my review ack
< elichai2>
sipa: Ok, at this point I honestly got no idea where that came into my mind. it seems non existent :O
< kanzure>
hi/win 237
< elichai2>
I should re-watch the C++17 talks
< dongcarl>
GitHub is down! *crab dance*
< kanzure>
ah crap
< jonatack_>
github yay 🏆
< elichai2>
dongcarl: that explains why cargo is giving me troubles :/
< jeremyrubin>
[13:50] <sipa> arguably, it's saying that anything, in whatever namespace, with the name swap should have swap-like semantics, which is pretty polluting conceptually
< jeremyrubin>
sipa: I think this will be improved with concepts right?
< bitcoin-git>
bitcoin/master f65c9ad Elichai Turkel: Check for overflow when calculating sum of outputs
< bitcoin-git>
bitcoin/master dce6f3b MarcoFalke: Merge #18383: refactor: Check for overflow when calculating sum of tx outp...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #18383: refactor: Check for overflow when calculating sum of tx outputs (master...2020-03-value-overflow) https://github.com/bitcoin/bitcoin/pull/18383
< sipa>
jeremyrubin: partially maybe
< sipa>
it still means that various stl libraries will try to invoke your user-defined namespace's swap functions if you're not careful
< sipa>
*stl functions
< jeremyrubin>
True.
< jeremyrubin>
Also looking at c++20 swappable it looks wrong for accomplishing this
< jeremyrubin>
But in theory you could have a concept which uses std::swap to delegate to the member function swap
< sipa>
it seems c++20 actually explicitly allows user-defined std::swap (in the std namespace)
< jeremyrubin>
Probably a better way to fix it as most existing code bases will take a long time to get any sort of concepts deeply integrated
< jeremyrubin>
TBH one of the c++17 features I'm most excited for is the map/unordered_map node extract API :p
< fanquake>
jonatack: why did you remove parts of the release notes in the wiki? i.e dark mode support on macOS?