< jtimon> random data point: as of height 475560, it takes 157.7 GB of storage (using -txindex)
< bitcoin-git> [bitcoin] sipa pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/479afa0f8486...e4fcbf797ed3
< bitcoin-git> bitcoin/master 1e3a320 practicalswift: Simplify "!foo || (foo && bar)" as "!foo || bar"
< bitcoin-git> bitcoin/master e4fcbf7 Pieter Wuille: Merge #10780: Simplify "!foo || (foo && bar)" as "!foo || bar"...
< bitcoin-git> [bitcoin] sipa closed pull request #10780: Simplify "!foo || (foo && bar)" as "!foo || bar" (master...redundant-condition) https://github.com/bitcoin/bitcoin/pull/10780
< bitcoin-git> [bitcoin] jonasschnelli pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/e4fcbf797ed3...7666250ffb4e
< bitcoin-git> bitcoin/master 69a4339 Lawrence Nahum: missing white space in function arg
< bitcoin-git> bitcoin/master 7666250 Jonas Schnelli: Merge #10810: missing white space in function arg...
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #10810: missing white space in function arg (master...missing_white_space) https://github.com/bitcoin/bitcoin/pull/10810
< 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?
< luke-jr> ‎[07:57:03] ‎<‎jonasschnelli‎>‎ luke-jr: I'm fine about chaging the log print.. but not SanitizeString <-- what do you mean by this?
< gmaxwell> I didn't ack wumpus; but as far as yours went, because you included a bunch of red-flag characters that I've personally been burned by.
< jonasschnelli> luke-jr: () wumpus, yours: !#%&*+=^{}~
< 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?
< 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> 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> Oh... I should read more.
< jonasschnelli> What about typo fixes?
< jonasschnelli> So yes. My quick merge was wrong.
< luke-jr> does #10731 look acceptable now?
< gribble> https://github.com/bitcoin/bitcoin/issues/10731 | Escape rather than remove any printable characters in UAs by luke-jr · Pull Request #10731 · bitcoin/bitcoin · GitHub
< 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
< ryanofsky> jonasschnelli, does suggested code in https://github.com/bitcoin/bitcoin/pull/10650#discussion_r123484826 not work?
< ryanofsky> maybe it needs /v1/ string added
< 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
< ryanofsky> actually should probably be __truediv__ not idiv: https://docs.python.org/3/reference/datamodel.html?highlight=truediv#object.__truediv__
< jnewbery> jonasschnelli: what's the current status with 10814? Are you expecting to rebase/address feedback today?
< jnewbery> sorry, not 10814, 10650
< jnewbery> #10650
< gribble> https://github.com/bitcoin/bitcoin/issues/10650 | Multiwallet: add RPC endpoint support by jonasschnelli · Pull Request #10650 · bitcoin/bitcoin · GitHub
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/10706 | Improve wallet fee logic and fix GUI bugs by morcos · Pull Request #10706 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/10707 | Better API for estimatesmartfee RPC by morcos · Pull Request #10707 · bitcoin/bitcoin · GitHub
< morcos> All this needs to go in before string freeze
< jonasschnelli> jnewbery or ryanofsky
< jonasschnelli> i could not get that authproxy endoint thing running
< jonasschnelli> I guess python doesnt like me...
< jonasschnelli> Anyone of you willing to provide a commit for 10650?
< jnewbery> jonasschnelli: I'll give it a go now
< jonasschnelli> perfect. thanks
< jnewbery> I've got authproxy working and written a test, but any requests to /v1/wallet/<wallet> get 404s
< jnewbery> I think because RegisterJSONEndpoint() isn't called anywhere?
< jnewbery> jonasschnelli ^
< 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/10650 | Multiwallet: add RPC endpoint support by jonasschnelli · Pull Request #10650 · bitcoin/bitcoin · GitHub
< 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
< jnewbery> 4597818... is the authproxy commit
< jonasschnelli> jnewbery: Ah.. is was looking for that one: https://github.com/jnewbery/bitcoin/commit/4597818e43900dc07271773f544f47d1da6d635c
< jonasschnelli> Thanks.. will finish 10650 soon. best to do hand off for now (until I'll comment its rebased)
< jonasschnelli> *hands
< jnewbery> ok, ping me if you need anything else. It'd be great to get this ready for final review/merge before the meeting
< jonasschnelli> jnewbery with your AuthProxy thing I get also a 404
< jonasschnelli> But bitcoin-cli works.. so the endpoint must be registered
< jnewbery> can you push your latest branch and I'll test?
< jnewbery> suggest you tcpdump and compare the requests from bitcoin-cli and authproxy
< jonasschnelli> ah.. found!
< jonasschnelli> path='//v1/wallet/w0/'
< jonasschnelli> it should be AuthServiceProxy("{}{}" (for the __truediff__ op)
< ryanofsky> shouldn't you just write / "path" instead of / "/path"?
< jnewbery> ryanofsky: right. That's what's in the test I provided https://github.com/jnewbery/bitcoin/commit/53be2f334979f2cd979554fe81c4837ad02328cd
< jnewbery> doesn't matter either way, as long as you're consistent
< ryanofsky> / "path" is more consistent with python path api
< jonasschnelli> jnewbery, ryanofsky: I just pushed the new version.. now its ready for a review
< jonasschnelli> I think usually its better to wait for a 2nd rebase until the author said its overhauled... :)
< jonasschnelli> But I guess Github sends those code-push mails.
< morcos> Can someone please remove 0.15 milestone from: #8501 and #10418 and add 0.15 milestone to: #10815
< gribble> https://github.com/bitcoin/bitcoin/issues/8501 | Add mempool statistics collector by jonasschnelli · Pull Request #8501 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/10418 | Need fee estimation migration for 0.15 · Issue #10418 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/10815 | bitcoin-qt core dumps when bitcoin(d/-qt) is already running · Issue #10815 · 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
< gribble> https://github.com/bitcoin/bitcoin/issues/10235 | Track keypool entries as internal vs external in memory by TheBlueMatt · Pull Request #10235 · bitcoin/bitcoin · GitHub
< wumpus> unless there is an official way of doing it, of course, which is better, then we should move to that, but it's tiring
< wumpus> there's always something that is UB lately
< wumpus> c++ sucks
< sipa> wumpus: yes, you're always allowed to access a type's byte representation through a char pointer
< wumpus> but we don't want to access the type's byte representation
< wumpus> it was to initialize a pointer in a structure
< sipa> yes, but the wrong one :)
< wumpus> or do you want to use offset_of and clear the byte range?
< sipa> the compiler _incorrectly_ thinks that the indirect pointer may be used uninitialized
< sipa> this is not possible, the size field is used as a union tag
< wumpus> morcos: the problem in 10815 is that it does a shutdown, even though the init didn't complete
< sipa> we're trying to silence it by initializing that pointer, even though the other union field is the only one that's being used
< wumpus> morcos: so it shuts down the HTTP server, which deletes your authentication cookie etc, even though it was never started
< wumpus> sipa: bleh
< sipa> wumpus: forget i brought it up; there are plenty of reasons why it should be fine
< morcos> wumpus: and what causes core dump?
< wumpus> morcos: g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
< morcos> 10799 can be tagged 0.15 too
< wumpus> morcos: probably disconnecting something that was never connected in the first place
< morcos> hmm.. that sucks
< morcos> while i have you, can i have your opinion on another RPC agument name change issue, this time all messed up solely by me: https://github.com/bitcoin/bitcoin/pull/10707#issuecomment-314869251
< 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.
< sipa> cfields: neither do it
< sipa> *do i
< bitcoin-git> [bitcoin] morcos opened pull request #10817: Add a discard_rate to avoid small change (master...discardmore) https://github.com/bitcoin/bitcoin/pull/10817
< 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
< luke-jr> It is time.
< gmaxwell> Neat: luke-jr found a way to abuse the coinbase api to create a sybil resistant poll based on coinbases' KYC process: https://luke.dashjr.org/programs/kycpoll/answers.php
< petertodd> gmaxwell: nice
< achow101> meeting?
< gmaxwell> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr btcdrak sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier
< instagibbs> hi
< wumpus> #startmeeting
< jonasschnelli> Hi
< lightningbot> Meeting started Thu Jul 13 19:01:40 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< morcos> i'm here for 30 mins
< petertodd> hi
< luke-jr> I'm here until I pass out <.<
< jtimon> hi
< wumpus> proposed topics?
< paveljanik> hi
< achow101> possible topic: 0.15 feature freeze
< achow101> it's soon, july 16th
< cfields> hi
< jtimon> review begging as first topic ?
< wumpus> PSA: https://github.com/bitcoin/bitcoin/issues/9961, feature freeze is sunday (which is an awful day, it'll probably be monday in practice)
< wumpus> #topic high priority for review
< achow101> everything marked as 0.15?
< achow101> :p
< luke-jr> we can delay the feature freeze to July 16th, 2018 to avoid a Sunday
< kanzure> hi.
< morcos> #10711 can be removed from high-priority as its not for 0.15
< gribble> https://github.com/bitcoin/bitcoin/issues/10711 | [tests] Introduce TestNode by jnewbery · Pull Request #10711 · bitcoin/bitcoin · GitHub
< wumpus> agree with that jnewbery?
< morcos> Please add #10706 to high priority i guess, since the PR's it was depending on were merged
< gribble> https://github.com/bitcoin/bitcoin/issues/10706 | Improve wallet fee logic and fix GUI bugs by morcos · Pull Request #10706 · bitcoin/bitcoin · GitHub
< morcos> i think i asked him, but he stepped away
< jonasschnelli> I'm removing the 0.15 milestone from #10240 (will def. not make it)
< gribble> https://github.com/bitcoin/bitcoin/issues/10240 | Add HD wallet auto-restore functionality by jonasschnelli · Pull Request #10240 · bitcoin/bitcoin · GitHub
< sipa> jonasschnelli: no
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/10757 | RPC: Introduce getblockstats to plot things by jtimon · Pull Request #10757 · bitcoin/bitcoin · GitHub
< jonasschnelli> sipa: you want that in?
< sipa> jonasschnelli: if we remove that from 0.15, we must revert the hd split
< wumpus> I tend to agree with achow101 - it's better to use the 0.15 tag now for high priority for review
< instagibbs> sipa, ?
< jonasschnelli> proposed topic then: Hd split / hd restore
< cfields> #9566 can be removed from 0.15
< gribble> https://github.com/bitcoin/bitcoin/issues/9566 | threading: use std::chrono for timestamps by theuni · Pull Request #9566 · bitcoin/bitcoin · GitHub
< morcos> wumpus: i think its still helpful to distinguish between hope for 0.15 and really need
< 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)?
< jtimon> mhm, only #10652 in project 8...
< gribble> https://github.com/bitcoin/bitcoin/issues/10652 | Small step towards demangling cs_main from CNodeState by TheBlueMatt · Pull Request #10652 · bitcoin/bitcoin · GitHub
< 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
< sipa> making you miss it as incoming payment
< jonasschnelli> So what should we do?
< sipa> fix it
< sipa> #10240 is a bug fix
< gribble> https://github.com/bitcoin/bitcoin/issues/10240 | Add HD wallet auto-restore functionality by jonasschnelli · Pull Request #10240 · bitcoin/bitcoin · GitHub
< jonasschnelli> Agree
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/7965 | Remaining instances of ENABLE_WALLET in `libbitcoin_server.a` · Issue #7965 · bitcoin/bitcoin · GitHub
< jonasschnelli> sipa: I don't know: :/
< jonasschnelli> I guess you get an expection
< 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
< rhavar> Anyone familiar enough with constraint solving to help me out with this model? https://gist.github.com/RHavar/0710144c713033d42f8f443a99fefbb7
< 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?
< jnewbery> no problem. Topic suggestion: #10650
< gribble> https://github.com/bitcoin/bitcoin/issues/10650 | Multiwallet: add RPC endpoint support by jonasschnelli · Pull Request #10650 · bitcoin/bitcoin · GitHub
< 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 ?
< jonasschnelli> Yes. Agree
< gribble> https://github.com/bitcoin/bitcoin/issues/9728 | Can create Watch Only HD wallet with -hdwatchonly by NicolasDorier · Pull Request #9728 · bitcoin/bitcoin · GitHub
< instagibbs> jtimon, it doesn't do anything special for now
< instagibbs> same as hardened
< wumpus> other topics?
< morcos> jnewbery suggested 10650
< jnewbery> I think we're almost there with 10650. Only major sticking point is not having a default wallet when there are multiple wallets loaded
< jtimon> instagibbs: I see, so it kind of depends on #10240 ?
< gribble> https://github.com/bitcoin/bitcoin/issues/10240 | Add HD wallet auto-restore functionality by jonasschnelli · Pull Request #10240 · bitcoin/bitcoin · GitHub
< instagibbs> jtimon, we can chat offline about that after meeting
< wumpus> #topic Multiwallet: add RPC endpoint support
< jtimon> sure
< jonasschnelli> I just pushed the overhaule of 10650 that fixes the points reported by ryanofsky jnewbery and morcos
< jonasschnelli> *overhaul
< sipa> jonasschnelli: cool
< luke-jr> #10650
< gribble> https://github.com/bitcoin/bitcoin/issues/10650 | Multiwallet: add RPC endpoint support by jonasschnelli · Pull Request #10650 · bitcoin/bitcoin · GitHub
< jnewbery> great! Will review
< jnewbery> thanks jonas
< wumpus> nice
< morcos> yes, excellent. woo!
< 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...
< instagibbs> (total size has been shrinking for a month now)
< petertodd> could easily be someone ran out of money for an attack, and is spending the coins again
< wumpus> any other topics?
< jtimon> many more possible explanations...
< luke-jr> maybe a reminder that Tokyo Core is in a few weeks
< instagibbs> 2 weeks specifically
< luke-jr> https://coredev.tech/tokyo.html lots of unconfirmed invites still
< instagibbs> kind of a hike for those not already in area for other reasons
< jtimon> oh, missed that one, I guess I can confirm now I'm not attending
< wumpus> I'm not coming to tokyo core, but will be in the SF one in september
< sipa> wumpus: great!
< jonasschnelli> SF is pretty full booked..
< jonasschnelli> Not saying we running out of space, but >20 confirmed
< wumpus> sipa: you're coming too? great
< sipa> wumpus: yes, of course
< sipa> (i live nearby)
< achow101> do we have a location for the sf meetup?
< wumpus> yes, but it's still possible it will change
< wumpus> depending on just how large the room needs to be I guess :-)
< instagibbs> will we finally activate segwit there? *ducks*
< rhavar> petertodd: the day it stopped growing is the day alphabay shut down
< luke-jr> instagibbs: it'll already be active by then
< achow101> luke-jr: hopefully
< petertodd> rhavar: interesting!
< instagibbs> rhavar, ? what date did it go down
< rhavar> the 3rd i think
< instagibbs> been holding/dropping since 3rd of June though
< instagibbs> might be contributing factor still..
< instagibbs> sorry, is meeting done
< rhavar> oh, sorry -- I'm a month off
< instagibbs> 3 minutes
< rhavar> alphabay shut down the 3rd of july
< rhavar> and that question was 3rd of june
< instagibbs> my SWAG: I think internalizing customer transaction costs did the equiv of fees for exchange trading in China
< wumpus> seems time to end the meeting, speculation about the reason for the stop of utxo growth is interesting but not a bitcoin core meeting topic
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu Jul 13 19:58:08 2017 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< rhavar> So anyway, anyone familiar with constraint solving (and minizinc in particular)? https://gist.github.com/RHavar/0710144c713033d42f8f443a99fefbb7
< 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> can someone close https://github.com/bitcoin/bitcoin/pull/10768 ? the author said it but for some reason didn't closed it himself
< 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 ?
< bitcoin-git> [bitcoin] promag opened pull request #10819: Fix uninitialized atomic variables (master...2017-07-fix-unitialized-atomic) https://github.com/bitcoin/bitcoin/pull/10819
< promag> I got this output: Fee Calculation: Fee:4520 Bytes:226 Tgt:6 (requested 6) Reason:"Fallback fee" Decay 0.00000: Estimation: (-1 - -1) -nan% 0.0/(0.0 0 mem 0.0 out) Fail: (-1 - -1) -nan% 0.0/(0.0 0 mem 0.0 out)
< promag> buggy output right?
< bitcoin-git> [bitcoin] sipa opened pull request #10820: Use cpuid intrinsics instead of asm code (master...20170717_cpuid) https://github.com/bitcoin/bitcoin/pull/10820