19:01:27 #startmeeting 19:01:27 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 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:01:37 hi 19:01:45 hi 19:01:47 #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 hi 19:01:54 hi 19:02:01 buenos dias 19:02:04 wat 19:02:27 hi 19:02:49 PSA feature freeze for 0.18 is next week #14438 19:02:50 https://github.com/bitcoin/bitcoin/issues/14438 | Release schedule for 0.18.0 · Issue #14438 · bitcoin/bitcoin · GitHub 19:03:33 probably makes sense to discuss what will be ready for merge this week, as that should be prioritized for review 19:03:37 hi 19:03:51 hi - no proposedtopics from the week 19:03:59 moneyball: thanks 19:04:19 hi 19:04:22 #14856 has had lots of review 19:04:23 #topic 0.18 last-minute features 19:04:24 https://github.com/bitcoin/bitcoin/issues/14856 | net: remove more CConnman globals (theuni) by dongcarl · Pull Request #14856 · bitcoin/bitcoin · GitHub 19:04:25 hi 19:04:34 dongcarl: that's not a feature tho 19:04:41 * dongcarl ducks 19:04:43 true 19:05:35 so there's some feature PRs both in the 0.18.0 tag and high priority for review 19:05:38 https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+project%3Abitcoin%2Fbitcoin%2F8 19:05:48 https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.18.0 19:06:06 I guess most are related to the wallet 19:06:20 and the importmulti descriptor imports 19:06:30 oh ofc that's wallet too :< 19:06:43 XD 19:06:58 it would be nice for that to be merged. and blank wallets 19:07:26 so yes, wallet, what will be ready for merge this week? maybe you can weigh in meshcollider? 19:08:05 i see #15153 has a tested ACK 19:08:07 https://github.com/bitcoin/bitcoin/issues/15153 | gui: Add Open Wallet menu by promag · Pull Request #15153 · bitcoin/bitcoin · GitHub 19:09:10 #15226 too 19:09:13 https://github.com/bitcoin/bitcoin/issues/15226 | Allow creating blank (empty) wallets (alternative) by achow101 · Pull Request #15226 · bitcoin/bitcoin · GitHub 19:09:24 so I suppose they can be merged soon 19:10:21 #14491 even seems ready for merge 19:10:23 hi 19:10:25 https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub 19:11:13 #action review last-minute wallet feature PRs for 0.18 19:12:57 if 15153 goes then #15195 and #15204 should probably go too - but these are simpler 19:12:59 https://github.com/bitcoin/bitcoin/issues/15195 | gui: Add Close Wallet action by promag · Pull Request #15195 · bitcoin/bitcoin · GitHub 19:13:01 https://github.com/bitcoin/bitcoin/issues/15204 | gui: Add Open External Wallet action by promag · Pull Request #15204 · bitcoin/bitcoin · GitHub 19:13:41 that's a lot for one week 19:14:07 but we'll see I guess... 19:14:10 any other topics? 19:14:51 (Hi) 19:15:14 (hi!) 19:15:18 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 #topic Advisory and full disclosure for CVE-2018-20587 on bitcoincore.org 19:16:12 hi 19:16:27 looks like the PR is somewhat controversial 19:16:54 that seems weird to me. 19:17:11 it's an absurdly narrow corner case, not some major concern. it should get release notes. 19:17:27 well, there's no fix in Core, so release notes don't really make sense 19:17:31 some docs were updated tho 19:17:40 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 hi! 19:17:56 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 gmaxwell: the attacker could be another unprivileged user 19:18:30 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 gmaxwell: especially for the kind of environments that users that don't know this run 19:18:40 (FWIW, #15223 is the doc/ updates) 19:18:41 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 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 luke-jr: yes but almost all the time multiuser OSes are vulnerable to one priv esc vuln or another. 19:19:05 gmaxwell: not really, many things fail if they can't bind 19:19:51 (I haven't had any negative reports from the fix in Knots, maybe I should PR that) 19:19:51 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 (and 'more' but not by much) 19:20:46 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 using a unix socket would eliminate the vulnerability but I think we've been through this 19:20:56 A real fix would be mutual auth and or using a unix domain socket. 19:21:25 (with a bind=>failure fix) 19:21:52 gmaxwell: I'm not sure most JSON-RPC libraries can support those 19:22:00 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 +http 19:22:22 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 [13bitcoin] 15MeshCollider pushed 2 commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/d83d6079432c...1933e38c1a08 19:22:29 13bitcoin/06master 145952838 15Sjors Provoost: [rpc] util: add deriveaddresses method 19:22:29 13bitcoin/06master 141933e38 15MeshCollider: Merge #14667: Add deriveaddresses RPC util method 19:22:40 luke-jr: indeed, which is another point that essentially everything off a privleged port is similarly 'vulnerable'. 19:23:01 [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 annoyingly, most OSs doesn't make it practical to reserve ports [privileged or otherwise] for specific users :/ 19:23:53 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 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 the blog post also won't be noticed, plus I'm concerned that its crying wolf -- increasing the profile of minutia. 19:24:33 I'm also worried of that. 19:24:45 most people will be able to do exactly nothing with this advisory 19:24:48 wumpus: it is? 19:24:48 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 luke-jr: yes, there's a uid filter 19:25:15 okay, sounds like I should just close the PR then and leave it at that 19:25:39 at least there was back in the day I was still interested in using user ids for separation instead of VMs 19:25:41 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 wumpus: but that can't block binding afaik? I guess if nobody can connect it's effectively the same.. 19:27:14 ok, any other topics? 19:27:32 short topic: descriptor checksums 19:27:43 #topic Descriptor checksums (sipa) 19:27:55 so, this was discussed in the wallet meeting 1 or 2 weeks ago 19:28:25 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 and things like keypaths and public keys are quite vulnerable to that 19:28:55 makes sense 19:29:00 I think it might be annoying to have to calculate checksums if you enter a keypath by hand..? 19:29:28 it may be optional 19:29:31 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 oh, there's a wallet meeting? 19:29:48 jtimon: same time Friday, every other week 19:29:56 thanks 19:30:25 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 sipa: will they be optional? 19:30:53 luke-jr: for RPCs where mistakes aren't critical, yes 19:30:55 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 and can you (eg) have checksums on the keys, but not the paths? 19:31:14 no, you'd need a tool or RPC to recompute the checksum 19:31:18 :/ 19:31:34 my thinking is that for most things where you're just "playing" with them, they're optional 19:31:37 luke-jr: keys would still be checksummed becaused of base58 19:32:00 sipa: how large of a PR is it? 19:32:12 meshcollider: not big, very localized 19:32:22 thats good, I think we could get it in then 19:32:37 good, at least it won't interfere with the others then 19:32:48 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 is it based on #14491 then? 19:33:16 I think there should be a flag to make them optional 19:33:17 https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub 19:33:18 skipping the checksum on RPC level is probably acceptable 19:33:32 meshcollider: no, but it'd be a one-line change to integrate 19:33:45 awesome 19:33:47 almost certainly the response to mandatory checksum will probably be to just calculate it :x 19:33:59 probably enough to just make sure if it is present and doesn't match, it fails 19:34:10 luke-jr: that's perhaps true... but it's the best we can do 19:36:01 end topic 19:36:03 we should avoid ever creating / displaying one without a checksum ourselves 19:36:35 gwillen: yes, my PR adds the checksum in the text serialization code; it's optional just in the parser 19:36:43 :+1: 19:37:03 (no point in discussing this here; will open the PR as soon as a few tests are fixed) 19:37:35 more topics? 19:37:50 hi 19:38:02 I have just a little reminder... 19:38:09 #13360 19:38:12 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 we should finally add older wallet tests 19:38:38 with growing states (blank wallets, disable private key, hd non hd) 19:38:59 I mean function tests that test stuff with wallets from older bitcoin core versions 19:39:11 #topic old wallet tests 19:39:14 Or we will certainly break stuff in the near future 19:39:45 [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 I think MarcoFalke once made an issue: #14536 19:39:49 https://github.com/bitcoin/bitcoin/issues/14536 | functional test with ancient wallet.dat (upgrade test) · Issue #14536 · bitcoin/bitcoin · GitHub 19:41:04 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 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 sorry,... I meant chainstate database upgrade (not levelDB upgrades) 19:42:21 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 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 Sjors has a PR for using older versions in testing: #12134 19:42:49 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 I wouldn't think a simple old wallet would be that large 19:43:02 Probably,... but a couple of older wallet.dats in as test statics would not be completely wrong IMO 19:43:27 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 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 but that's getting complex 19:43:38 I don't care where they are stored but we should finally add tests 19:44:16 I think the idea of having old wallet files in the tests is more maintainable 19:44:17 luke-jr: thats another tests that would certenly required to compile older version of Core 19:45:09 I'm not too happy with the idea of adding wallet.dat's to the repository 19:45:30 wumpus: seen my reply above? 19:45:47 MarcoFalke: no I missed that, :+1: to that 19:46:00 wumpus: we could rename them to static.dump *duck* 19:46:06 one could manually construct wallet.dats without keypool for testing, yeah? So they don't need to be very large? 19:46:21 unless that itself would break or fail to test important stuff. 19:46:22 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 jup. agree 19:46:37 since this is for testing, can't we just download older versions from https://github.com/bitcoin/bitcoin/releases ? 19:46:38 yes- 19:46:46 that's why I labeled it as "reminder". 19:46:51 heh 19:46:53 Create blank wallets will be merged soon 19:47:03 jonasschnelli: or have it available anyway 19:47:27 wumpus: what about Python maps of key/values that get shoved in a BDB db manually? :P 19:47:44 luke-jr: thats not authentic enough IMO 19:48:24 Jup, we really need a wallet.dat before every feature-bump in Bitcoin Core 19:48:36 well, that would still be better than nothing, I guess 19:49:45 any more topics? 19:50:00 I'd like to have some concept ACK and review for #13360, which makes out-of-bound SIGHASH_SINGLE non-standard. 19:50:02 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 this could become a softfork someday 19:50:42 #topic out-of-bound SIGHASH_SINGLE (jl2012) 19:50:46 jl2012: ltns. What's the benefit though? 19:51:27 luke-jr: to avoid losing money by accident 19:51:55 ah, so it's more for local sendrawtx than for relaying 19:52:13 in legacy script, signing with out-of-bound SINGLE is almost like revealing your private key 19:52:28 (I'm not sure it makes sense to do a softfork for that reason though) 19:52:34 it's like absurd fee 19:52:54 fee is a feature, but this thing is clearly a bug 19:53:26 jl2012: IIRC, it has been proposed to use it, in the past 19:53:37 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 I believe the feature in that case, was that the signature was smaller than a normal one 19:54:14 (the goal was to clean dust UTXOs) 19:54:36 luke-jr, only example I knpw of :P https://underhandedcrypto.com/2016/08/17/the-2016-backdoored-cryptocurrency-contest-winner/ 19:54:39 how smaller? It's still 72 bytes or so 19:54:55 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 or maybe it was compressability? I'm not sure 19:54:59 you can use SIGHASH_NONE for donating dust 19:55:27 wumpus: +1 19:55:32 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 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 gmaxwell: I think yuo had suggested some "privkey gets compromised" dust cleaning thing - was that this? 19:56:50 luke-jr: sorry, missing the context 19:57:02 luke-jr: I guess you mean that's cheaper to validate, as you don't need to hash it 19:57:21 gmaxwell: [19:50:00] I'd like to have some concept ACK and review for #13360, which makes out-of-bound SIGHASH_SINGLE non-standard. 19:57:23 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 but using ANYONECANPAY|NONE would not be too bad 19:57:33 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 get rid of SIGHASH_SINGLE completely? 19:58:10 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 no, the bug. 19:58:33 Single itself is fine, sorry-- but the single bug is a hazard. 19:58:43 it's also not actually useful. 19:58:45 Nor used. 19:58:54 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 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 k 19:59:51 (though may have made them somewhat more gzippable, but not more compressable with a format aware compressor) 20:00:40 #endmeeting