< bitcoin-git> [bitcoin] achow101 opened pull request #10874: [RPC] getblockchaininfo: Loop through the bip9 soft fork deployments instead of hard coding (master...getblockchaininfo-bip9-loop) https://github.com/bitcoin/bitcoin/pull/10874
< bitcoin-git> [bitcoin] achow101 opened pull request #10875: BIP 91 deployment parameters (master...bip91-dep-params) https://github.com/bitcoin/bitcoin/pull/10875
< bitcoin-git> [bitcoin] DJMorgan opened pull request #10876: What do you mean a bug in the latest version? (0.14...master) https://github.com/bitcoin/bitcoin/pull/10876
< gmaxwell> ::sigh::
< bitcoin-git> [bitcoin] sipa closed pull request #10876: What do you mean a bug in the latest version? (0.14...master) https://github.com/bitcoin/bitcoin/pull/10876
< bitcoin-git> [bitcoin] kallewoof opened pull request #10877: [rpc] Verbose flags for chaining and scripting (master...verbose-flagging) https://github.com/bitcoin/bitcoin/pull/10877
< bitcoin-git> [bitcoin] dongcarl opened pull request #10878: Docs: Fix markdown line breaks in init.md (master...patch-1) https://github.com/bitcoin/bitcoin/pull/10878
< bitcoin-git> [bitcoin] kallewoof opened pull request #10879: Difficulty adjustment (master...difficulty-adjustment) https://github.com/bitcoin/bitcoin/pull/10879
< bitcoin-git> [bitcoin] kallewoof closed pull request #10879: Difficulty adjustment (master...difficulty-adjustment) https://github.com/bitcoin/bitcoin/pull/10879
< bitcoin-git> [bitcoin] eklitzke opened pull request #10880: Update .gitignore to ignore more test files (master...gitignore) https://github.com/bitcoin/bitcoin/pull/10880
< eck> what's the recommended way for me to test a change before submitting a PR? is "make check" sufficient?
< gmaxwell> eck: no, read test/README.md
< eck> thanks
< gmaxwell> test/functional/test_runner.py
< gmaxwell> is what you want to run in addition to a make check.
< gmaxwell> of course it depends on what you're doing, for some things make check is sufficient... or just a particular one of the functional tests.
< gmaxwell> but if you don't know, better to run them all.
< gmaxwell> Our CI setup will run them on whatever you submit, but if it fails you'll still need to be able to run them locally to figure out what happened, so its best to get in the habbit of it.
< bitcoin-git> [bitcoin] eklitzke opened pull request #10881: trivial: fix various pyflakes/vulture warnings (master...vulture) https://github.com/bitcoin/bitcoin/pull/10881
< bitcoin-git> [bitcoin] jonasschnelli pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/9e8d6a3fb43a...a6ec5802b0a9
< bitcoin-git> bitcoin/master e0d4592 practicalswift: Avoid redundant redeclaration of GetWarnings(const string&)...
< bitcoin-git> bitcoin/master a6ec580 Jonas Schnelli: Merge #10864: Avoid redundant redeclaration of GetWarnings(const string&)...
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #10864: Avoid redundant redeclaration of GetWarnings(const string&) (master...GetWarnings-in-warnings.h) https://github.com/bitcoin/bitcoin/pull/10864
< jonasschnelli> Oh. I guess I should not have merged this because of the freeze.
< jonasschnelli> Sry
< jonasschnelli> I can't reproduce the following travis issue locally (by using Ubuntu 14.04 and the same build configuration): https://travis-ci.org/bitcoin/bitcoin/jobs/254826520#L2299
< jonasschnelli> Maybe someone has an idea why I get a "/usr/include/c++/4.8/debug/safe_iterator.h:279:error: attempt to dereference a singular iterator."
< cfields> jonasschnelli: that one uses everything built with extra debugging defines
< jonasschnelli> cfields: D_GLIBCXX_DEBUG, right?
< cfields> jonasschnelli: -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC
< cfields> make -C depends DEBUG=1
< jonasschnelli> Ah.. the dependencies...
< jonasschnelli> cfields: Just passing -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC into ./configure won't be "enought"?
< cfields> jonasschnelli: iirc all the deps have to be built with those flags otherwise wonky things happen
< cfields> but i could be misremembering
< jonasschnelli> okay. Let me try that...
< jonasschnelli> (via depends)
< cfields> jonasschnelli: those errors are almost always foo.empty() && foo[0]
< jonasschnelli> cfields: accessing an index that is not available?
< cfields> jonasschnelli: ah, looks like a "singular" iterator is one that can't be reached.
< cfields> so probably more like *foo.end()
< jonasschnelli> cfields: Thanks... I'll check the code.
< wumpus> re: #10873 we really need to add a comment, this isn't the first time someone has tried to replace the signal handling with a mutex
< gribble> https://github.com/bitcoin/bitcoin/issues/10873 | Use a condition variable for shutdown notifications by eklitzke · Pull Request #10873 · bitcoin/bitcoin · GitHub
< wumpus> this won't work for handling UNIX signals, certainly not in a portable way
< jonasschnelli> wumpus: Yes. How long was it ago since I tried it. :)
< jonasschnelli> I didn't commented because I tried with a boost signal rather then a mutex
< jonasschnelli> Wasn't sure if this is portable or not
< cfields> jonasschnelli: the list of things you're actually allowed to use is remarkably small
< jonasschnelli> cfields: Yes. I once checked... atomics should be fine... but I guess conditional vars aren't
< wumpus> yes, setting a flag is fine
< wumpus> I think you are allowed to read/write pipes as well
< wumpus> there are some tricks that could be taken over from other daemons, but it might not be worth the trouble
< wumpus> maybe there's even some obscure trick with sigwait() that would work
< bitcoin-git> [bitcoin] laanwj pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/a6ec5802b0a9...9022aa37226b
< bitcoin-git> bitcoin/master b138585 Alex Morcos: Remove factor of 3 from definition of dust....
< bitcoin-git> bitcoin/master f4d00e6 Alex Morcos: Add a discard_rate...
< bitcoin-git> bitcoin/master 9022aa3 Wladimir J. van der Laan: Merge #10817: Redefine Dust and add a discard_rate...
< bitcoin-git> [bitcoin] laanwj closed pull request #10817: Redefine Dust and add a discard_rate (master...discardmore) https://github.com/bitcoin/bitcoin/pull/10817
< promag> is practicalswift around?
< jonasschnelli> jnewbery, ryanofsky: continue the discussion about -wallet vs -usewallet here?
< jonasschnelli> I can't follow the argument of having -wallet for both use-cases (define which wallet to use and for adding a wallet to the daemon).
< jonasschnelli> Why would you want the use same argument for two different things?
< jonasschnelli> In the daemon case, you define a wallet to add to the sync process. For cli, you want to define which of those availavle wallets you want to use (1:n)
< ryanofsky> jonasschnelli, maybe you can give a practical example of why having different argument names is useful
< ryanofsky> obviously command line arguments can have different meanings on different commands. if you add --help or --verbose to a unix command it will do different things depending on the command
< jonasschnelli> ryanofsky: a) because its to different things (and users should learn that from the beginning), b) -usewallet could be reused for bitcoin-Qt (until there is a GUI way to select the wallet)
< jonasschnelli> "two
< jnewbery> jonasschnelli, for the reasons I gave in https://github.com/bitcoin/bitcoin/pull/10868#issuecomment-316409725 - I think the name usewallet is meaningless and confusing
< ryanofsky> can you explain qt use case a little more? qt runs a full node & wallet
< jonasschnelli> Users now need to learn that they can run multiple wallets by providing multiple of the same -wallet arguments (it's already kind of confusing, most users would expect a comma separator argument)
< jonasschnelli> ryanofsky; Right now, Qt always uses the wallet at index 0
< jonasschnelli> I think having then -wallet for CLI, they would need to learn that in one case they specify multiple wallets while in the other case they specify a single wallet... seems confusing to me
< ryanofsky> right, so what do you need a -usewallet option for in context of qt?
< jonasschnelli> ryanofsky: Qt: until we have a wallet selection via GUI, you could define which of those wallets is accessible over Qt.
< ryanofsky> jonasschnelli, right now with qt, you already have it serve multiple wallets, and you can choose which wallet will be shown in the display by putting the wallet first. there is no need for -usewallet
< ryanofsky> adding -usewallet for qt would again just be adding confusion & complexity for no reason
< jonasschnelli> Yes. Agree that this would work, though do we really want to depend on orders of arguments?
< ryanofsky> no, we want to add a dropdown or tab
< jonasschnelli> Yes. Indeed.
< ryanofsky> i'm saying adding "usewallet" is not clarifying, it is just throwing in more complexity
< jonasschnelli> Yes. I'm not sure if the -usewallet case for Qt is valid... though I think it is for cli
< jonasschnelli> It is already complex.
< ryanofsky> -wallet=filename everywhere should just work
< jonasschnelli> So users need to know that in one case (daemon) it's the key to "have multiple" while in the other case (cli) is the key to reduce it from multiple to one?
< ryanofsky> nobody objects that "cp" and "mv" both take "--help" or "--verbose" arguments because verbose output is different in both cases, it's just a weird argument
< jonasschnelli> --help is always help. Not once setting the -help information and once reading it
< ryanofsky> users do need to know that daemon accepts multiple -wallet= arguments to load multiple wallets. i'm not sure why that is a problem
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/9022aa37226b...d445a2c2eaa1
< bitcoin-git> bitcoin/master 1c9b818 Andrew Chow: getinfo deprecation warning
< bitcoin-git> bitcoin/master d445a2c Wladimir J. van der Laan: Merge #10857: [RPC] Add a deprecation warning to getinfo's output...
< ryanofsky> users should just be shown an error if they are expecting bitcoin-cli to send an single rpc command to multiple wallets
< bitcoin-git> [bitcoin] laanwj closed pull request #10857: [RPC] Add a deprecation warning to getinfo's output (master...deprecate-getinfo) https://github.com/bitcoin/bitcoin/pull/10857
< jonasschnelli> It's just my opinion. I can live with the pure -wallet for everything approach.
< ryanofsky> sounds good to me
< jnewbery> I think the error message in my PR can be improved, but russ is correct - the error should inform the user that they need to specify -wallet on the command line
< jonasschnelli> I wonder what other developers think...
< jonasschnelli> jnewbery: but that is how it currently works (just with -usewallet), right?
< ryanofsky> pr needs reviews, so i also want other developers to chime in :)
< jonasschnelli> I just can't follow why you would want to add code to differenciate -wallet in bitcoin conf to -wallet passed into bitcoin-cli
< ryanofsky> that is a drawback. if you want bitcoin-cli to be able to use wallet filename from a config file, you have to call the option something other than -wallet
< jnewbery> but how -usewallet is interpreted is differentiated as well - acted on by bitcoin-cli, ignored by bitcoind, and you haven't decided on whether it should be ignored or not by bitcoin-qt
< ryanofsky> reason i think tradeoff is worth is is that -wallet=filename means our tools do not silently ignore bad wallet filenames, and there is weirdness and option interactions to have to think about
< jonasschnelli> Maybe something we should discuss at this weeks IRC meeting
< ryanofsky> i'd prefer pr reviews & acks but ok :)
< jnewbery> I agree. I think Russ is the only one who's reviewed/tested the code so far. But happy to discuss further at the meeting
< sipa> jonasschnelli: i though the same way
< sipa> jonasschnelli: but it's not so different from how -rpcport for bitcoind means "the port to listen on" nd for bitcoin-cli means "the port to connect to"
< sipa> the ignoring of arguments in the config file is weird... but it is (almost) just an implementation detail... in practice you always get the expected behaviour
< sipa> evwn if it were implemwnted differentlt
< wumpus> I don't think re-using -wallet for bitcoin-cli is a good idea
< wumpus> re-using rpcport is clever because the setting on client and server side happens to match up w/ the interface in between
< wumpus> if it'd be possible to specify multiple rpcports in the server config, we'd have the same problem there
< wumpus> -rpcbind is allowed to be used multiple times, but isn't used by the client at all
< wumpus> e.g. a multi-option on the server side shouldn't be a single option on the client side, given we parse the same configuration file
< wumpus> <ryanofsky> nobody objects that "cp" and "mv" both take "--help" or "--verbose" arguments because verbose output is different in both cases, it's just a weird argument <- yes, but cp and mv don't read their arguments from a configuration file
< wumpus> e.g. you might want to set a default wallet for bitcoin-cli to use in the configuration file, you can't use -wallet for that
< wumpus> if bitconi-cli read a different configuration file I'd have agreed there was no conflict
< wumpus> but that's not where we're coming from, or even where we're going
< ryanofsky> wumpus, yes, the tradeoff for having bitcoin-cli accept a wallet argument is having to ignore wallet values from the config file
< wumpus> that sounds stupid
< wumpus> why would you be so insistent in using -wallet for the option to special case it in config file reading?
< wumpus> just use a different argument name and be done with it
< ryanofsky> ok, well that's the tradeoff, we've discussed what i think are better options for allowing default wallets
< ryanofsky> i'm not insistent on anything
< wumpus> what is the trade-off? why would you insist on using -wallet at all?
< wumpus> it causes a conflict so imo the option should not be named that
< ryanofsky> i'm just saying what i think, which is that using different arguments is confusing, and being able to read wallet from config file is not actually useful compared to better options
< wumpus> making a special workarounds just to be able to call it -wallet is strange
< wumpus> if bitcoin-cli would not read the config file at all, or a different config file, I'd be ok with it
< ryanofsky> ok, well that's my strange argument :)
< wumpus> but we can't just do that, certainly not for 0.15
< ryanofsky> if you object, then go with usewallet and wallet
< wumpus> special-casing just seems weird
< wumpus> I really dislike it
< wumpus> 'why would all options be read from the config file except for X?' that's just drunk
< jnewbery> having a `useargument` argument that is used by bitcoin-cli but is not used by bitcoind seems strange to me
< wumpus> why owuld that be strange? bitcoin-cli has lots of arguments that are not used by bitcoind
< wumpus> why would they need to overlap?
< ryanofsky> yeah, i just think this wallet config file thing is a strange corner case compared to everyday usage
< wumpus> what would bitcoind want to do with it ta all?
< jnewbery> Because of the arguments I give here: https://github.com/bitcoin/bitcoin/pull/10868#issuecomment-316409725
< wumpus> it'd be the default wallet for the *client*
< sipa> if the '-use' sounds liie meaningless, use -rpcwallet maybe
< wumpus> sipa: yes! sounds good to me
< ryanofsky> i'd be fine with rpcwallet, that's what i suggested when the arg was first added
< instagibbs> rpcwallet is nicer
< wumpus> I don't really care what name, it should just not be -wallet
< wumpus> though I guess rpcwallet would be more consistent with some other rpc options
< jnewbery> I still think ryanofksy's point about protecting against users putting in bad -wallet arguments is valid, but -rpcwallet is definitely a step up from -usewallet
< ryanofsky> maybe also consider making it an error for user to specify -wallet on bitcoin-cli command line if it will be ignored
< wumpus> multiple -rpcwallet should also be an error
< sipa> ryanofsky: ?
< wumpus> ryanofsky: that'd result in an error as well when it's specified in the config file
< ryanofsky> yes that's why i said "on bitcoin-cli command line"
< jnewbery> wumpus: depends where you put the check
< wumpus> then again, when bitcoind runs in multiwallet mode, it won't accept missing wallet
< ryanofsky> or don't, anyway i was just suggesting that because i think -wallet -rpcwallet are easy to confuse
< wumpus> so you *have* to run -cli with rpcwallet
< wumpus> right?
< ryanofsky> you wouldn't need -rpcwallet if you are making a non-wallet rpc call or your node only has one wallet loaded
< wumpus> yes, and that's fine
< jnewbery> I think Russ is thinking about the case where someone starts bitcoind with -wallet=mybusinesswallet.dat in the conf file, then calls `bitcoin-cli -wallet=mypersonalwallet.dat send ...` and is confused that money is sent from their business wallet even though they specified a different wallet
< ryanofsky> yes, i'm just said that in response to "so you *have* to run -cli with rpcwallet
< wumpus> we don't distinguish command line and config options, and imo that should stay that way
< jnewbery> not true - rpcport is an example
< wumpus> how?
< sipa> it has a different meaning for client and server, sure
< sipa> but we don't complain in bitcoin-cli that -dbcache is specfied, right?
< jnewbery> oh sorry - yes you're right
< sipa> i guess there is one example of an option that is treated differently between cmdline and configfile... -conf
< jnewbery> and I'd expect #10267 to be treated differently on command-line versus .conf file, but again I may be in the minority
< gribble> https://github.com/bitcoin/bitcoin/issues/10267 | New -includeconf argument for including external configuration files by kallewoof · Pull Request #10267 · bitcoin/bitcoin · GitHub
< jnewbery> >we don't complain in bitcoin-cli that -dbcache is specfied
< jnewbery> but specifying the wrong -dbcache on the command line for bitcoin-cli cannot lead to loss of funds
< sipa> i think that's unlikely here
< gmaxwell> Wrong testnet parameter can, in the same way wallet parameters can. (much worse, in fact)
< gmaxwell> I've been saved multiple times from doing something phenominally dumb re testnet vs mainnet through using wallet encryption as an authorization step.
< wumpus> yes, having separate configuration files per network would make a lot of sense too
< gmaxwell> (I like sharing them for most parameters, in fact...)
< sipa> wumpus: oh tes
< sipa> yes
< wumpus> jnewbery: yes, -conf is somewhat of an exception too, it's not explicitly treated differently but in effect it is because -conf in a configuration file does nothing, it's parsed before
< wumpus> gmaxwell: yes, keeping a shared one is ok too
< wumpus> doesn't have to be either or
< wumpus> as long as the precedence order is well-defined
< wumpus> especially for things like network configuration it's almost essential have different config files per network, e.g. as soon as you specify rpcbind or bind with explicit port you'll run into port conflicts otherwise
< bitcoin-git> [bitcoin] jnewbery opened pull request #10882: [WIP] Keypool topup (master...keypool_topup) https://github.com/bitcoin/bitcoin/pull/10882
< jnewbery> 10882 is the cut-down version of #10830 which sipa and gmaxwell were asking for in the last meeting. I'm still fixing the testcase, but I'd appreciate some code review if possible
< gribble> https://github.com/bitcoin/bitcoin/issues/10830 | [WIP] [wallet] keypool restore by jnewbery · Pull Request #10830 · bitcoin/bitcoin · GitHub
< jnewbery> I think people wanted this in 0.15 as a bugfix
< instagibbs> jnewbery, will review
< jnewbery> thanks instagibbs
< bitcoin-git> [bitcoin] jnewbery closed pull request #10830: [WIP] [wallet] keypool restore (master...pr10240) https://github.com/bitcoin/bitcoin/pull/10830
< jonasschnelli> I just did a quick scrollback read. Is -rpcwallet still in talk?
< jonasschnelli> I'd prefere to keep rpc out of the naming...
< jonasschnelli> isn't bitcoin-cli transport agnostic?
< jonasschnelli> (from the users interface perspective)
< sipa> what do you mean transport agnostic/
< jonasschnelli> I probably over-engineer. But what if we once change bitcoin-cli's transport layer?
< jonasschnelli> the rpc layer is hidden from the user
< jonasschnelli> -rpcwallet includes the used transport layer in the naming scheme.
< sipa> so?
< sipa> so does -wallet
< jonasschnelli> ?
< sipa> i may misunderstand what you're saying
< jonasschnelli> -(rpc)wallet
< sipa> bitcoin-cli is an rpc client
< sipa> the transport layer may change from TCP to Unix sockets or something
< sipa> but it'll still be an RPC client
< jonasschnelli> What if we change to a different IPC protocol?
< jonasschnelli> I probably over-enginee. Nm.
< sipa> then everything needs to change; including rpcport, rpcserver, ...
< jonasschnelli> I just though until now, I could not see any "rpc" in bitcoin-cli (from the users perspetive)
< jonasschnelli> Yeah. That.. so yes. nm then.
< jonasschnelli> Okay. I take back my argument. With -rpcserver, etc. -rpcwallet may make sense.
< gmaxwell> what is -rpcwallet precisely
< jonasschnelli> gmaxwell: I guess it would be the substitute for -usewallet
< gmaxwell> is it the setting that tells bitcoin-cli what wallet to talk to
< sipa> gmaxwell: suggested new name for -usewallet
< gmaxwell> okay
< sipa> yes
< jonasschnelli> What was the issue with -usewallet?
< sipa> people think use is a weasel word
< sipa> (i really don't care anymore)
< jonasschnelli> Yeah, I don't care. But please not -wallet
< gmaxwell> gowallet=foo :P
< sipa> ./bitcoin-cli -thethingimean=foo
< gmaxwell> enablewallet=bob
< gmaxwell> walletify=bob
< sipa> ok...
< gmaxwell> openwallet=info
< jonasschnelli> Among those options, I would still prefere "usewallet"
< morcos> wumpus: I don't care too much what we call the argument name, and I think we should just decide so we can make the behavior nice around it (my vote would be wallet, rpcwallet, usewallet in that order, but I don't care)
< morcos> I would say though that I think we should break the conf file and command line option thing being treated equally no matter what
< morcos> If we use wallet as the -cli option, then we break it as per jnewbery's pull. If we use rpcwallet or usewallet, then i think it makes sense to flag an error if -wallet is given on the command line.
< morcos> I agree with ryanofsky that it is possible people will make that mistake, and I don't think we should let them semi-reasonably think they are specifying one wallet and have things happen to another
< wumpus> if it wasn't for the authentication options we could stop parsing bitcoin.conf completely for the -cli
< wumpus> that would be fine with me, I don't want to parse one option from .conf but not another though, exceptions like that always result in confusion and wtf in the longer run
< morcos> We should also let it be an error if there are multiple instances of (rpc/use)wallet which begs the question of what do we do if you put (rpc/use)wallet in the conf file because clearly you need to be able to override from the command line
< wumpus> I'm not convinced it should be treated otherwise than other options
< morcos> Meaning the last value controls?
< wumpus> yes
< wumpus> if you use -rpcport you don't expect it to connect to all ports at once, do you?
< wumpus> not sure why -rpcwallet would be different
< morcos> no but i expect someone to tell me i'm an idiot
< wumpus> if so,the whole command line/config parsing should be refactored, whih should probably happen at some time, including errors if you provide an unrecognized option
< wumpus> however we're not there, and we're not going to be there for 0.15
< wumpus> I think any special-casing we introduce now for rpcwallet is a bad idea, especially as the whole API is still supposed to be experimental in the first place
< morcos> hmm, is the code to figure out whether something is a multiarg vs a single arg shared btwn bitcoind and -cli
< morcos> i guess it must not be?
< morcos> oh
< morcos> never mind
< morcos> if its different names it doesnt matter
< wumpus> all the argument parsing code is shared
< wumpus> itis' in util.cpp
< morcos> (rpc/use)wallet is a single arg, wallet ais a multi arg
< wumpus> but just use a different name so it doesn't matter
< wumpus> right.
< ryanofsky> i mostly like john's pr because i think it gets rid of all the ways to shoot yourself in the foot. and i don't think the special casing is a big deal. but if it is, then obvs different approach is needed
< wumpus> I do think the special casing is a big deal
< wumpus> but I have to maintain the code over a long time
< morcos> well lets just pick and be done with it, any will be fine..
< wumpus> and I'm sick of all kind of weird special casing
< morcos> rpcwallet it is.. although i admit i'm hesitant about specifying multiple rpcwallets and having the last one control.. but i don't know an easy solution to that
< wumpus> it might seem easier on the short term but on the long time, arbitrariness is going to confuse
< wumpus> both developers and users
< wumpus> but it's always 'hey, why not add another special case'
< jnewbery> I don't even consider it special casing. Command line overrides conf file, just as for other arguments
< wumpus> noo of course not
< morcos> but you're right a later clean up could allow for all rpc options, you can only have a second single arg if its the command line overriding the conf file
< jnewbery> it's implemented differently because it's not possible to fully override mapmultiargs
< wumpus> if you do it it's not a special case
< jnewbery> but that's the functional outcome
< ryanofsky> wumpus, these arguments just seem pretty abstract to me, and i wish you'd bring them down to concrete examples of how the actual code in the pr could cause problems
< wumpus> I don't feel like repeating myself
< wumpus> I already argued against reusing -wallet forbitcoin-cli specifically
< ryanofsky> happy to defer
< wumpus> it's strange to have a same argument name as single-option for one utility then for multi-option for another utility
< morcos> so arguably the only thing to change from the merged code is s/usewallet/rpcwallet/ if that would overall be preferred?
< wumpus> *if* htey parse the same configuration file
< wumpus> yes
< wumpus> rpcwallet is a better name
< jonasschnelli> Should we still revoke (halt bitcoin-cli) if one provides multiple -rpc/-usewallet arguments?
< wumpus> and I'm ok with making providing -wallet on the command line for -cli an error, though I think it's overthinking it
< wumpus> jonasschnelli: no, I don't think so
< jonasschnelli> So just use the last one?
< wumpus> jonasschnelli: just like multiple -rpcport is not an error
< wumpus> unless we change that behavior on a global level
< jonasschnelli> wumpus: Okay. Fine by me
< wumpus> so if multiple of any non-multi argument is an error, it should be an error, obviously
< wumpus> but there's no need to suddenly do all kinds of crazy stuff for rpcwallet
< jonasschnelli> Yes. No special case for every argument type
< jonasschnelli> But I guess what we should merge for 0.15 is the Qt console support: https://github.com/bitcoin/bitcoin/pull/10870
< jonasschnelli> Right now, Qt console is useless in multiwallet
< wumpus> yes
< wumpus> I certainly think making the functionality work in the first place is important :)
< wumpus> tagged it
< jonasschnelli> thanks
< wumpus> from what I see "if an argument is specified multiple times, the last value holds" seems to be pretty standard behavior, I don't know of many command line utilities that reject if you specify a flag multiple times
< eck> I am trying to understand the BIP process, and I'm confused how I can tell which BIPs were actually deployed
< eck> e.g. is BIP 62 deployed?
< wumpus> it can even be useful in scripts etc, to be able to override something that is set before
< ryanofsky> GetArg implement first value holds, right?
< wumpus> ryanofsky: ugh, okay
< instagibbs> eck, #bitcoin (can answer there)
< jonasschnelli> eck: look at the overview. Bip62 is "Withdrawn"
< wumpus> ryanofsky: are you sure?
< ryanofsky> no
< ryanofsky> which is why i like these things to be errors :)
< wumpus> but every other comamnd line utiltiy I know uses 'last setting holds'
< jonasschnelli> But then it should be globally?
< wumpus> gcc, ld, ls, etc
< bitcoin-git> [bitcoin] morcos opened pull request #10883: Rename -usewallet to -rpcwallet (master...rpcwallet) https://github.com/bitcoin/bitcoin/pull/10883
< morcos> wumpus: sigh.. it chooses the first option given in the conf file unless you give it on the command line and then its the last one from the command line
< jonasschnelli> :/
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #10867: Allow only a single -usewallet argument (master...2017/07/multiwallet_bitcoincli) https://github.com/bitcoin/bitcoin/pull/10867
< wumpus> yes, it seems to correctly chose the latest from the command line
< wumpus> .conf semantics are strange
< jonasschnelli> but why the first from the conf?
< jonasschnelli> that makes no sense..
< wumpus> no, it doesn't
< wumpus> it should be the other way aorund
< wumpus> for all options, not special casing one :)
< jonasschnelli> yes.
< jonasschnelli> Though changing it now could have unexpected consequences. :/
< morcos> that behavior is what preseves command line overriding right?
< wumpus> morcos: yes, indeed, in the silly way it works currently
< jonasschnelli> morcos: IMO the command line overriding is good. But the last config value should be takend before we go to the optional command line overriding
< jonasschnelli> however, we should not change that.
< wumpus> in any case there are probably a zillion things broken in command line / config file handling, nothing we should do about that for 0.15, but for later on it makes sense to actually think about it
< wumpus> especially with things like includeconf=..., people are going to expect later config options are overriding previous ones
< wumpus> like it is for any other config file parser in the world
< jonasschnelli> heh. Indeed
< wumpus> how do other programs handle multi-args? I think our way is strange, in that we can never clear something that has been set, things are only added
< wumpus> so if the config file sets bind=127.0.0.1:1234, the command line can never override that, only add to it
< jonasschnelli> I'm not sure, but isn't comma separation a thing? -wallet=w1.dat,w2.dat?
< wumpus> yeah
< wumpus> though that always leaves 'what if we have a comma in our value'
< jonasschnelli> Use ;
< wumpus> but that conflicts with the shell :-)
< jonasschnelli> heh.. yes. use -wallet="a;b;c"?
< jonasschnelli> But meh
< wumpus> hehe
< wumpus> I was just illustrating a point, I know it's hard to work around
< wumpus> json syntax config file would work
< wumpus> for command line it's harder
< jonasschnelli> Yes
< wumpus> I must say it wouldn't be the first time I typed -debug=tor,bench though, comma-separated is kind of natural, when it works
< jonasschnelli> Yes. For the wallet it would feel more natural and there is no need for a comma in the filename, though there could be other use cases where you may want/need it
< wumpus> cool, it warns at least now: Warning: Unsupported logging category -debug=rpc,net,tor.
< wumpus> suppose that is since the debug categories are an enum
< wumpus> it used to be silent about it
< jnewbery> quick question about -rpcwallet in .conf: does that mean that server RPCs are also sent to the /wallet/<rpcwallet> endpoint? That's allowed in the current implementation, but it'll break when we do wallet process separation
< jonasschnelli> BTW: github recommends to add a COC to the bitcoin core project: https://opensource.guide/code-of-conduct/
< wumpus> jnewbery: I'd expect -rpcwallet would mean commands are sent to a wallet endpoint, yes, -cli doesn't have the knowledge whether something is a wallet call or not
< jnewbery> I suppose we'll generally need to give bitcoin-cli more smarts if we do wallet process separation
< wumpus> well ... we'll worry about that, then
< sipa> well my view is that the 'wallet process' would still be a full bitcoind-like thing communicating over SPV... so it would still have P2P connections (typically just one), etc
< wumpus> there's no way to specify an alternative endpoint at all at the moment
< sipa> so there's not much reason why you couldn't send a getpeerinfo to a wallet endpoint
< sipa> which is exactly what it does now
< sipa> for a few things, like mempool or fee estimation something else may be needed
< sipa> but that's a worry for later indeed
< wumpus> heh yes if a wallet endpoints getpeerinfo you can send getpeerinfo to it
< wumpus> if it doesn't, you'll get an error that it's not implemented
< jonasschnelli> sipa: Curious, could you think that – if we have p2p enc/auth – mempool and fee est would be something to serve over p2p to auth. peers?
< jonasschnelli> Or do you see a different channel (RPC, etc,) for that?
< sipa> jonasschnelli: maybe... that may be controversial
< sipa> but i don't see much problem in having private extensions available only for known peers
< jonasschnelli> I'd say that would simplify connection management massively. But I'd also say some people would dislike that.
< jonasschnelli> Yes. me two
< sipa> actually, i think that was one of the uses for -whitelist when i suggested :)
< jonasschnelli> *too
< jonasschnelli> But thats far in future (net refactor, Bip151, Bip150 and maybe then).
< sipa> yah
< wumpus> I agree authenticated extensions could make sense, please don't overload -whitelist with more things
< sipa> wumpus: oh, absolutely - i agree with that now
< sipa> just giving some historical background
< wumpus> oh sure, it's one kind of whitelisting option
< bitcoin-git> [bitcoin] eliahuhorwitz opened pull request #10884: Update build-windows.md (master...patch-1) https://github.com/bitcoin/bitcoin/pull/10884
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/d445a2c2eaa1...df0793f324e3
< bitcoin-git> bitcoin/master 7ec3343 Gregory Sanders: add gdb attach process to test README
< bitcoin-git> bitcoin/master df0793f MarcoFalke: Merge #10681: add gdb attach process to test README...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #10681: add gdb attach process to test README (master...gdbattach) https://github.com/bitcoin/bitcoin/pull/10681
< promag> is this correct bitcoin-qt -wallet=foo.dat -wallet=wallet.dat -regtest ?
< promag> to run multiwallet in qt?
< sipa> yes
< promag> sipa: what should be the load order when -wallet=foo -wallet=bar -wallet=foo
< sipa> ha, no clue
< sipa> does that even work?
< promag> yeah
< promag> :P
< promag> I'm submitting a PR to handle that
< sipa> cool
< promag> anyway, I have 2 ideas: 1st add second argumento unique = false to ArgsManager::GetArgs
< promag> the other is to add GetUniqueArgs
< promag> which preserves order (first takes effect)
< cfields> i think the arg manager itself should just sort that out
< cfields> would we ever care about dupes post-init?
< promag> however ParseParameters explicit says that the last takes effect when one only wants the arg as a single value
< cfields> promag: yes, that's the common convention. so that you can add things later in the line to disable earlier ones
< cfields> s/disable/override/
< promag> cfields: right so it's config < arg[n] < arg[m] where m > n
< promag> but in the case of multi value then the order is? config[0], config[1], arg[0], arg[1] right?
< cfields> that's what i'd expect, yes
< promag> would we ever care about dupes post-init? -> dunno, wdyt?
< sipa> i thinkit should just error if you soecify the same one twice
< sipa> sorry, ohone typing
< promag> sipa: and if you config[1] = arg[0] ?
< promag> I mean, if you pass a value in the command line which happens to be in config?
< promag> is that a error?
< cfields> right, i would expect it to just quietly drop the dupe
< promag> the error/warn should be just for the command line args right?
< promag> so it's just a matter of fixing ParseParameters
< sipa> ok, merging them is also a reasonable thing to fo
< sipa> is it *always* the right thing?
< cfields> sipa: you mean for all args? or for wallet in particular?
< promag> dunno, didn't go that far
< sipa> cfields: for all args
< sipa> if you're talking about changing ParseParameters
< promag> for instance, -connect
< cfields> sipa: well at minimum, i think we always want to be able to override the config
< cfields> and i think that always implies merging?
< sipa> cfields: multiargs don't get merged, they get appended
< promag> if it's multiarg then imo it's unique stable merge
< sipa> i'm not convinced that for all options uniquifying is the right thing to do
< cfields> yea, ok...
< cfields> -addnode=127.0.0.1 -addnode=127.0.0.1
< cfields> i want 2 connections there
< promag> does that make sense?
< promag> if that's the case, GetUniqueArgs is the way to go?
< sipa> or just unique it in the wallet loading code
< sipa> at least if you're talking about 0.15
< promag> it's in 3 different places
< promag> the PR adds GetUniqueArgs and changes 3 occurencies of gArgs.GetArgs("-wallet")
< promag> and adds tests for GetUniqueArgs
< promag> sipa, cfields: what should GetArg("-foo") give when -foo=a -foo=b -foo=a? (for me last "a" takes effect even though it's duplicate)
< sipa> yes, that's intentional
< sipa> last option takes effect
< promag> but GetUniqueArgs("-foo") gives {"a", "b"} sgty?
< sipa> sure