< 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] fanquake closed pull request #18489: Rebrand to Bitcoin Corrr (master...2004-rebrandCorrr) https://github.com/bitcoin/bitcoin/pull/18489
< wumpus> fanquake: done
< promag> wumpus: in #18482 kryvel verified that #18487 fixes the deadlock
< gribble> https://github.com/bitcoin/bitcoin/issues/18482 | RPC stuck on walletpassphrase call · Issue #18482 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18487 | rpc: Fix rpcRunLater race in walletpassphrase by promag · Pull Request #18487 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/41fa2926d86a...5c1ba3a10a18
< bitcoin-git> bitcoin/master a46484c Ben Woosley: build: Detect gmtime_* definitions via configure
< bitcoin-git> bitcoin/master 5c1ba3a Wladimir J. van der Laan: Merge #18358: util: fix compilation with mingw-w64 7.0.0
< bitcoin-git> [bitcoin] laanwj merged pull request #18358: util: fix compilation with mingw-w64 7.0.0 (master...mingw_w64_gmtime_s) https://github.com/bitcoin/bitcoin/pull/18358
< promag> still thing #16923 is good for 0.20, hebasto and jonatack__ did a good review/test there
< gribble> https://github.com/bitcoin/bitcoin/issues/16923 | wallet: Handle duplicate fileid exception by promag · Pull Request #16923 · bitcoin/bitcoin · GitHub
< hebasto> wumpus: fanquake: it seems we need cd1e7bb064315ce909dfa1a0a14a5660d985f266 from 0.19 in the master before branching off to fix #17010, no?
< gribble> https://github.com/bitcoin/bitcoin/issues/17010 | Missing Boost::System on ARM Ubuntu 18.04 · Issue #17010 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] hebasto opened pull request #18501: build: Fix boost detection on Ubuntu ARM 18.04 (master...20200402-boost-arm) https://github.com/bitcoin/bitcoin/pull/18501
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #18500: chainparams: Bump assumed valid hash (master...2004-chainparamsBump) https://github.com/bitcoin/bitcoin/pull/18500
< bitcoin-git> [bitcoin] MarcoFalke reopened pull request #18500: chainparams: Bump assumed valid hash (master...2004-chainparamsBump) https://github.com/bitcoin/bitcoin/pull/18500
< 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
< hebasto> wumpus: https://github.com/autoconf-archive/autoconf-archive/pull/198 seems stalled. What plan do you suggest for master?
< luke-jr> wumpus: it's been broken for several releases now tho
< wumpus> if we can't find a way to handle this that is acceptable to autoconf upstream, we should probably drop it completely
< hebasto> ok
< luke-jr> eh, but the current upstream is broken?
< jonatack__> luke-jr: before removing getbalance, i began with removing getunconfirmedbalance with #18451
< gribble> https://github.com/bitcoin/bitcoin/issues/18451 | rpc: remove deprecated getunconfirmedbalance by jonatack · Pull Request #18451 · bitcoin/bitcoin · GitHub
< 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
< luke-jr> wumpus: unlikely, we're probably going to add boost::process soon?
< luke-jr> wumpus: yes they are?
< wumpus> in modern libraries this is solved by pkg-config and similar mechanisms but ofc boost needs to be special
< luke-jr> and this m4 stuff only checks the default library paths anyway
< aj> wumpus: sure, as far as i can tell, boost's is actually pretty good though
< wumpus> aj: okay it's fine if people want to stick with boost forever now that's ok with me too
< wumpus> this changes every few months
< aj> wumpus: boost forever would be a great pr idea in about 364 days i guess
< luke-jr> probably will be some time before all of our boost needs are in C++ std
< wumpus> aj: I"m just tired of the back and forth, one day there's a flurry of 'move away form boost' PRs the next everyone is defending boost
< aj> wumpus: ah, sorry then. didn't mean to end up on the defending boost side
< luke-jr> wumpus: it makes sense to migrate stuff from boost to C++ std, but C++ std doesn't do everything yet
< wumpus> aj: no problem, sorry, not feeling well at the moment shoudln't take it out on you
< aj> wumpus: i am feeling well, so it's fine :)
< bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/6bdd515ccf1b...b83565625e32
< bitcoin-git> bitcoin/master 222253e MarcoFalke: chainparams: Bump assumed valid hash
< bitcoin-git> bitcoin/master b835656 Wladimir J. van der Laan: Merge #18500: chainparams: Bump assumed valid hash
< bitcoin-git> [bitcoin] laanwj merged pull request #18500: chainparams: Bump assumed valid hash (master...2004-chainparamsBump) https://github.com/bitcoin/bitcoin/pull/18500
< bitcoin-git> [bitcoin] laanwj opened pull request #18506: net: Hardcoded seeds update for 0.20 (master...2020_04_hardcoded_seeds) https://github.com/bitcoin/bitcoin/pull/18506
< 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> btw, comment added
< bitcoin-git> [bitcoin] luke-jr opened pull request #18508: RPC: Fix more formatting nits (master...rpcdoc_format_20200402) https://github.com/bitcoin/bitcoin/pull/18508
< MarcoFalke> Looks like #18487 , #18487 and #18487 are the last three things to get in before branch-off?
< gribble> https://github.com/bitcoin/bitcoin/issues/18487 | rpc: Fix rpcRunLater race in walletpassphrase by promag · Pull Request #18487 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18487 | rpc: Fix rpcRunLater race in walletpassphrase by promag · Pull Request #18487 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18487 | rpc: Fix rpcRunLater race in walletpassphrase by promag · Pull Request #18487 · bitcoin/bitcoin · GitHub
< MarcoFalke> * #18458 #18506
< gribble> https://github.com/bitcoin/bitcoin/issues/18458 | net: Add missing cs_vNodes lock by MarcoFalke · Pull Request #18458 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18506 | net: Hardcoded seeds update for 0.20 by laanwj · Pull Request #18506 · bitcoin/bitcoin · GitHub
< MarcoFalke> Even if they miss, they can be backported. I think we should open the master branch again for merges for 0.21
< luke-jr> #18192 and #18465 should be fixed before rc1 IMO
< gribble> https://github.com/bitcoin/bitcoin/issues/18192 | Bugfix: Wallet: Safely deal with change in the address book by luke-jr · Pull Request #18192 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18465 | bitcoin-tx (and probably others) fails to build without libevent · Issue #18465 · bitcoin/bitcoin · GitHub
< 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> agreed
< promag> hi
< fjahr> I think meeting starts in one hour
< promag> fjahr: right, summer time
< promag> thanks
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/b83565625e32...ff53433fe4ed
< bitcoin-git> bitcoin/master 6112a20 Jon Atack: test: replace (send_message + sync_with_ping) with send_and_ping
< bitcoin-git> bitcoin/master ff53433 MarcoFalke: Merge #18494: test: replace (send_message + sync_with_ping) with send_and_...
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #18494: test: replace (send_message + sync_with_ping) with send_and_ping (master...send_and_ping) https://github.com/bitcoin/bitcoin/pull/18494
< wumpus> emzy: your list contains no 0.19.x nodes at all, that's weird!
< emzy> strange. But I think I'm on master
< wumpus> but e.g. cat seeds_sipa.txt |grep "Satoshi:0\\.19"|wc gives 4026 matches, doing the same for your list gives none :/
< wumpus> I'm going to merge alle the lists and deduplicate so for me it's not a big deal, but it might indicate a problem with your crawler
< emzy> I will invastigate later.
< emzy> after update to master I have 0.19 clients
< emzy> so sorry wrong grep. there are still none.
< sipa> i wonder if i turned off tor after realizing it made crawling super slow
< sipa> my % of outgoing connections of my crawler is going up
< emzy> do I have to generate the seeds.txt.gz file?
< emzy> Ok seems to be this: dnsseed.dump S
< emzy> So it was my fault. I had a old seeds.txt.gz file.
< sipa> yeah, it's a gzip of the dnsseed.dump file
< emzy> like: gzip --best <dnsseed.dump >seeds.txt.gz
< emzy> I put a old file up. :(
< wumpus> emzy: no problem, can you post a new one please?
< emzy> I also deleted my dnsseed.dump and all other files. So I will have to wait until it is full again. I only have 902 lines in the file.
< sipa> ah, that's unfortunate
< emzy> And no backup for that server :(
< wumpus> hopefully someone has a list with recent onions in them
< emzy> no onions in my file now :(
< wumpus> #startmeeting
< lightningbot> Meeting started Thu Apr 2 19:01:12 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< jonasschnelli> hi
< sipsorcery> hi
< hebasto> hi
< cfields> hi
< fjahr> hi
< sipa> hi
< amiti> hi
< 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 amiti fjahr
< wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55
< jkczyz> hi
< jb55> hi
< achow101> hi
< jonatack_> hi
< nehan_> hi
< wumpus> no proposed topics for this week, it appears
< elichai2> Hi
< wumpus> any last minute ones?
< jeremyrubin> hi
< luke-jr> wumpus: I saw one earlier.. :p
< jeremyrubin> Hope everyone is safe & healthy :)
< luke-jr> 0.20 bugfixes
< luke-jr> or smth like that
< wumpus> FWIW it's time to do the branch-off for 0.20
< wumpus> and release rc1
< wumpus> luke-jr: don't see it in http://gnusha.org/bitcoin-core-dev/proposedmeetingtopics.txt, but that makes sense
< luke-jr> IMO #18192 and #18465 should be fixed before rc1
< gribble> https://github.com/bitcoin/bitcoin/issues/18192 | Bugfix: Wallet: Safely deal with change in the address book by luke-jr · Pull Request #18192 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18465 | bitcoin-tx (and probably others) fails to build without libevent · Issue #18465 · bitcoin/bitcoin · GitHub
< luke-jr> could perhaps still branch off anyway, as they're not really ready :/
< luke-jr> 18192 only affects avoid-reuse wallets, but the harm is irrepairable once affected
< wumpus> those are not even tagged for 0.20
< promag> hi
< wumpus> unless they're realy urgent might include them in a later rc or 0.20.1
< sipa> are they 0.20 regressions?
< dongcarl> hi
< luke-jr> not regressions, no
< luke-jr> I suppose 18465 has an easy workaround
< sipa> when was 18192 introduced?
< sipa> or when was the problem it solves introduced?
< luke-jr> when avoid-reuse wallets were introduced, 0.19.0 IIRC
< sipa> this sounds moderately serious... i suspect we just haven't heard about it much because few people enable that setting, i expect?
< luke-jr> maybe; I don't know how popular it is
< luke-jr> IMO if we don't fix it, we should at least put it in rel notes as a known issue
< achow101> i doubt people really use multiwallet or create non-default wallets
< luke-jr> achow101: it's not multiwallet-related
< achow101> luke-jr: but you have to be using the multiwallet feature to enable it
< achow101> i.e. create a new wallet
< luke-jr> hmm
< luke-jr> also from earlier [16:52:31] <MarcoFalke> Looks like #18487 , #18487 and #18487 are the last three things to get in before branch-off?
< achow101> oh wait, we can use the setwalletflag rpc to enable it. but that requires knowing what you're doing. it's not exposed in the gui
< gribble> https://github.com/bitcoin/bitcoin/issues/18487 | rpc: Fix rpcRunLater race in walletpassphrase by promag · Pull Request #18487 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18487 | rpc: Fix rpcRunLater race in walletpassphrase by promag · Pull Request #18487 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18487 | rpc: Fix rpcRunLater race in walletpassphrase by promag · Pull Request #18487 · bitcoin/bitcoin · GitHub
< luke-jr> rtt
< luke-jr> err*
< luke-jr> [16:53:06] <MarcoFalke> * #18458 #18506
< gribble> https://github.com/bitcoin/bitcoin/issues/18458 | net: Add missing cs_vNodes lock by MarcoFalke · Pull Request #18458 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18506 | net: Hardcoded seeds update for 0.20 by laanwj · Pull Request #18506 · bitcoin/bitcoin · GitHub
< promag> lol
< luke-jr> 18192 & 18465 should probably at least get tagged, even if they end up slipping the release
< wumpus> ok
< jonatack_> another avoid_reuse related bugfix is #17824 (2 acks)
< gribble> https://github.com/bitcoin/bitcoin/issues/17824 | wallet: Prefer full destination groups in coin selection by fjahr · Pull Request #17824 · bitcoin/bitcoin · GitHub
< wumpus> this adds way more things last-minute for 0.20 than I expected
< jonatack_> (not saying it's as urgent)
< sipa> 17824 seems like a bigger change
< wumpus> we could also add a note to the release notes that avoid-reuse is buggy
< wumpus> or disable it
< wumpus> oh wait it was introduced in 0.19 not 0.20
< wumpus> so not that
< sipa> maybe let's prioritize review on #18192, and see how far we get?
< gribble> https://github.com/bitcoin/bitcoin/issues/18192 | Bugfix: Wallet: Safely deal with change in the address book by luke-jr · Pull Request #18192 · bitcoin/bitcoin · GitHub
< sipa> if not, a release note can always be added
< wumpus> let's set a new deadline for the branch-off then?
< wumpus> we missed yesterday at least :)
< luke-jr> why not just branch off now?
< promag> +1
< wumpus> I want to do rc1 release at the same time
< promag> oh kk
< sipa> seems reasonable to aim to do them simultaneously
< jeremyrubin> #proposedmeetingtopic limited use of boost in consensus for backports
< * luke-jr> shrugs
< wumpus> I don't see a reason to do a branch-off without immediately doing rc1, that would just result in more backporting work
< wumpus> oh no, no boost topic please
< jeremyrubin> lol :|
< sipa> at least not when we have known issues to solve still
< luke-jr> jeremyrubin: we'd need to adapt the build system.. isn't it avoidable?
< jeremyrubin> luke-jr: no it isn't
< wumpus> sipa: right
< luke-jr> what kind of bugfix requires boost? :/
< jeremyrubin> anyways we can discuss after the meeting if needed or when it's the topics turn
< jeremyrubin> but I don't think anyone wants to discuss it now
< jeremyrubin> Except maybe you and I
< wumpus> well it's not like we have other topics
< luke-jr> ^
< jeremyrubin> Ah I took your request literally
< wumpus> #topic limited use of boost in consensus for backports
< jeremyrubin> Well; I think on the 0.21 horizon is upgrading to c++17, one feature of which means things like std::option being available
< jeremyrubin> We're currently reviewing code for things like #18401
< gribble> https://github.com/bitcoin/bitcoin/issues/18401 | Refactor: Initialize PrecomputedTransactionData in CheckInputScripts by jnewbery · Pull Request #18401 · bitcoin/bitcoin · GitHub
< jeremyrubin> Which IMO should be properly written/refactored using option types
< sipa> if it came at no cost, sure
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/18468 | Span improvements by sipa · Pull Request #18468 · bitcoin/bitcoin · GitHub
< dongcarl> Yeah making a little table now, perhaps jeremyrubin can add on about the backporting plan, not sure I understand it fully yet
< dongcarl> (add on in the issue)
< cncr04s> don't make me have to upgrade my distro to support new c++ versions
< sipa> cncr04s: what distro might that be?
< sipa> (we don't generally update c++ versions when it interferes with building on common platforms)
< cncr04s> ubuntu 14.04
< jeremyrubin> I'm not sure upgrading core is your largest concern there
< cfields> cncr04s: also, runtime is not affected. Releases are linked statically against lib(std)c++.
< cfields> So this is 99% a development decision.
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu Apr 2 20:00:43 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< wumpus> cfields: +1
< jeremyrubin> 14.04 has been EOL for a while so you want to upgrade anyways...
< wumpus> the binaries will still work on 14.04
< cncr04s> I should have said new gcc versions but, I stick with the versions that come from repo
< wumpus> but if you want to do development you'll need a gcc that supports c++17
< cncr04s> 18.04 likley then
< sipa> not that according to the plan, you'd need to do so at the earliest once 0.22 branches off (+- a year from now)
< wumpus> this is not unreasonable in these days, mind that this is a discussion for 0.22 at first which is a year away
< wumpus> 16.04 should have (can't find the issue right now tho)
< cfields> Yea, it's much simpler these days. c++11 took _years_ to gain compiler support.
< luke-jr> [19:38:06] <luke-jr> how about only building libconsensus with C++11?
< luke-jr> [19:38:25] <luke-jr> eg a dedicated Travis job that only builds that
< luke-jr> [19:38:39] <luke-jr> might be good to do anyway to ensure we don't accidentally boost-ify it
< cfields> so it was more understandable as a holdup imo.
< luke-jr> [19:38:46] <luke-jr> (don't install boost either)
< luke-jr> sorry, my internet cut out
< cncr04s> still using 0x only just over a year ago
< cncr04s> personally
< dongcarl> jeremyrubin: Updated issue description here: #16684
< gribble> https://github.com/bitcoin/bitcoin/issues/16684 | Discussion: upgrading to C++17 · Issue #16684 · bitcoin/bitcoin · GitHub
< 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 :/
< elichai2> Finally finished compiling stage2 clang-10 :D
< sipa> even gcc 9.3 doesn't have std::span :(
< elichai2> yeah std::span is c++2x
< sipa> gcc 9.3 has some parts of c++2a
< sipa> but not std::span, it seems
< elichai2> right 2a not 2x
< elichai2> yeah clang guys are really into experimental stuff where gcc seems less so
< sipa> it was c++1x(=11), c++1y(=14), c++1z(=17), c++a2(=20)
< dongcarl> std::span support starts with libstdc++ 10
< sipa> and in clang libc++ 7
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/ff53433fe4ed...0d71395848bb
< bitcoin-git> bitcoin/master fa6e01f MarcoFalke: doc: block-relay-only is not blocksonly
< bitcoin-git> bitcoin/master 0d71395 MarcoFalke: Merge #18464: doc: block-relay-only vs blocksonly
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #18464: doc: block-relay-only vs blocksonly (master...2003-docBlockRelayOnly) https://github.com/bitcoin/bitcoin/pull/18464
< 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] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/0d71395848bb...dce6f3b29b41
< 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?