< bitcoin-git>
[bitcoin] jonasschnelli closed pull request #10731: SanitizeString: Expand upon allowed characters in logging to include "!#%&*+=^{}~" (master...log_more_uacomment) https://github.com/bitcoin/bitcoin/pull/10731
< luke-jr>
jonasschnelli: how do we fix the current bug in Core then?
< luke-jr>
#10731 is a bug fix for a present issue
< gribble>
https://github.com/bitcoin/bitcoin/issues/10731 | SanitizeString: Expand upon allowed characters in logging to include "!#%&*+=^{}~" by luke-jr · Pull Request #10731 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] jonasschnelli reopened pull request #10731: SanitizeString: Expand upon allowed characters in logging to include "!#%&*+=^{}~" (master...log_more_uacomment) https://github.com/bitcoin/bitcoin/pull/10731
< jonasschnelli>
luke-jr: Your description: "Current Core strips out the !, + and = characters used by Knots to indicate whether BIP148 enforcement is enabled."
< jonasschnelli>
luke-jr: Can you describe the bug more precise?
< jonasschnelli>
I fail to see the bug in Core
< luke-jr>
jonasschnelli: 2017-07-13 07:37:02 receive version message: /Satoshi:0.14.2(BIP148)/Knots:20170618/: version 70015…
< luke-jr>
jonasschnelli: Core logs this regardless of whether it's !BIP148= or +BIP148 or !BIP148
< jonasschnelli>
Well.. why is this a bug in core?
< luke-jr>
because they are all valid UA characters
< jonasschnelli>
Yes. But invalid log prints.
< gmaxwell>
because you just say they are valid?
< gmaxwell>
They were never printed before.
< luke-jr>
they don't violate BIP 14
< jonasschnelli>
It's a log...
< luke-jr>
jonasschnelli: why?
< jonasschnelli>
luke-jr: BIP 14 is not about a log file
< luke-jr>
there's no reason logs shouldn't have those characters. most do. Core does other places.
< jonasschnelli>
Yes. I'm happy if you fix it. But don't break the SanitizeString assumption
< luke-jr>
there are far worse characters also allowed in SanitizeString
< gmaxwell>
BIP14 seems to also allow \n nul and so on
< gmaxwell>
IOW: bip-14 is brain damaged
< luke-jr>
gmaxwell: those are sane to forbid
< luke-jr>
what if SanitizeString turns ! into \! ?
< jonasschnelli>
No
< jonasschnelli>
It's not escaping
< jonasschnelli>
Its sanitizing
< gmaxwell>
Characters that will screw up shell processing and potentially lead to XSS in URL also are sane to forbid.
< luke-jr>
gmaxwell: we already allow many of those
< jonasschnelli>
Then make it better but not worse?
< gmaxwell>
I am tempted to say we should change the printing to hex just as we do with other potentially malicious network input.
< gmaxwell>
already people spam advertisements via UAs.
< luke-jr>
jonasschnelli: it's not a reasonable expectation. it's a log file, not a database query.
< luke-jr>
what other log files forbid characters for such reasons?
< gmaxwell>
why are we even logging these strings?
< jonasschnelli>
luke-jr: I'm fine about chaging the log print.. but not SanitizeString
< jonasschnelli>
SanitizeString and the log file are conceptually two things.
< luke-jr>
why is it when wumpus submits a PR doing this, it gets only ACKs and merged; yet when I do, people dig up obscure reasons to argue against it?
< gmaxwell>
among other reasons, the privacy arguments we have for not logging IPs also applies, esp with so much diversity in stupid values people are setting in these strhings.
< jonasschnelli>
luke-jr: Can you refere to wumpus's PR?
< gmaxwell>
Again, why are we logging this in debug.log by default anyways?
< gmaxwell>
We do not log IPs for privacy reasons.
< luke-jr>
gmaxwell: I don't know an answer to that. This issue affects the GUI as well, though.
< jonasschnelli>
luke-jr: If you think those chars should be visible in the log, maybe find a way as gmaxwell mentioned via hex-representation of some partially-invalid (they turn to hex)...
< gmaxwell>
luke-jr: I think it's a lot less of a concern in the GUI. I think your new characters are probably okay there.
< jonasschnelli>
GUI is isolated
< luke-jr>
so URL escape in debug log?
< luke-jr>
%xx
< gmaxwell>
jonasschnelli: % lets you escape arbritary characters for html/urls among other things.
< gmaxwell>
luke-jr: why not drop it from the debug log, and make the GUI more permissive?
< luke-jr>
gmaxwell: I de facto have a script parsing the debug log to analyse this
< gmaxwell>
though I question perhaps if we really should be showing these things in the gui.. not the characters, but third party sourced strings
< jonasschnelli>
gmaxwell, luke-jr: Hm.. yes. Maybe use the urlencode for the UA-string before printing to the log?
< gmaxwell>
UA: DANGER YOUR WALLET HAS BEEN COMPROMISED GO TO HTTP://fixwallet.eu/ NOW
< luke-jr>
:|
< jonasschnelli>
I kinda agree with gmaxwell that BIP 14 is BD
< jonasschnelli>
The original idea was also to place a donation bitcoin address there... :/
< sipa>
gmaxwell: the .eu makes it totally legit
< luke-jr>
lol
< jonasschnelli>
hahaha
< gmaxwell>
UA: OFFICIAL BITCOIN NOTICE. BITCOIN HAS BEEN REPLACED BY ULTRACOIN BUY REPLACEMENT COINS AT BITCOIN.COM NOW
< gmaxwell>
I mean this is why we don't try to do any fancy decoding of data stuffed in transactions either... :)
< sipa>
let's print the hash of the UA; we do that for almost all data already anyway
< gmaxwell>
similar risks exist in logs.
< luke-jr>
I thought that was to discourage fancy encoding <.<
< gmaxwell>
luke-jr: well advertisments in UA is also bad (which some people have been doing)
< gmaxwell>
sipa: we display hex of many things too.
< gmaxwell>
hex can be decoded but it's not going to trick anyone.
< luke-jr>
gmaxwell: that's not clear to me. how is it bad?
< gmaxwell>
luke-jr: spin up lots of sybil nodes just to get people to see DRINK MORE OVALTINE.
< luke-jr>
hmm
< luke-jr>
but people *don't* see it that much
< luke-jr>
if they do, it's more often than not a website that doesn't care what we do here
< gmaxwell>
(and god forbid people use the malicious messages like I suggested above, which they'd see in optimally bad situations: when they're wondering if their node is working right)
< gmaxwell>
luke-jr: yea, thats those websites problems (actually a lot of them filter that stuff)
< luke-jr>
should I make SanitizeString do the %xx escaping, or have a new function for this?
< gmaxwell>
FWIW I protested the UAs to begin with. Just for the record. :P
< gmaxwell>
nothing good ever comes from strings in protocols.
< luke-jr>
I know a protocol that uses strings for commands
< gmaxwell>
as far as url escaping the logs, that solves some problems not others... e.g. I think it doesn't make them necessarily safe for shell processing.
< luke-jr>
… sortof XD
< luke-jr>
gmaxwell: ?
< gmaxwell>
or maybe it does. gah, part of my complaint is that these sorts of changes aren't worth the security review time.
< gmaxwell>
luke-jr: re wladimir adding ( and ) in 2014: at least those characters are specifically named in the BIP.
< gmaxwell>
(they are also not ones that I've personally ever had bite me in data, I think)
< gmaxwell>
where ! % very much have.
< gmaxwell>
Lets only allow the characters BIP14 specifically mentions. :P
< sipa>
you're agreeing with a NACK, and then proceed to merge it?
< jonasschnelli>
sipa: Yes
< sipa>
(i have little opinion either way, just trying to follow what's happening)
< gmaxwell>
luke-jr: also, FWIW, I can promise it wasn't because it was you that I protested... I was into writing my opposition before noticing it was you that was pring it.
< jonasschnelli>
sipa: The best way to stop such PR is to merge them quickly. We can't close them.
< sipa>
why not?
< jonasschnelli>
Trolling and because it's an improvement (the space was missing there).
< jonasschnelli>
Closing because it may slow us down is somehow weak?
< jonasschnelli>
A quick merge seems to be the best path forward
< jonasschnelli>
"get it out of the way" style
< gmaxwell>
lets hope it didn't conflict any other PRs and cause needed rebasing.
< jonasschnelli>
gmaxwell: I checked that and it shouldn't
< jonasschnelli>
Maybe that something we can discuss in todays meeting. These typo-fix/add-spaced PRs need a clear strategy how to avoid them in the long run.
< jonasschnelli>
Maybe the quick merge is not ideal then
< jonasschnelli>
But if we want to close them (not merge them), then we should have a quick part in the PR guidlines
< sipa>
> Do not submit patches solely to modify the style of existing code.
< jonasschnelli>
luke-jr: looks good. But I guess the PR description could be a bit better (going to be in git history)
< luke-jr>
done
< luke-jr>
insofar as UA phishing, how hard would it be to add a red exclamation mark icon next to the UA string with a tooltip warning the user it's not to be trusted?
< luke-jr>
(otoh, maybe that would make the problem worse?)
< jonasschnelli>
ryanofsky: I like your def __idiv__(self, relative_uri): approach
< jonasschnelli>
Can you help me how I have to do this?
< bitcoin-git>
[bitcoin] ReneNyffenegger opened pull request #10814: Change type of op to agree with type of MAX_OPCODE. (master...MAX_OPCODE) https://github.com/bitcoin/bitcoin/pull/10814
< bitcoin-git>
[bitcoin] fanquake closed pull request #10814: Change type of op to agree with type of MAX_OPCODE. (master...MAX_OPCODE) https://github.com/bitcoin/bitcoin/pull/10814
< jonasschnelli>
jnewbery, ryanofsky: working on a rebase of 10650 with add points adressed
< jonasschnelli>
But git history is somehow fucked up... will fix soon
< jonasschnelli>
So I pushed the just rebased version now
< jonasschnelli>
Because the fixed has a strange history.. will fix soon
< jnewbery>
anything I can do to help?
< jonasschnelli>
jnewbery: no all good.
< instagibbs>
morcos, displaying wallet name is still useful in case you have multiple wallets in same datadir and you've forgotten which one you loaded :)
< jonasschnelli>
instagibbs you mean in getwalletinfo?
< instagibbs>
yeah
< jonasschnelli>
Yes. I think it's a must for getwalletinfo
< jonasschnelli>
Its how users can be sure they interacting with the right wallet
< instagibbs>
squinting at the hd key is the way I distinguish now
< jonasschnelli>
yeah. that works too
< morcos>
instagibbs: heh, i was joking about the listwallets feature.. but hopefully 10650 makes it!
< morcos>
it lookes like #10706 could use one more ack, but it's close. #10707 is then 2 simple commits on top of that. But I'd like opinon on the second commit
< jonasschnelli>
jnewbery: Yes. The PR is currently not working
< jonasschnelli>
jnewbery: which commit is relevant? I'd like to cherry pick (and keep you as the author)
< jnewbery>
that is (your PR #10650) MINUS (the authproxy commit) PLUS (my PR #10604 rebased on top) PLUS (ryanofsky's suggested authproxy change) PLUS (a functional test for multiwallet endpoints) PLUS (a hack in httprpc.cpp to get the test to pass)
< gribble>
https://github.com/bitcoin/bitcoin/issues/10604 | [wallet] [tests] Add listwallets RPC, include wallet name in `getwalletinfo` and add multiwallet test by jnewbery · Pull Request #10604 · bitcoin/bitcoin · GitHub
< morcos>
Actually 10418 can just be closed, although it should be addressed in the release notes
< bitcoin-git>
[bitcoin] morcos opened pull request #10816: Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (master...multiwallet_parameters) https://github.com/bitcoin/bitcoin/pull/10816
< wumpus>
10815 is crazy, when did that start to happen? I'm sure it was ok on 0.14 :/
< wumpus>
we really need tests for the init sequence, it's too easy to mess it up
< morcos>
wumpus: i don't know.. i've been noticing for a while now that occasionally i don't seem to be able to get RPC credentials.. what causes that aspect of the problem?
< morcos>
don't know if those other cases were related to this problem, its kind of easy to miss the core dumps when you expect it to not start properly anyway
< morcos>
by the way, general request for anyone who has open PR's please check whether your 0.15 milestone status is correct or not. would be nice to focus on the right things and make sure we don't miss anything important.
< wumpus>
well if it continues after not being able to lock the data directory it could do all kinds of things, such as deleting files that are used by the running instance
< sipa>
it seems 0.15 will use around 1.4x less CPU for validation than 0.14.2 (with infinite dbcache, before assumevalid point)
< wumpus>
great!
< sipa>
and at the tip, it's probably even more due to tx validation cache
< sipa>
wumpus: the whole type punning thing is complicated
< morcos>
negative CPU usage sounds awesome!
< sipa>
morcos: haha
< wumpus>
sipa: I'm sometimes thinking about giving up on C++ completely, I really can't keep up anymore
< sipa>
wumpus: in C11, type punning through a union is officially supported, and it's ambiguous in C99
< sipa>
but in C++, there seems to be nothing in the standard to indicate that it is legal
< wumpus>
everyone uses it though, we were all told to use it, after punning through a pointer was no longer allowed
< wumpus>
now this is no longer allowed either?
< sipa>
however, it's been supported in actual compilers since forever
< sipa>
it was never allowed by standards, but in practice everyone used it, so compilers support it
< wumpus>
so is this a question of language lawyering or a practical problem?
< sipa>
in this PR, it's purely theoretical... i gave 3 reasons why it's not a problem :)
< wumpus>
if everything supports it in practice and all code out there uses something, then it's very hard to argue avoiding it
< morcos>
#10235 should be milestoned 0.15 but is ready for merge
< morcos>
i don't want to miss string freeze, so just want to be sure we all agree on what the final API should look like so i can make sure commits are updated
< wumpus>
morcos: sure, it's good to use a consistent name over multiple calls, if it is used for the same thing
< wumpus>
and if it hasn't been in any release it's ok to change it
< morcos>
yep only issue is the only one in stable release is using the wrong convention confTarget instead of conf_target. make all the new ones wrong, or just allow that old one to stick around unchanged.
< wumpus>
also the string freeze doesn't apply to RPC help or api, because it's not translated
< wumpus>
it's just for GUI messages _(...) and tr(...)
< morcos>
nblocks has been in release, but already changing estimatesmartfee anyway based on the unstable warning
< morcos>
oh...
< morcos>
whew, ok
< cfields>
sipa: why not use aggregate initialization: prevector() : _size(0), _union{{}}
< cfields>
or give the union a ctor?
< luke-jr>
yeah, string freeze is to give translators time ;)
< sipa>
cfields: oh?
< sipa>
cfields: which field of the union does that initialize?
< cfields>
sipa: the first
< sipa>
cfields: go comment on the PR
< luke-jr>
unions can have a ctor? :o
< sipa>
luke-jr: yup, and other methods
< sipa>
(but not virtual ones)
< cfields>
sipa: sure. I'm not seeing the warning without the change, though.
< morcos>
gmaxwell: ^ probably too late, but was waiting on my other PR's to move forward, its still depending on 10706
< bitcoin-git>
[bitcoin] laanwj opened pull request #10818: init: Keep track of whether data directory locked, don't cleanup if not (master...2017_05_locked_datadir) https://github.com/bitcoin/bitcoin/pull/10818
< jtimon>
since it seems #8498 cannot be priority for some reason that scapes me, what about #10757 from me ?
< gribble>
https://github.com/bitcoin/bitcoin/issues/8498 | Near-Bugfix: Optimization: Minimize the number of times it is checked that no money... by jtimon · Pull Request #8498 · bitcoin/bitcoin · GitHub
< gmaxwell>
I think if we do not fix the restore we need to disable HD by default. The current situation can pretty easily lead to funds loss.
< morcos>
but yes i also agree we need to clean up the 0.15 milestone list
< wumpus>
cfields: bumped to 0.16
< cfields>
thanks
< luke-jr>
gmaxwell: that would be very confusing to users, since older versions have HD
< jnewbery>
wumpus: yess please remove 10711
< jonasschnelli>
I can work on the HD restrore. But It's pretty complex with pruning / encrypted wallets... the PR is already large and will get bigger...
< jonasschnelli>
If there is enough review power, we can try for 0.15
< gmaxwell>
E.g. just pick up a walled you'd previously saved, rescan won't move the keypool forward, and you'll end up missing transactions (then discarding wallets with money), and handing out addresses to people you already gave to other people and misattributing payments.
< jonasschnelli>
I can have it overhauled by tuesday
< wumpus>
jnewbery: done
< jonasschnelli>
gmaxwell: Yeah.. is also true for all other wallets with gap limits of 5 (most do)
< jonasschnelli>
We should def. do better
< luke-jr>
can just the wallet-format-touching parts of HD restore be prioritised? eg, move out the actual restoring logic?
< sipa>
jonasschnelli: this is not true for anything that automatically tops up the keypool
< jonasschnelli>
What about just provide HD restore for non-pruning (to reduce the size)?
< jonasschnelli>
sipa: most HD wallets in the wilde stop topping the window futher up if a gap of >5< keys where found
< sipa>
jonasschnelli: yes
< gmaxwell>
jonasschnelli: to that extent that thats true at least in other cases those wallets behaviors are well documented and the interfaces is built around them, they're also used almost exclusively for personal use, rather than industrial use... (and it's not completely true because if there isn't a long gap they do handling it right and we do not)
< sipa>
jonasschnelli: but we don't top up at all
< jonasschnelli>
sipa: Yes. Not saying that is better. :)
< sipa>
jonasschnelli: and hd split makes it worse, because it risks reusing a key that was previously used as change as a payment address
< jonasschnelli>
I just wanted to re-state the HD restore in general is a broken thing
< jonasschnelli>
Okay. Then we have more time.. :)
< gmaxwell>
Right now this is responsible for several serious bugs in our behavior, which regressed vs the past, and will predictable result in funds loss through several different vectors. I don't see an easy workaround to prevent exposure-- I thought perhaps refusing to load a wallet if the tip doesn't match the chain tip, but thats too cumbersome and disruptive.
< jonasschnelli>
Since we have great reviewers, .. I'm convinced we get it in
< morcos>
10240 (when ready) is an example of something that should also be on high-priority... it's going to take some review time and its important to get in (in addition to 0.15 milestone)
< jonasschnelli>
Okay. I though it not going to make it for 0.15 thats why I moved focus away.. but I see the issue now better
< jonasschnelli>
*thought
< jnewbery>
jonas: anything I can do to help for 10240? Would you like me to rebase it?
< wumpus>
ok, will add 10240
< jonasschnelli>
jnewbery: Please take over if you can
< sipa>
jonasschnelli: rescanning beyond your pruning depth should already be an issue? what do we do in that case?
< morcos>
achow101: what about the signrawtransaction splitting stuff, is that still aimed for 0.15?
< jnewbery>
sure. I'll take it
< jonasschnelli>
sipa: the PR halts validation
< gmaxwell>
jnewbery: You are now my personal hero for the day.
< sipa>
jonasschnelli: no, i mean right now
< sipa>
what do we do if we try to rescan beyond the prune depth
< achow101>
morcos: I'd like it to be. and the validateaddress stuff as that is related to #7965
< sipa>
so, i think pruning is not relevant for 10240
< gmaxwell>
the rescan calls just say no if you try that.
< sipa>
it's a problem right now if you rescan beyong the pruning depth, and it remains so
< jonasschnelli>
A large part of 10240 is about haling the full node in pruning... dropping that would reduce the review workload
< sipa>
jonasschnelli: i think it should stop regardless of pruning
< jonasschnelli>
So. Drop the pruning option from 10240?
< sipa>
it's crazy that your wallet would go out of sync with your node
< sipa>
that's a totally unsupported state right now
< jonasschnelli>
From the PR on encrypted wallets:
< jonasschnelli>
Same as above, but, If we hit the gap limit with an encrypted wallet, we can't topup the keypool. In that case, we just pause the sync (not the node, only the wallet).
< sipa>
maybe that can be enabled later, once the wallet is more independent from the node
< sipa>
but i think 10240 should just stop sync entirely if your wallet is encrypted and the keypool runs out
< jonasschnelli>
sipa: well, if you use a backup wallet you have the same state
< sipa>
rhavar: not now, meeting
< instagibbs>
rhavar, ask again in 45 min :P
< sipa>
jonasschnelli: at startup; not anymore after rescan
< sipa>
during normal operation the wallet is always in sync with the node
< jonasschnelli>
Yes. Thats true
< * luke-jr>
wonders if a halted node will rewind based on headers
< jonasschnelli>
All that because of hardened derivation!
< sipa>
it's also easy to avoid; using 10000 keys in the keypool
< gmaxwell>
(indeed, which I also keep recommending)
< jonasschnelli>
You don't avoid it, you just make the timespan for the possible impact smaller
< sipa>
okay
< jonasschnelli>
And 10000 is just inefficient
< sipa>
well, i think all of that isn't the priority now
< jonasschnelli>
What about only allowing non-hardened derivation for encrypted wallets and disable all pkey export calls?
< sipa>
for 0.15, we need to have automatic marking of seen keys
< sipa>
jonasschnelli: yes, i like that, but not 0.15
< jonasschnelli>
Okay. jnewbery will focus on 10240 (he will rebase and overhaul I guess)
< gmaxwell>
jonasschnelli: we have a program that requires >1GB ram, runs best with >8GB ram, that does hours of processing just to start up-- I don't think worrying about 320k of key material is a major concern.
< sipa>
awesome; let's discuss further on the 10240 PR
< gmaxwell>
(also 1000 works too, it 10k is really too much.)
< jonasschnelli>
gmaxwell: should not be a concern. But it's still an inefficient fix for the problem we have
< jonasschnelli>
sipa: ack. Thanks jnewbery
< gmaxwell>
Inefficient compared to what? Inefficient to taking away private key export? In efficient compared to even one moment of one users time?
< jonasschnelli>
gmaxwell: Inefficient compared to support pub key derivation for encrypted keys or to topup the keypool on the fly
< gmaxwell>
jonasschnelli: are you actively trying to sabotage the project?
< sipa>
gmaxwell: please
< jonasschnelli>
?
< sipa>
jonasschnelli: there are good reasons to support hardened and unhardered derivation both; adding a feature is not a substitute for fixing a problem we have
< jonasschnelli>
gmaxwell: was that a joke or a serious question?
< sipa>
jonasschnelli: using non-hardened derivation implies you'll need to have a big keypool; it comes with that design choice
< gmaxwell>
Sorry to be rude, but I am just gobsmacked about aruging that setting the keypool to be big "on the order of tens or hundreds of kilobytes" is opposed compared to this long saw about public derrivation; which we aren't doing for the wallet at least now.
< gmaxwell>
So it seems to me like that you're intentionally in broken directions because you disagree with another decision.
< gmaxwell>
er intentionally pushing in
< jonasschnelli>
I though avoiding keypool with non-hardened derivation may be seen as a benefit for some of the users.. but it seems that i'm wrong. But at least it's not intentional sabotage
< morcos>
let's move on from this at least duing the meeting, i think we all agree that the keypool can be bigger than 200 regardless of otehr chnages we make
< gmaxwell>
Do we? it keeps getting argued against.
< morcos>
thats why i ended it by saying we all agree. :)
< sipa>
well, having non-hardened derivation with disabled key export is a perfectly fine _feature_ - but it's not usable for everyone (some people need key export), and for those users, we'll need to be able to deal with hardened derivation
< sipa>
so let's do that
< sipa>
next topic
< instagibbs>
ack
< jtimon>
NicolasDorier: how does #9728 interact with rescan ?
< luke-jr>
I very much dislike passing wallet by name. That just makes the GUI side ugly
< jonasschnelli>
luke-jr: you mean the selecting walltes by name?
< sipa>
luke-jr: as opposed to what? (sorry, i'm not up to date)
< luke-jr>
sipa: as opposed to passing a CWallet* on the JSONRPCRequest
< sipa>
that seems like something that's easy to change later
< luke-jr>
jonasschnelli: the GUI would have to go CWallet* -> string -> CWallet*
< luke-jr>
and hope it matches the right one up
< luke-jr>
sipa: I suppose, yes
< wumpus>
yes, indeed, can we avoid long discussions about small details that don't matter for correctness?
< wumpus>
we really want this in before the feature freeze
< wumpus>
so let's be pracical about it
< gmaxwell>
<3
< jonasschnelli>
Yes. WalletID or similar can be done later.
< wumpus>
yep
< jonasschnelli>
One thing that is a bit cumbersome is that you have to remove the -wallet argument from bitcoin-cli when calling a non wallet command
< jonasschnelli>
The endpoint node/wallet split is not very practical from the -cli use perspective
< wumpus>
well it makes some sense
< luke-jr>
hmm, bitcoin-cli reads bitcoin.conf, doesn't it? how does that interact? :/
< gmaxwell>
then make the cli command handle that internally?
< gmaxwell>
also, we can take some clunkyness with this expiremental feature in 0.15.
< jonasschnelli>
gmaxwell: Yes. I though of that.
< gmaxwell>
e.g. fix cli later.
< jonasschnelli>
Yes. Sure.
< jonasschnelli>
If you want to use multiwallet now, you need to add/remove -wallet when fiddling with -cli
< wumpus>
IMO a clean separation between wallet and non wallet commands is good
< luke-jr>
fixing cli could mean changing the -wallet= to something else
< wumpus>
even if it seems cumbersome in the beginning
< luke-jr>
jonasschnelli: how do yuo remove it, if it's in bitcoin.conf?
< instagibbs>
luke-jr, yeah, something that means "use wallet" not "load wallet"
< jonasschnelli>
Yes. Its good.
< jonasschnelli>
luke-jr: ? I can't follow
< jtimon>
bitcoin-cli calls with -wallet that don't need it could just ignore the extra argument and spit a warning somehwere or something?
< wumpus>
I mean it'd be easy to put every non-wallet command on wallet endpoints as well, but that's something that is awfullly hard to change later
< luke-jr>
jonasschnelli: typical multiwallet use case has wallet=abc.db \n wallet=def.db in bitcoin.conf
< sipa>
jonasschnelli: there are a bunch of things that the wallet - even after separation - will need access to (like fee estimates, mempool, ...)... i think it's fine if those remain inside the v1/wallet API (and also accessible as node commands)
< jonasschnelli>
wait.. that's actually a good point!
< luke-jr>
jonasschnelli: bitcoin-cli will get these options too.
< jonasschnelli>
Yes.. haven't tested that. :/
< luke-jr>
instagibbs's -usewallet or similar seems like a good solution
< instagibbs>
it likely takes the first wallet arg and uses that
< jonasschnelli>
Yes. I think -usewallet is better
< sipa>
so i think i may agree with having pretty much everything available through the wallet endpoint
< instagibbs>
luke-jr, something like that, if it's not complicating something else
< sipa>
(but not getinfo) *ducks*
< gmaxwell>
It's an expiremental feature, API isn't table. The endpoint can be leaky for now.
< luke-jr>
it's also easier to collapse args later than to split them, if we end up regretting it
< jonasschnelli>
I guess we can leave it for now we just need to mark the /v1 *EXPERIMENTAL* in the release notes
< instagibbs>
yes please
< sipa>
ack
< gmaxwell>
s/table/stable/
< gmaxwell>
And I think the alternative is to not have it at all, which isn't preferable.
< wumpus>
should do that anyway
< wumpus>
absolutely
< wumpus>
as long as it doesn't cause regressions for single-wallet mode
< wumpus>
that would be unacceptable - but everything new is experimental
< sipa>
agree
< jonasschnelli>
I guess user feedback will also help us how to extend this further
< wumpus>
indeed
< jonasschnelli>
But once 10650 is in, we have finally usable multiwallet in Core! That's a big step.
< sipa>
jonasschnelli: it is!
< jonasschnelli>
And it wasn't the first try.
< wumpus>
hehe multiwallet slipped so many releases it's a shame
< jtimon>
yeah, is a nice feature to anounce in 0.15, even if as experimental
< sipa>
just to repeat some other happy results briefly: my reindex-chainstate to 450k with infinity dbcache runs about 40% faster on master than on 0.14.2
< petertodd>
sipa: what do you mean by "infinity dbcache"?
< jtimon>
wow
< wumpus>
which reminds me, someone should really write a release notes section about all the wonderful perf improvements in 0.15
< sipa>
petertodd: dbcache sufficient for the entire utxo sets
< gmaxwell>
wumpus: I've already been talking to drak about some blog posts and whatnot on it.
< wumpus>
everything from crc instruction support in leveldb to the new and better database formats, to faster validation, etc
< petertodd>
sipa: ah, cool!
< sipa>
wumpus: don't forget tx validation caching; that's massive for performance at the tip
< instagibbs>
^
< wumpus>
sipa: +1
< sipa>
(just somewhat harder to benchmark and give cool numbers for)
< sipa>
petertodd: around 6-8GB in 0.15, in practice (and more in 0.14.2, due to the blowup at flushing time)
< petertodd>
sipa: that's still pretty small fortunately :)
< gmaxwell>
Was 2GB not that long ago.
< petertodd>
gmaxwell: be interesting to know why UTXO growth has stopped temporarily...
< rhavar>
That model works, just give shit results
< sipa>
in a previous life, i've used minizinc
< rhavar>
Since the ideal solution is blindly obvious, can you can convince minizinc to pick it `constraint count_selected_optional_outputs = 1;`
< rhavar>
But it doesn't find it on it's own
< rhavar>
But it shows that the ideal solution doesn't violate any constraints (and has a lower cost than the one it picks)
< rhavar>
I'm kind of wondering if the problem is just formatted in a stupid way or not
< rhavar>
(in general, I can pretty much always out-perform that model by hand. Makes me think something is seriously fundamentally screwed up)
< jtimon>
one could also argue that simply usage has been reduced in fear of a possible fork
< rhavar>
I got like 10-15 emails yesterday from people freaking out after the bitcoin.org notice lolz ><
< rhavar>
(which imo was reckless and alarmist. If they did enough mental gymnastics to convince themselves that bip148 would split the chain, they should have at least waited to be sure that segwit2x isn't going to activate in time)
< jtimon>
I guess that further disproves the argument that "people that will support uasf already know about bip148 already know about it and already 'upgraded', waiting for bip149 won't result in more people or businesses on board"
< jtimon>
sorry, getting offtopic...
< rhavar>
For every 1 person who understands the technical details of forks and chain splits etc. there are 1000 who don't
< clarkmoody>
rhavar I've done some constrained optimization stuff, but never with minizinc
< clarkmoody>
I've found that you can make a constraint solver do anything you like depending on the objective function
< rhavar>
If you can see why that model doesn't pick the obvious solution (the one you can force it to), I'd be happy to give you a $100 in bitcoin or something :P
< rhavar>
(It's obviously really easy to hack it, so it picks the obvious solution. But i'm more interested to why it sucks in general, like if the problem is phrased badly or something)
< clarkmoody>
Could you enumerate the combinations and print cost for each, then see where on the scale the solver is choosing? Like is it the worst, or near the best, etc?
< rhavar>
Well in this simple example, it's the 2nd best solution or something
< rhavar>
but considering it's a trivial example that you can do by hand in under a minute -- i think it's pretty embarrassing :P
< clarkmoody>
I know the feeling :-)
< rhavar>
There's also cases it say are not solvable, that are trivial (pick all the inputs, for instance)
< rhavar>
I actually have had some moderate success by splitting the model into two different problems (1 pick coins without change) and 1 pick coins always using change
< rhavar>
and then compare which is better
< rhavar>
But I feel like there must be something wrong at a more fundamental level if you need to do that
< jtimon>
wait, #10714 didn't solved the warning...
< gribble>
https://github.com/bitcoin/bitcoin/issues/10714 | Avoid printing incorrect block indexing time due to uninitialized variable by practicalswift · Pull Request #10714 · bitcoin/bitcoin · GitHub
< jtimon>
oh, yes, it does, sorry
< instagibbs>
what could cause "no response from server"? It connects but gets no response?
< instagibbs>
oh, likely my -cli not matching bitcoind (testing multiwallet endpoints)
< instagibbs>
jonasschnelli, it seems I get "trapped" if I specify a wallet in my conf file, unable to use -cli to stop the node, complains that the call is trying to use a /node/ endpoint call
< instagibbs>
so yeah we really need something other than "wallet" for -cli usage
< jtimon>
do we have a list of new rpc calls so far for 0.15 ?