< bitcoin-git> [bitcoin] ken2812221 closed pull request #15376: appveyor: ignore functional test failure (master...appveyor-ignore-functional-tests) https://github.com/bitcoin/bitcoin/pull/15376
< bitcoin-git> [bitcoin] Csi18nAlistairMann opened pull request #15387: docs: add "sections" info to example bitcoin.conf (master...confSections) https://github.com/bitcoin/bitcoin/pull/15387
< wumpus> do we have any code-wide preference for the order of input versus output arguments?
< wumpus> I don't *think* so, but we probably should
< wumpus> (this is something I realized while reviewing #13932)
< gribble> https://github.com/bitcoin/bitcoin/issues/13932 | Additional utility RPCs for PSBT by achow101 · Pull Request #13932 · bitcoin/bitcoin · GitHub
< sipa> i don't think we do
< provoostenator> wumpus: I also noticed it. Initially I was confused, but then found some reasonable arguments in favor of putting outputs first, because the number of input arguments tends to grow over time.
< provoostenator> But I don't think it's worth making rules around that.
< wumpus> at the least, I think having methods with different orderings on the same class is confusing
< gmaxwell> outputs first looks like assignment, output = input. :)
< wumpus> I don't have any particular preference I just like consistency
< sipa> libsecp256k1 has a strict ordering of output first, then inputs, and within each ordered from most-recently-computed to oldest
< provoostenator> Although in general I often find it hard to tell when I see methods get called. It's more clear in the method definitions.
< sipa> but imposing such rules on an existing project sounds much more burden than it's worth
< wumpus> would be better to simply return output arguments, too bad we can't use std::optional
< provoostenator> Especially with all the references it gets confusing fast.
< wumpus> sipa: I agree but what about new code?
< gmaxwell> (also it's a common convention in libc, e.g. memcpy(dest,src,...))
< provoostenator> std::optional sounds great. Part of c++17 so looking forward to using that in 2023
< sipa> c++ stl usually has inputs first, outputs last
< sipa> (different from libc)
< wumpus> yes
< gmaxwell> (sprintf(buf, fmt, vaargs)...)
< wumpus> I don't think c++'s reference syntax made output arguments particularly clearer
< wumpus> at least in C it's usually recognizable because the caller has to take a pointer
< gmaxwell> (to be clear, I don't particularly care, I have a preference for dest first in C, but C++ is already crazy regardless of the order. :P )
< sipa> google c++ only lets you pass const references; if you want something mutable, it needs to be a pointer
< wumpus> in C++ it's sometimes magic 'oh, so this value gets changed'
< wumpus> sipa: I like that
< sipa> (which makes sense syntactically, but semantically is somewhat ridiculous... modern c++ shouldn't need to deal with raw pointers pretty much ever)
< sipa> that was also pre-c++11, i don't know what changed
< sipa> ideally output arguments are actual outputs, and with std::pair/tuple and move semantics that became a lot more reasonable
< wumpus> yea, I don't mean we should do that, but I like that some people thought about this and came to a similar conclusion as me
< sipa> c++17 structured bindings are even nicer
< sipa> auto [a,b,c] = function_returning_3tuple()
< wumpus> yes, nice
< * sipa> patiently awaits RHEL to adopt GCC 8
< wumpus> ideally "output" arguments would only be necessary for "inout" arguments that get modified in place
< wumpus> not for anything that is created anew
< wumpus> GCC 8 ? that's going to be a long wait I'm afraid
< sipa> most of it is in gcc 7
< wumpus> gcc 7 is in ubuntu 18.04 LTS that's good at least
< sipa> gcc 8.2 is in bionic too, it seems: https://packages.ubuntu.com/bionic/gcc-8
< sipa> oh, not sure if that's gcc 8.1 or 8.2
< sipa> yeah, 8.2
< wumpus> it's possible to install the alternative gcc but I think it is installed next to the normal one
< sipa> yes
< wumpus> which is somewhat annoying to work with with build systems and such, remember having some trouble with this at some point
< wumpus> dang I had forgot we have our own Optional<> type wrapping boosts', have suggested using that to avoid this discussion
< wumpus> many translators get confused on the " Config setting for %s only applied on %s network when in [%s] section." message
< wumpus> it's unfortunate that we can't use named substitutions like in python
< rafalcpp> LXC vulnerability (used in Gitian). CVE-2019-5736: runc container breakout (all versions) https://seclists.org/oss-sec/2019/q1/119
< bitcoin-git> [bitcoin] laanwj closed pull request #15155: test: Support -cli tests using external bitcoin-cli (master...test_external_bcli) https://github.com/bitcoin/bitcoin/pull/15155
< rafalcpp> probably not a concern since we run known code inside Gitian LXC container
< wumpus> not a concern for gitian but still good to know, thanks
< rafalcpp> unless, someone would trick a person to build for him his PR in Gitian just to test things, and take over account of e.g. a developer
< rafalcpp> wumpus: Gitian is known to not be affected by this?
< wumpus> I don't think the gitian system is otherwise hardened against, say, malicious descriptors
< wumpus> so I don't think a possible escape changes much
< wumpus> but anyhow, upgrade if you can
< rafalcpp> before distros are fully update, it is probably good if main developers would run Gitian on separate machine, and sign results on another. Sorry to be pedantic, but it's BTC... :)
< wumpus> FWIW I've always done that
< luke-jr> rafalcpp: it's a concern for anyone who does gitian builds of arbitrary repos
< luke-jr> and yes, my gitian sign script is `true` :P
< wumpus> right, that ^^ if anyone would be able to sneak a VM escape into the gitian descriptors in one of our main branches things would be really bad
< bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/1bc149d05b09...65435701efda
< bitcoin-git> bitcoin/master a99999c MarcoFalke: util: Add SetupHelpOptions()
< bitcoin-git> bitcoin/master 6543570 Wladimir J. van der Laan: Merge #15358: util: Add SetupHelpOptions()
< bitcoin-git> [bitcoin] laanwj merged pull request #15358: util: Add SetupHelpOptions() (master...Mf1902-Help) https://github.com/bitcoin/bitcoin/pull/15358
< provoostenator> Any idea why "make src/bitcoin-tx" works, but "make src/bitcoin-wallet" returns "No rule to make target"?
< provoostenator> I can't see any obvious differences in Makefile.am
< luke-jr> provoostenator: I don't see it in Makefile.am at all?
< luke-jr> note the relevant Makefile.am is NOT the one in src/
< provoostenator> luke-jr: how so? "bin_PROGRAMS += bitcoin-tx" is in src/Makefile.am too
< provoostenator> Ah but there's no BITCOIN_WALLET_BIN in /Makefile.am, like there is a BITCOIN_TX_BIN
< provoostenator> So there's something missing...
< wumpus> I don't get why this code does (Network)1 (Network)2 instead of directly using the NET_* constants: https://github.com/bitcoin/bitcoin/blob/master/src/qt/clientmodel.cpp#L279
< wumpus> provoostenator: yea there needs to be a rule $(BITCOIN_TX_BIN): FORCE
< wumpus> $(MAKE) -C src $(@F)
< provoostenator> I'll make a PR...
< wumpus> for the wallet tool
< provoostenator> It's either a subset of, or detour from, the headache I'm getting in #15382 because I added a UniValue dependency to system.{h,cpp}
< gribble> https://github.com/bitcoin/bitcoin/issues/15382 | WIP [util] add runCommandParseJSON by Sjors · Pull Request #15382 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] Sjors opened pull request #15388: [build] Makefile.am: add rule for src/bitcoin-wallet (master...2019/02/wallet_tool_make) https://github.com/bitcoin/bitcoin/pull/15388
< wumpus> this is really weird code too "(strProxy == strDefaultProxyGUI.toStdString()) ? ui->proxyReachIPv4->setChecked(true) : ui->proxyReachIPv4->setChecked(false);" I've never seen a tertiary operator used to switch between imperative statements
< wumpus> (which return void)
< wumpus> especially as they could have simply used ui->proxyReachIPv4->setChecked(strProxy == strDefaultProxyGUI.toStdString()) ?
< luke-jr> wumpus: IIRC, it was contributed by a new dev at the time
< luke-jr> so I wouldn't assume there are good reasons
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/65435701efda...03732f8644a4
< wumpus> ok-I'm slightly about about the lack of review then :)
< bitcoin-git> bitcoin/master 1cdb9bb Gregory Sanders: minor p2p_sendheaders fix of height in coinbase
< bitcoin-git> bitcoin/master 03732f8 MarcoFalke: Merge #14543: [QA] minor p2p_sendheaders fix of height in coinbase
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #14543: [QA] minor p2p_sendheaders fix of height in coinbase (master...p2p_sendheaders_height) https://github.com/bitcoin/bitcoin/pull/14543
< luke-jr> #11491
< gribble> https://github.com/bitcoin/bitcoin/issues/11491 | [gui] Add proxy icon in statusbar by mess110 · Pull Request #11491 · bitcoin/bitcoin · GitHub
< luke-jr> > Note that the network ids aren't just arbitrary numbers - they have meanings (defined in netaddress.h). -myself :P
< wumpus> heh
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/03732f8644a4...d8794a78a887
< bitcoin-git> bitcoin/master ae892ad Chun Kuan Lee: tests: accept unicode characters on Windows
< bitcoin-git> bitcoin/master 15b3103 Chun Kuan Lee: appveyor: Remove outdated libraries
< bitcoin-git> bitcoin/master d8794a7 MarcoFalke: Merge #13787: Test for Windows encoding issue
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #13787: Test for Windows encoding issue (master...test_u8path) https://github.com/bitcoin/bitcoin/pull/13787
< wumpus> this is why I sometimes ask in PRs wether all people think their comments were addressed, it can be hard to track
< bitcoin-git> [bitcoin] scravy opened pull request #15389: Remove unnecessary const_cast (master...patch-1) https://github.com/bitcoin/bitcoin/pull/15389
< promag> jnewbery: please see my comment in #15153
< gribble> https://github.com/bitcoin/bitcoin/issues/15153 | gui: Add Open Wallet menu by promag · Pull Request #15153 · bitcoin/bitcoin · GitHub
< promag> provoostenator: why cpp-subprocess instead of boost::process?
< promag> provoostenator: just saw the comments in the other issue
< bitcoin-git> [bitcoin] jnewbery opened pull request #15390: [wallet] Close bdb when flushing wallet. (master...wallet_flush) https://github.com/bitcoin/bitcoin/pull/15390
< bitcoin-git> [bitcoin] practicalswift opened pull request #15391: Add compile time verification of assumptions we're currently making implicitly/tacitly (master...assumptions) https://github.com/bitcoin/bitcoin/pull/15391
< promag> could we just enable #13339 for non-win builds for 0.18?
< gribble> https://github.com/bitcoin/bitcoin/issues/13339 | wallet: Replace %w by wallet name in -walletnotify script by promag · Pull Request #13339 · bitcoin/bitcoin · GitHub
< * instagibbs> just learned about --pdbonfailure, my life is a lie
< bitcoin-git> [bitcoin] jonasschnelli pushed 9 commits to master: https://github.com/bitcoin/bitcoin/compare/d8794a78a887...7d3f255316fc
< bitcoin-git> bitcoin/master 17abc0f João Barbosa: wallet: Factor out LoadWallet
< bitcoin-git> bitcoin/master ab288b4 João Barbosa: interfaces: Add loadWallet to Node
< bitcoin-git> bitcoin/master 32a8c6a João Barbosa: gui: Add openWallet and getWalletsAvailableToOpen to WalletController
< bitcoin-git> [bitcoin] jonasschnelli merged pull request #15153: gui: Add Open Wallet menu (master...2019-01-openwallet) https://github.com/bitcoin/bitcoin/pull/15153
< provoostenator> promag: the author of cpp-subprocess said he found boost::process implementation unpretty. I haven't studied it in enough detail yet to have a strong opinion. But aren't we trying to nuke Boost?
< luke-jr> provoostenator: we're trying to migrate from boost to C++11; if C++11 doesn't provide it, no reason to avoid boost for it
< luke-jr> (I'm not sure there's much left to migrate at this point either?)
< provoostenator> Ok, sounds like I'll have to study both implementations then :-)
< provoostenator> Assuming Boost does work with Windows that would be a good argument for it.
< jnewbery> Great to see #15153 merged. I think it could do with a bit more review (I plan to when I get a chance)
< gribble> https://github.com/bitcoin/bitcoin/issues/15153 | gui: Add Open Wallet menu by promag · Pull Request #15153 · bitcoin/bitcoin · GitHub
< luke-jr> has there been any discussion previously about the binary name for bitcoin-wallet? seems strange to me.
< promag> provoostenator: what luke-jr said. anyway, I don't have experience with boost::process, just think it's nice by reading the docs.
< provoostenator> luke-jr jnewbery: I was thoroughly confused by this naming today while debugging the Makefile, because that actualy contains "bitcoin-wallet" as an intermediate stage for the wallet itself.
< provoostenator> Might be better to call it wallet-tool
< jnewbery> Was changed from bitcoin-wallet-tool after discussion here: https://github.com/bitcoin/bitcoin/pull/13926#issuecomment-423326338
< provoostenator> In this case I actually thing the -tool suffix makes sense, to distinguish it from a live wallet.
< luke-jr> when we split the actual wallet out from the main code, what will we call its binary? :P
< provoostenator> Though I might be suffering from cognitive bias :-)
< provoostenator> luke-jr in #10102 that new binary is called bitcoin-wallet
< gribble> https://github.com/bitcoin/bitcoin/issues/10102 | [experimental] Multiprocess bitcoin by ryanofsky · Pull Request #10102 · bitcoin/bitcoin · GitHub
< luke-jr> …but…
< provoostenator> So wallet-tool makes sense for this tool. Or even offline-wallet or something. The live wallet in a split code base, which connects to a node, could then be called bitcoin-wallet.
< jnewbery> ryanofsky: thoughts ^ ?
< provoostenator> Ironically bitcoin-wallet was ryanofsky's idea in that comment :-)
< jnewbery> I currently have no strong opinion one way or the other (but think we should make a decision before 0.18 freeze)
< ryanofsky> i was just thinking that you have one binary called bitcoin-wallet that you use for all wallet stuff
< ryanofsky> not sure what advantage there would be to having separate bitcoin-wallet and bitcoin-wallet-tool binaries
< jnewbery> That seems like a reasonable suggestion to me. Merge wallet tool functionality into the new new bitcoin-wallet when it happens
< ryanofsky> yeah, i already implemented that (though can't remember if i pushed it to 10102 yet)
< provoostenator> Any volunteers for bumping QT to 5.5 as per #13478?
< gribble> https://github.com/bitcoin/bitcoin/issues/13478 | [RFC] gui: Minimum required Qt5 · Issue #13478 · bitcoin/bitcoin · GitHub
< jnewbery> so for now, keep the wallet tool as bitcoin-wallet, and when the separated bitcoin-wallet happens the tool commands will continue to work as expected?
< provoostenator> ryanofsky: that works for me as well. If the Makefile confuses someone again, we can also rename the intermediate bitcoin_wallet step.
< gmaxwell> FWIW, I actually misread PRs related to the wallet tool and thought it was named bitcoin-wallet-tool
< provoostenator> I think appveyor is on strike by the way.
< provoostenator> gmaxwell: it was renamed from bitcoin-wallet-tool to bitcoin-wallet along the way.
< bitcoin-git> [bitcoin] Sjors opened pull request #15393: Bump minimum Qt version to 5.5.1 (master...2019/02/qt-5_5) https://github.com/bitcoin/bitcoin/pull/15393
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/7d3f255316fc...ea022d9fd047
< bitcoin-git> bitcoin/master dc3b2cc Ben Carman: tests: Added missing tests for RPC wallet errors
< bitcoin-git> bitcoin/master ea022d9 MarcoFalke: Merge #15378: tests: Added missing tests for RPC wallet errors
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #15378: tests: Added missing tests for RPC wallet errors (master...tests_missing_tests) https://github.com/bitcoin/bitcoin/pull/15378
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/ea022d9fd047...d73918447faf
< bitcoin-git> bitcoin/master 8c9b8a3 Hennadii Stepanov: Replace script name with special parameter
< bitcoin-git> bitcoin/master d739184 MarcoFalke: Merge #15216: Scripts and tools: Replace script name with a special parame...
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #15216: Scripts and tools: Replace script name with a special parameter (master...20190120-improve-shell-ux) https://github.com/bitcoin/bitcoin/pull/15216
< promag> is this flacky?
< luke-jr> provoostenator: did you make a decision for subprocess mgmt?
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/d73918447faf...029d28a7aa56
< bitcoin-git> bitcoin/master 1e7f741 Gregory Sanders: remove some magic mining constants in functional tests
< bitcoin-git> bitcoin/master b651ef7 Gregory Sanders: submitheader: more directly test missing prev block header
< bitcoin-git> bitcoin/master 029d28a MarcoFalke: Merge #15238: [QA] remove some magic mining constants in functional tests
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #15238: [QA] remove some magic mining constants in functional tests (master...magic_miner) https://github.com/bitcoin/bitcoin/pull/15238
< hebasto> promag: hi! how can I push commit with fixed commit message only? -f does not work.
< promag> hebasto: really?
< dongcarl> hebasto: is the remote correct?
< promag> usually I commit --amend and then push -f
< promag> it works :/ maybe it's what dongcarl asked
< dongcarl> Like check what is considered your upstream, as that is what `push -f` tries to push to
< hebasto> my bad. it's ok now. thank you.
< dongcarl> :-)
< promag> hebasto: please fix pr title too
< hebasto> promag: is it ok now?
< promag> lgtm
< hebasto> thanks
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #15395: test: Remove TODO comments to remove -txindex option (master...Mf1902-qaNoTodo) https://github.com/bitcoin/bitcoin/pull/15395
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/029d28a7aa56...0d1160e42185
< bitcoin-git> bitcoin/master fa0ad4e MarcoFalke: RPCHelpMan: Check default values are given at compile-time
< bitcoin-git> bitcoin/master 0d1160e MarcoFalke: Merge #14918: RPCHelpMan: Check default values are given at compile-time
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #14918: RPCHelpMan: Check default values are given at compile-time (master...Mf1812-rpcOptionalCompile) https://github.com/bitcoin/bitcoin/pull/14918