19:01:27 <wumpus> #startmeeting
19:01:27 <lightningbot> Meeting started Thu Feb  7 19:01:27 2019 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:01:27 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:01:37 <jnewbery> hi
19:01:45 <jonasschnelli> hi
19:01:47 <wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb
19:01:53 <achow101> hi
19:01:54 <gkrizek> hi
19:02:01 <gwillen> buenos dias
19:02:04 <luke-jr> wat
19:02:27 <jamesob> hi
19:02:49 <wumpus> PSA feature freeze for 0.18 is next week #14438
19:02:50 <gribble> https://github.com/bitcoin/bitcoin/issues/14438 | Release schedule for 0.18.0 · Issue #14438 · bitcoin/bitcoin · GitHub
19:03:33 <wumpus> probably makes sense to discuss what will be ready for merge this week, as that should be prioritized for review
19:03:37 <promag> hi
19:03:51 <moneyball> hi - no proposedtopics from the week
19:03:59 <wumpus> moneyball: thanks
19:04:19 <instagibbs> hi
19:04:22 <dongcarl> #14856 has had lots of review
19:04:23 <wumpus> #topic 0.18 last-minute features
19:04:24 <gribble> https://github.com/bitcoin/bitcoin/issues/14856 | net: remove more CConnman globals (theuni) by dongcarl · Pull Request #14856 · bitcoin/bitcoin · GitHub
19:04:25 <jtimon> hi
19:04:34 <wumpus> dongcarl: that's not a feature tho
19:04:41 * dongcarl ducks
19:04:43 <dongcarl> true
19:05:35 <wumpus> so there's some feature PRs both in the 0.18.0 tag and high priority for review
19:05:38 <wumpus> https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+project%3Abitcoin%2Fbitcoin%2F8
19:05:48 <wumpus> https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.18.0
19:06:06 <wumpus> I guess most are related to the wallet
19:06:20 <wumpus> and the importmulti descriptor imports
19:06:30 <wumpus> oh ofc that's wallet too :<
19:06:43 <luke-jr> XD
19:06:58 <achow101> it would be nice for that to be merged. and blank wallets
19:07:26 <wumpus> so yes, wallet, what will be ready for merge this week? maybe you can weigh in meshcollider?
19:08:05 <wumpus> i see #15153 has a tested ACK
19:08:07 <gribble> https://github.com/bitcoin/bitcoin/issues/15153 | gui: Add Open Wallet menu by promag · Pull Request #15153 · bitcoin/bitcoin · GitHub
19:09:10 <wumpus> #15226 too
19:09:13 <gribble> https://github.com/bitcoin/bitcoin/issues/15226 | Allow creating blank (empty) wallets (alternative) by achow101 · Pull Request #15226 · bitcoin/bitcoin · GitHub
19:09:24 <wumpus> so I suppose they can be merged soon
19:10:21 <wumpus> #14491 even seems ready for merge
19:10:23 <phantomcircuit> hi
19:10:25 <gribble> https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub
19:11:13 <wumpus> #action review last-minute wallet feature PRs for 0.18
19:12:57 <promag> if 15153 goes then #15195 and #15204 should probably go too - but these are simpler
19:12:59 <gribble> https://github.com/bitcoin/bitcoin/issues/15195 | gui: Add Close Wallet action by promag · Pull Request #15195 · bitcoin/bitcoin · GitHub
19:13:01 <gribble> https://github.com/bitcoin/bitcoin/issues/15204 | gui: Add Open External Wallet action by promag · Pull Request #15204 · bitcoin/bitcoin · GitHub
19:13:41 <wumpus> that's a lot for one week
19:14:07 <wumpus> but we'll see I guess...
19:14:10 <wumpus> any other topics?
19:14:51 <gmaxwell> (Hi)
19:15:14 <wumpus> (hi!)
19:15:18 <luke-jr> should I close https://github.com/bitcoin-core/bitcoincore.org/pull/637, or do we still want to do something for that?
19:15:46 <wumpus> #topic Advisory and full disclosure for CVE-2018-20587 on bitcoincore.org
19:16:12 <meshcollider> hi
19:16:27 <wumpus> looks like the PR is somewhat controversial
19:16:54 <gmaxwell> that seems weird to me.
19:17:11 <gmaxwell> it's an absurdly narrow corner case, not some major concern. it should get release notes.
19:17:27 <luke-jr> well, there's no fix in Core, so release notes don't really make sense
19:17:31 <luke-jr> some docs were updated tho
19:17:40 <wumpus> I tend to agree with harding though "I'm a fan of updating the documentation in the Bitcoin Core docs/ directory and putting something into the release notes instead of publishing a long blog post that basically says that using a computer not under your exclusive control is unsafe"
19:17:55 <sipa> hi!
19:17:56 <gmaxwell> requires an attacker on your local host, which 99.99999% of the time means you are boned anyways.
19:18:10 * jonasschnelli does also agree with harding
19:18:13 <luke-jr> gmaxwell: the attacker could be another unprivileged user
19:18:30 <gmaxwell> and it's also a 'vulnerablity' that is shared by virtually every other piece of software with an rpc on a non-privledged port.
19:18:30 <wumpus> gmaxwell: especially for the kind of environments that users that don't know this run
19:18:40 <luke-jr> (FWIW, #15223 is the doc/ updates)
19:18:41 <gribble> https://github.com/bitcoin/bitcoin/issues/15223 | Doc: add information about security to the JSON-RPC doc by harding · Pull Request #15223 · bitcoin/bitcoin · GitHub
19:18:55 <wumpus> e.g. if you're on windows and run everything as the same user anyhow then someone can just as well install a keylogger
19:18:59 <gmaxwell> luke-jr: yes but almost all the time multiuser OSes are vulnerable to one priv esc vuln or another.
19:19:05 <luke-jr> gmaxwell: not really, many things fail if they can't bind
19:19:51 <luke-jr> (I haven't had any negative reports from the fix in Knots, maybe I should PR that)
19:19:51 <gmaxwell> luke-jr: failing if you don't bind doesn't actually eliminate the vulnerablity here, just makes you more likely to notice (esp after you're screwed).
19:20:24 <gmaxwell> (and 'more' but not by much)
19:20:46 <luke-jr> gmaxwell: if you don't notice prior to the attack, at most they can get the walletpassphrase, which isn't a big problem if the wallet file is inaccessible
19:20:50 <wumpus> using a unix socket would eliminate the vulnerability but I think we've been through this
19:20:56 <gmaxwell> A real fix would be mutual auth and or using a unix domain socket.
19:21:25 <luke-jr> (with a bind=>failure fix)
19:21:52 <luke-jr> gmaxwell: I'm not sure most JSON-RPC libraries can support those
19:22:00 <wumpus> which would be nice, but I don't think there's anything new to discuss in that regard, there's still the libevent server crap
19:22:09 <wumpus> +http
19:22:22 <gmaxwell> luke-jr: the way you'd do the attack is start a process that binds in a tight loop until it gets it. then when it gets it it listens for a walletpassphrase then shuts off.  What the user will is is that their daemon crashed, and they'll restart it before digging through the logs with almost certanty.
19:22:28 <bitcoin-git> [13bitcoin] 15MeshCollider pushed 2 commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/d83d6079432c...1933e38c1a08
19:22:29 <bitcoin-git> 13bitcoin/06master 145952838 15Sjors Provoost: [rpc] util: add deriveaddresses method
19:22:29 <bitcoin-git> 13bitcoin/06master 141933e38 15MeshCollider: Merge #14667: Add deriveaddresses RPC util method
19:22:40 <gmaxwell> luke-jr: indeed, which is another point that essentially everything off a privleged port is similarly 'vulnerable'.
19:23:01 <bitcoin-git> [13bitcoin] 15MeshCollider merged pull request #14667: Add deriveaddresses RPC util method (06master...062018/11/deriveaddress) 02https://github.com/bitcoin/bitcoin/pull/14667
19:23:25 <luke-jr> annoyingly, most OSs doesn't make it practical to reserve ports [privileged or otherwise] for specific users :/
19:23:53 <luke-jr> anyway, main question is do we want to do some blog post alerting people to this? or just leave it at a doc/ probably nobody will notice?
19:24:19 <wumpus> it's possible on linux with iptables IIRC buut I'm not sure we really want to go into that rabbit hole for documentation
19:24:25 <gmaxwell> the blog post also won't be noticed, plus I'm concerned that its crying wolf -- increasing the profile of minutia.
19:24:33 <wumpus> I'm also worried of that.
19:24:45 <wumpus> most people will be able to do exactly nothing with this advisory
19:24:48 <luke-jr> wumpus: it is?
19:24:48 <gmaxwell> Like we don't have a blog post warning you about following links or whatever, which is a much greater risk to users than this.
19:25:12 <wumpus> luke-jr: yes, there's a uid filter
19:25:15 <luke-jr> okay, sounds like I should just close the PR then and leave it at that
19:25:39 <wumpus> at least there was back in the day I was still interested in using user ids for separation instead of VMs
19:25:41 <gmaxwell> If we really wanted a blog post ... maybe instead we should have a blog post about bitcoin wallet security best practices, and this port thing could be mentioned somewhere in it. that would make sense.  But alerting on this? I don't think so.
19:25:42 <luke-jr> wumpus: but that can't block binding afaik? I guess if nobody can connect it's effectively the same..
19:27:14 <wumpus> ok, any other topics?
19:27:32 <sipa> short topic: descriptor checksums
19:27:43 <wumpus> #topic Descriptor checksums (sipa)
19:27:55 <sipa> so, this was discussed in the wallet meeting 1 or 2 weeks ago
19:28:25 <sipa> the idea is that if descriptors are going to be used as import, and to generate addresses, we probably want to protect against typos (or due to length more commonly, copy-paste errors)
19:28:37 <sipa> and things like keypaths and public keys are quite vulnerable to that
19:28:55 <wumpus> makes sense
19:29:00 <luke-jr> I think it might be annoying to have to calculate checksums if you enter a keypath by hand..?
19:29:28 <jonasschnelli> it may be optional
19:29:31 <sipa> i have a PR almost ready to add these (will submit today), but since deriveaddress was just merged, i think it would make sense to have these in 0.18 (as it'd otherwise an incompatible change later; scantxoutset doesn't really require checksums)
19:29:33 <jtimon> oh, there's a wallet meeting?
19:29:48 <luke-jr> jtimon: same time Friday, every other week
19:29:56 <jtimon> thanks
19:30:25 <sipa> i'm just bringing it up as a heads up here; obviously if review doesn't let us get it in 0.18, so be it
19:30:41 <luke-jr> sipa: will they be optional?
19:30:53 <sipa> luke-jr: for RPCs where mistakes aren't critical, yes
19:30:55 <wumpus> it is very last minute considering there's many other wallet PRs that are tagged 0.18, but sure it'd be nice
19:30:58 <luke-jr> and can you (eg) have checksums on the keys, but not the paths?
19:31:14 <sipa> no, you'd need a tool or RPC to recompute the checksum
19:31:18 <luke-jr> :/
19:31:34 <sipa> my thinking is that for most things where you're just "playing" with them, they're optional
19:31:37 <achow101> luke-jr: keys would still be checksummed becaused of base58
19:32:00 <meshcollider> sipa: how large of a PR is it?
19:32:12 <sipa> meshcollider: not big, very localized
19:32:22 <meshcollider> thats good, I think we could get it in then
19:32:37 <wumpus> good, at least it won't interfere with the others then
19:32:48 <sipa> but when importing (which isn't merged yet), the checksum would be mandatory (or there could be a flag to disable that, if people like that...)
19:33:14 <meshcollider> is it based on #14491 then?
19:33:16 <achow101> I think there should be a flag to make them optional
19:33:17 <gribble> https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub
19:33:18 <jonasschnelli> skipping the checksum on RPC level is probably acceptable
19:33:32 <sipa> meshcollider: no, but it'd be a one-line change to integrate
19:33:45 <meshcollider> awesome
19:33:47 <luke-jr> almost certainly the response to mandatory checksum will probably be to just calculate it :x
19:33:59 <luke-jr> probably enough to just make sure if it is present and doesn't match, it fails
19:34:10 <sipa> luke-jr: that's perhaps true... but it's the best we can do
19:36:01 <sipa> end topic
19:36:03 <gwillen> we should avoid ever creating / displaying one without a checksum ourselves
19:36:35 <sipa> gwillen: yes, my PR adds the checksum in the text serialization code; it's optional just in the parser
19:36:43 <gwillen> :+1:
19:37:03 <sipa> (no point in discussing this here; will open the PR as soon as a few tests are fixed)
19:37:35 <wumpus> more topics?
19:37:50 <jl2012> hi
19:38:02 <jonasschnelli> I have just a little reminder...
19:38:09 <jl2012> #13360
19:38:12 <gribble> https://github.com/bitcoin/bitcoin/issues/13360 | [Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012 · Pull Request #13360 · bitcoin/bitcoin · GitHub
19:38:20 <jonasschnelli> we should finally add older wallet tests
19:38:38 <jonasschnelli> with growing states (blank wallets, disable private key, hd non hd)
19:38:59 <jonasschnelli> I mean function tests that test stuff with wallets from older bitcoin core versions
19:39:11 <wumpus> #topic old wallet tests
19:39:14 <jonasschnelli> Or we will certainly break stuff in the near future
19:39:45 <bitcoin-git> [13bitcoin] 15MarcoFalke opened pull request #15365: wallet: Add lock annotation for mapAddressBook (06master...06Mf1902-LockAnnotmapAddressBook) 02https://github.com/bitcoin/bitcoin/pull/15365
19:39:48 <jonasschnelli> I think MarcoFalke once made an issue: #14536
19:39:49 <gribble> https://github.com/bitcoin/bitcoin/issues/14536 | functional test with ancient wallet.dat (upgrade test) · Issue #14536 · bitcoin/bitcoin · GitHub
19:41:04 <jonasschnelli> So if someone is interested to write some tests against older wallets and eventually just use static older wallet files,.... your welcome
19:41:38 <jonasschnelli> The other route would be compiling older Bitcoin Core versions and use that as base for file interoperability tests (including levelDb upgrades, etc.)
19:42:16 <jonasschnelli> sorry,... I meant chainstate database upgrade (not levelDB upgrades)
19:42:21 <luke-jr> jonasschnelli: that complicates testing, which might be fine for extended tests, but I think wallet tests would be better as an "always run"
19:42:30 <MarcoFalke> The wallet file could be added to https://github.com/bitcoin-core/qa-assets in case core-repo-bloat is a concern
19:42:46 <jnewbery> Sjors has a PR for using older versions in testing: #12134
19:42:49 <gribble> https://github.com/bitcoin/bitcoin/issues/12134 | Build previous releases and run functional tests by Sjors · Pull Request #12134 · bitcoin/bitcoin · GitHub
19:42:50 <luke-jr> I wouldn't think a simple old wallet would be that large
19:43:02 <jonasschnelli> Probably,... but a couple of older wallet.dats in as test statics would not be completely wrong IMO
19:43:27 <jnewbery> I think it's a nice idea, but the instability of the interfaces and testing framework means it'll be quite an effort to maintain going forward
19:43:28 <luke-jr> I guess an ideal test would make an old wallet, load it in a new version, do stuff, then make sure the old version still works
19:43:33 <luke-jr> but that's getting complex
19:43:38 <jonasschnelli> I don't care where they are stored but we should finally add tests
19:44:16 <jnewbery> I think the idea of having old wallet files in the tests is more maintainable
19:44:17 <jonasschnelli> luke-jr: thats another tests that would certenly required to compile older version of Core
19:45:09 <wumpus> I'm not too happy with the idea of adding wallet.dat's to the repository
19:45:30 <MarcoFalke> wumpus: seen my reply above?
19:45:47 <wumpus> MarcoFalke: no I missed that, :+1: to that
19:46:00 <jonasschnelli> wumpus: we could rename them to static.dump *duck*
19:46:06 <gwillen> one could manually construct wallet.dats without keypool for testing, yeah? So they don't need to be very large?
19:46:21 <gwillen> unless that itself would break or fail to test important stuff.
19:46:22 <MarcoFalke> I don't think there is anything to discuss here. It just needs someone to create some wallet.dats and write the test
19:46:33 <jonasschnelli> jup. agree
19:46:37 <jtimon> since this is for testing, can't we just download older versions from https://github.com/bitcoin/bitcoin/releases ?
19:46:38 <wumpus> yes-
19:46:46 <jonasschnelli> that's why I labeled it as "reminder".
19:46:51 <MarcoFalke> heh
19:46:53 <jonasschnelli> Create blank wallets will be merged soon
19:47:03 <luke-jr> jonasschnelli: or have it available anyway
19:47:27 <luke-jr> wumpus: what about Python maps of key/values that get shoved in a BDB db manually? :P
19:47:44 <jonasschnelli> luke-jr: thats not authentic enough IMO
19:48:24 <MarcoFalke> Jup, we really need a wallet.dat before every feature-bump in Bitcoin Core
19:48:36 <jtimon> well, that would still be better than nothing, I guess
19:49:45 <wumpus> any more topics?
19:50:00 <jl2012> I'd like to have some concept ACK and review for #13360, which makes out-of-bound SIGHASH_SINGLE non-standard.
19:50:02 <gribble> https://github.com/bitcoin/bitcoin/issues/13360 | [Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012 · Pull Request #13360 · bitcoin/bitcoin · GitHub
19:50:16 <jl2012> this could become a softfork someday
19:50:42 <wumpus> #topic out-of-bound SIGHASH_SINGLE (jl2012)
19:50:46 <luke-jr> jl2012: ltns. What's the benefit though?
19:51:27 <jl2012> luke-jr: to avoid losing money by accident
19:51:55 <luke-jr> ah, so it's more for local sendrawtx than for relaying
19:52:13 <jl2012> in legacy script, signing with out-of-bound SINGLE is almost like revealing your private key
19:52:28 <luke-jr> (I'm not sure it makes sense to do a softfork for that reason though)
19:52:34 <luke-jr> it's like absurd fee
19:52:54 <jl2012> fee is a feature, but this thing is clearly a bug
19:53:26 <luke-jr> jl2012: IIRC, it has been proposed to use it, in the past
19:53:37 <wumpus> I'm also not sure it makes sense to do a softfork for this, if you're afraid of users doing this then it indeed makes sense to add it on submitrawtransaction
19:54:05 <luke-jr> I believe the feature in that case, was that the signature was smaller than a normal one
19:54:14 <luke-jr> (the goal was to clean dust UTXOs)
19:54:36 <instagibbs> luke-jr, only example I knpw of :P https://underhandedcrypto.com/2016/08/17/the-2016-backdoored-cryptocurrency-contest-winner/
19:54:39 <jl2012> how smaller? It's still 72 bytes or so
19:54:55 <wumpus> I mean there's tons of ways to lose money with raw transactions, it is user friendly to try to protect this on RPC, but I'm not sure it merits changing the consensus rules... but that's just me
19:54:56 <luke-jr> or maybe it was compressability? I'm not sure
19:54:59 <jl2012> you can use SIGHASH_NONE for donating dust
19:55:27 <luke-jr> wumpus: +1
19:55:32 <gmaxwell> achow101: I don't think there should be a flag to make them optional, instead just have a command that adds/fixes them.
19:56:03 <gmaxwell> achow101: so then you're not peppering every interface with a flag, and also don't run into people just setting the flag all the time "because thats how you do it"
19:56:07 <luke-jr> gmaxwell: I think yuo had suggested some "privkey gets compromised" dust cleaning thing - was that this?
19:56:50 <gmaxwell> luke-jr: sorry, missing the context
19:57:02 <jl2012> luke-jr: I guess you mean that's cheaper to validate, as you don't need to hash it
19:57:21 <luke-jr> gmaxwell: [19:50:00] <jl2012> I'd like to have some concept ACK and review for #13360, which makes out-of-bound SIGHASH_SINGLE non-standard.
19:57:23 <gribble> https://github.com/bitcoin/bitcoin/issues/13360 | [Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012 · Pull Request #13360 · bitcoin/bitcoin · GitHub
19:57:32 <jl2012> but using ANYONECANPAY|NONE would not be too bad
19:57:33 <gmaxwell> I think we should get rid of sighash single, not to protect users (the UI does that), but because it's a constant hazard in consensus rules.
19:58:10 <wumpus> get rid of SIGHASH_SINGLE completely?
19:58:10 <gmaxwell> Twice now I've gone through some horror of thinking some interaction of consensus rules/ecdsa would make all funds trivially stealable but only to be saved by dumb luck.
19:58:17 <gmaxwell> no, the bug.
19:58:33 <gmaxwell> Single itself is fine, sorry-- but the single bug is a hazard.
19:58:43 <gmaxwell> it's also not actually useful.
19:58:45 <gmaxwell> Nor used.
19:58:54 <luke-jr> gmaxwell: a few years ago, you had a way to reduce block space used in dust cleanup, but in a way that compromised the privkey - was that related to this?
19:59:21 <gmaxwell> luke-jr: It just used a nonce with an usually small nonce. Using the single bug didn't make the txn any smaller.
19:59:27 <luke-jr> k
19:59:51 <gmaxwell> (though may have made them somewhat more gzippable, but not more compressable with a format aware compressor)
20:00:40 <wumpus> #endmeeting