19:00:42 #startmeeting 19:00:42 Meeting started Thu Dec 15 19:00:42 2016 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:42 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:48 here 19:00:52 proposed topics? 19:01:03 #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 19:01:08 hi 19:01:15 + jl2012 19:01:24 I was going to talk about some backlog but almost all of it got merged. 19:01:28 oh right 19:01:31 * gmaxwell looks 19:01:44 sick and useless, but here 19:02:28 cfields: ouch you got it now, too? 19:02:44 BlueMatt: mine was nice enough to wait until the flight home :\ 19:02:47 #9322 seems to need discussion 19:02:49 https://github.com/bitcoin/bitcoin/issues/9322 | An error has occurred and has been logged. Please contact this bot's administrator for more information. 19:02:58 hi 19:03:01 interesting issue 19:03:04 "Don't set unknown rpcserialversion" 19:03:12 #9322 19:03:14 https://github.com/bitcoin/bitcoin/issues/9322 | An error has occurred and has been logged. Please contact this bot's administrator for more information. 19:03:16 #9352 needs to move forward quickly for fibre/some current network issues 19:03:18 https://github.com/bitcoin/bitcoin/issues/9352 | Attempt reconstruction from all compact block announcements by sdaftuar · Pull Request #9352 · bitcoin/bitcoin · GitHub 19:03:34 looks like there is disagreement about whether it should be done at all 19:04:04 wumpus: on which? 19:04:08 the one I just mentioned 19:05:11 I don't see the disagreement on 9332? 19:05:31 luke-jr seemingly does 19:05:34 #9352 is 21 hours old, that could hardly be called backlog 19:05:36 https://github.com/bitcoin/bitcoin/issues/9352 | Attempt reconstruction from all compact block announcements by sdaftuar · Pull Request #9352 · bitcoin/bitcoin · GitHub 19:05:41 The purpose of the change is to return an error if you ask for seralization version 9 on software that supports 0/1. 19:05:52 wumpus: didnt say backlog, said critical to address current ongoing network issues 19:05:54 yes I think that's well understood 19:06:01 we can talk about that next. 19:06:18 would have been useful if luke-jr was here 19:06:32 hi. late. 19:06:37 oh missed his comment. 19:06:41 #9332 19:06:43 https://github.com/bitcoin/bitcoin/issues/9332 | Let wallet importmulti RPC accept labels for standard scriptPubKeys (on top of #9331) by ryanofsky · Pull Request #9332 · bitcoin/bitcoin · GitHub 19:07:10 I read luke's comment as saying he wanted a "max you support" version. 19:07:12 you mean 9322? 19:07:42 and the response was that this was expected to be the default. Or at least thats my understanding. 19:07:46 is luke's comment related to https://github.com/bitcoin/bitcoin/pull/9322/files#r92147938 ? 19:08:19 I agree that being able to ask for a max possible is fine. (though 9999 isn't an especially good number for it. :P) 19:08:26 I think #9262 is ready, but some disagreement over default value? 19:08:28 https://github.com/bitcoin/bitcoin/issues/9262 | Prefer coins that have fewer ancestors, sanity check txn before ATMP by instagibbs · Pull Request #9262 · bitcoin/bitcoin · GitHub 19:08:39 do we have a topic? 19:08:46 jtimon: pr backlog 19:09:03 gmaxwell: in any case that doesn't have to be done in that pull, so we can just go ahead and merge it 19:09:16 ACK 19:10:01 in 9262 I don't believe this should default to on, for the same reason that spending unconfirmed coins is enabled by default. 19:10:27 The transactions will be queued in the wallet and periodically rebroadcast (due to other fixes) and go out once they're no longer overlimit. 19:10:43 the meat of the change was avoiding those cases (sometimes) when it could. 19:10:54 #9289 is holding up the next round of changes, and I believe the linked issue is unrelated 19:10:56 https://github.com/bitcoin/bitcoin/issues/9289 | net: drop boost::thread_group by theuni · Pull Request #9289 · bitcoin/bitcoin · GitHub 19:10:58 with other fixes I'm completely comfortable with it off by default. 19:10:59 [13bitcoin] 15laanwj pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/756374c522c4...c9e00591cd3f 19:11:00 13bitcoin/06master 1480d073c 15Pieter Wuille: Complain when unknown rpcserialversion is specified 19:11:00 13bitcoin/06master 14fa615d3 15MarcoFalke: [qa] Don't set unknown rpcserialversion 19:11:01 13bitcoin/06master 14c9e0059 15Wladimir J. van der Laan: Merge #9322: [qa] Don't set unknown rpcserialversion... 19:11:06 (though maybe #9212 deserves a topic of its own) 19:11:07 https://github.com/bitcoin/bitcoin/issues/9212 | Assertion failed: (nSendVersion != 0), function GetSendVersion, file ./net.h, line 775. · Issue #9212 · bitcoin/bitcoin · GitHub 19:11:12 [13bitcoin] 15laanwj closed pull request #9292: Complain when unknown rpcserialversion is specified (06master...06nofutureserial) 02https://github.com/bitcoin/bitcoin/pull/9292 19:11:53 cfields: agreed 19:12:58 ok, so #9262 off by default? should it still be backported then? 19:13:01 https://github.com/bitcoin/bitcoin/issues/9262 | Prefer coins that have fewer ancestors, sanity check txn before ATMP by instagibbs · Pull Request #9262 · bitcoin/bitcoin · GitHub 19:13:14 cfields/wumpus: I think there is a fix commit for 9212 on the issue page at the bottom (I havent pr'ed yet because testing, but I think it'd work) 19:13:28 wumpus: yes, it should, the main thing in the change is that it avoids creating those poorly propagating transactions when it's possible. 19:13:42 (My opinion) 19:13:44 wumpus: 9262 does 2 things 1) avoid long chains 2) pre-reject created wallet transactions that would exceed limits 19:13:51 gmaxwell: so it still does something even if it's disabled? okay 19:13:52 wumpus: only 2 is optional 19:13:59 okay, right, that wasn't clear to me 19:14:19 BlueMatt: ok, will test that too 19:14:38 yes so with default off it will simply try harder to pick coins that have shorter chain length 19:14:44 rather than blindly 19:15:06 which won't have an effect if you're always sending your full change 19:15:10 but better is better 19:15:32 BlueMatt: the reason I didn't do that is that it hides the previous behavior. The current asserts point out issues that need to be backported to 0.13 19:15:43 (which admittedly should've been loud errors rather than asserts) 19:15:46 The original suggestion to create that change was (1) based on me actually encountering users that could have avoided the long chains. 19:16:11 here 19:16:39 cfields: critical issues? or nothing that needs to hold up 0.13.2? 19:16:51 cfields: I had a node go down with-- we think-- that assert.. but can't tell where it was triggered from. 19:16:59 cfields: do they really need backporting? 19:17:03 wumpus: likely nothing critical, just possible data leaks 19:17:20 so if I get this right there are now two remaining unmerged pulls that need backport to 0.13.2 https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.13.2 19:17:26 cfields: why are those data leaks? anyway, I think we previously discussed not using nVersion != 0 for this check 19:17:38 just one I mean, the other *is* a a backport 19:17:41 cfields: i'd say that such issues are things where we're certainly violating some of our own assumptions about how the p2p implementation works, but unlikely things that cause issues in interaction with other nodes 19:18:14 any assert represents some case where we should be disconnected, but instead are still sending/responding. 19:18:25 #8855 could need rebase if there's new uses of Params(std::string), but if there are, they won't necessarily cause git conflicts 19:18:35 cfields: no, in this case it means we are sending, but havent yet sent version message 19:19:00 wumpus: I believe #9352 will be tagged for backport-- but it's too green to comment on it for the moment. 19:19:25 gmaxwell: that's too bad, I hoped we could finally get this over with this week 19:19:41 BlueMatt: right, which specifically here means that we've refused the connection due to missing connection flags, but we're still sending/responding 19:19:47 gmaxwell: can't it wait for 0.13.3? 19:19:48 gmaxwell: should i go ahead and open the backport version of #9352? 19:19:58 wumpus: its a relatively simple patch, I'm hopeful we still can :) 19:20:09 I will review asap 19:20:58 BlueMatt: let's take it up after the meeting 19:21:03 cfields: sure 19:23:22 okay, any other topics to discuss? 19:23:32 sdaftuar: I think that would be useful. 19:23:49 wumpus: I really want 0.13.2 in RC ASAP. just have some specific conerns about needing that. We'll work through it. 19:24:12 Could 9262 delay the rc? 19:24:20 Is it well tested? 19:24:20 #8498 has been in the backlog for a while too (before that, #6445 was waiting for #6526/#6625/#6557 and friends, which were merged or closed long ago) 19:24:35 MarcoFalke: I've tested the heck out of it. dunno about others. 19:24:35 (Note that it was not yet merged into master) 19:24:49 (I haven't really looked at it) 19:24:56 wumpus: regarding the assertion backports, nothing would be a regression from 0.13, so no need to delay, only a bonus if we get fixes in. 19:25:08 sdaftuar: ack on backport #9352 19:25:28 MarcoFalke: it's the oldest of thes long-chain wallet fixes, just last to merge. as it had lots of oppturnities for shed painting and resulted in deciding to fix the other issues. :) 19:25:39 MarcoFalke: there was at least the discussion to disable the setting by default, but after that change I don't know why it should hold up anything 19:25:56 MarcoFalke: I don't think there's any critical concerns with it left 19:26:05 with the default off it only changes 'non-determinstic' behavior. 19:26:19 (selectcoins) 19:26:27 the patch always had the setting off by default - i was the one arguing that it should be on by default instead (and it seems few people agree, fine) 19:26:36 Hm? it was on before 19:26:42 but this is pre other 2 changes 19:26:45 oh? maybe before i looked at it :) 19:27:22 let's just settle on having it disabled by default in the initial merge and the backport, it can always be set to be enabled by default later... 19:27:24 sipa: you could argue for that in 0.14 later. 19:27:31 that. 19:27:32 Agree wumpus 19:28:06 there's no need to fix everything in one pull, or one version for that matter, sometimes things are held up too long on minor discussion points 19:28:18 better is better 19:28:22 right. 19:28:26 morcos: gmaxwell: Do you have a strong opinion about the fLimitFree flag in the #9290 19:28:28 backport? 19:28:33 sometimes better is worse, there is totally like an essay on this. :P 19:28:47 sipa: just said fine on not having it on by default, didn't he? 19:29:15 yes he did, I meant in general 19:29:17 jtimon: yes, i'm fine with it being off 19:29:54 MarcoFalke: ah yes that's an important point 19:29:55 MarcoFalke: Didn't see your question until now. will evaluate. 19:30:13 so this is about this comment: https://github.com/bitcoin/bitcoin/pull/9347#discussion_r92503011 19:30:22 Imo it should not matter too much, but I'd rather have a second opinion 19:30:25 [13bitcoin] 15sdaftuar opened pull request #9357: [0.13 backport] Attempt reconstruction from all compact block announcements (060.13...06backport-optimistic-cb) 02https://github.com/bitcoin/bitcoin/pull/9357 19:32:06 I haven't checked if it caused issues with txes evicted from the pool due to low fee. 19:32:53 I need to look into it carefully to make a decision on my view, not going to manage it during the meeting. 19:33:10 ok, other topics? 19:34:02 MarcoFalke: I hadn't seen the flimitFree thing before now, I'll take a look and get back to you after... (same as gmaxwell) 19:34:18 great, thx 19:34:31 We could talk about the compact block announcement stuff not the backports but the change; just so people know what the change is about. 19:34:54 #topic compact block announcement stuff 19:35:13 Right now, if someone sends us a header, we request a block and mark the block in flight. If a compact block (e.g. from a HB mode sender that sends unsolicited ones) shows up while we're waiting.. we just ignore it, instead of trying to reconstruct the block. 19:35:55 This means that if a peer is broken and slowly transmits or fails to reply, the HB mode will fail to work around it. 19:36:44 There is a deep rabbit hole we can go down towards optimal behavior, but what is proposed right now is a super minimal change where even if a block is in flight, we'll still see if we can recover the whole block from just the compact block. And if we can, we take it, and mark the block as complete. 19:37:16 sounds sensible 19:37:42 greater than 2/3rs of all blocks can be recovered from just the compact block (varries a lot based on miner/network behavior) so even this small improvement should be a pretty big help. 19:37:44 there seems some potential for race conditions though 19:37:46 this is especially important with prefill, where, if your peer upgrades to prefill txn in the announcement you can recover the block somtimes and recover from stalling without yourself upgrading 19:38:25 what if the compact block is reconstructed, and then the inflight block comes in? 19:38:39 wumpus: yes, though we don't count on the in-flight to protect against that, and if a full block shows up right now we'll accept it. 19:38:54 wumpus: should not be a problem. there's generally no downside to receiving a block you already have. 19:38:58 wumpus: then its just like someone sending us an unsolicited full block, which we'll process if it's not best already. 19:40:02 sdaftuar: in general there's no downside, just thought it'd be a potential edge case, but if that's handled that's ok 19:40:44 In any case, I think that summarizes where that is, I have several nodes testing live right now.. obviously will need review and testing.. but I just wanted everyone to know what was going on there. 19:44:01 thanks, I assume more questions about this or other topics? 19:46:14 when are we planning to start rc'ing for 0.13.2 19:47:22 dunno, yesterday if it was up to me :p 19:48:04 in any case there's still a few things open and you can help by testing and reviewing: https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.13.2 19:49:40 bah looks like the build is broken 19:50:51 any other topics? if not we'll close the meeting early 19:51:14 very short report: gmaxwell and i have been experimenting with a per-txout utxo cache approach 19:51:15 Close meeting early and make 0.13.2 great again ACK. 19:51:25 so far results don't look too promising 19:51:27 heh 19:51:42 sipa: yeah i haven't looked at that yet 19:51:46 i'm surprised! 19:51:56 i was super optimistic 19:52:00 me too 19:52:05 sipa: so grouping the utxos per transaction turns out to have been a good optimization? I'm surprised too 19:52:12 Well when it's operating totally in memory it's 15% faster even though sipa has not exploited the new structure for better cache intelligence (so its still doing the same dumb flush thing). But when leveldb came into the picture it ate dirt. 19:52:21 15% is for babies 19:52:34 what level are you on morcos :) 19:52:52 i'm going to give a cheers for the sigcache cuckoocache merge now! 19:53:05 mhm, haven't looked at the branch, are the utxo's catched per txout but stored per-tx? 19:53:23 jtimon: both per txout 19:53:23 Assuming the issue isn't extra debugging sipa added, the downside is perhaps that it is just much harder on leveldb and writes a lot more traffic to the leveldb log. 19:53:24 gmaxwell: seems like something where you can per-utxo in memory and per-tx on disk? 19:53:46 BlueMatt: yes I was about to suggest that too 19:53:51 The real gains from the change would come from making the cache smarter, so I thought 15% was great news.. since that likely came from reduced malloc traffic. 19:53:52 i mean might lose all the performance on the boundary, but its worth a shot 19:54:01 sipa: thanks. mhmm, yeah, this is surprising then 19:54:05 BlueMatt: that doesn't solve the O(n^2) issue with large transactions 19:54:10 BlueMatt: yes, I made that observation too.... but it means that read modify write cycles would be needed. 19:54:28 gmaxwell: yeah that would be bad... 19:54:48 lookups are slow, if you need read-modify-write cycles it's not going to help performance 19:54:55 the O(n^2) issue is that a tx with many outputs on every spend needs to write n-i outputs to the database 19:55:19 wumpus: yes, though it might pay for itself by the cache being much more effective. I guess we won't know until after more testing. 19:55:19 sdaftuar: +1. Still catching up, didn't see that got merged. Great to see :) 19:56:08 the other negative is that it looks like this change will require a chainstate reindex. making it compatible with undo files seems really hard. 19:56:44 basically my reason for wanting per-txout cache is that the current behaviour may be good on average, but it's terrible for big transactions 19:56:48 maybe somehow writting txouts in batches could help? (thinking out loud, may be a stupid thought) 19:56:59 requiring everyone to do a chainstate reindex would be bad too :/ 19:57:05 jtimon: we're already batching _all_ writes from many blocks 19:57:37 sipa: I see, it was a stupid thought 19:57:47 anyway, just reporting on an experiment - nothing more at this point 19:57:50 gmaxwell: i'm not sure what you mean about making the cache smarter 19:58:02 wumpus: right now everyone's chainstate is corrupted... to at some point we'll need to do something about that. (TXversions) 19:58:09 writes are pretty fast with leveldb, it's lookups/reads are slow, especially on slow disks 19:58:09 morcos: not wiping the cache after a write 19:58:13 in my view once its only keeping utxos that were actually accessed and not the rest that tagged along with the tx, then thats as smart as it gets 19:58:29 Are we thinking txs are going to become larger in terms of inputs/outputs as Bitcoin grows? UTXO size is constantly growing right? 19:58:34 sipa: sure but you still have to do something when you hit memory limits 19:58:43 Chris_Stewart_5: i wish it were not growing at all 19:59:01 batching writes more is not going to help, and batches are already huge in memory 19:59:02 you can save the things that are in blocks from the top of your mempool, but thats really small... small enough that it can be done pretty effectively with the existing model 19:59:03 Chris_Stewart_5: http://bitcoin.sipa.be/utxo_size.png 19:59:05 morcos: yes the right thing to do is to expire only the oldest entrties at that point. Which is much cleaner when there is no such thing as entry mutation. 19:59:12 I guess it is just interesting to hear the tidbit about terrible performance of large txs. 19:59:14 gmaxwell: requiring everyone to reindex at the same time is not an acceptable solution though :) 19:59:26 ah, oldest, yes ok, but that requires extra state 19:59:30 gmaxwell: maybe it could support two database versions for a while 19:59:42 Chris_Stewart_5: in general, we need to optimize worst-case performance, not average performance 19:59:44 gmaxwell: new reindexes/syncs would use the new format 20:00:01 in any acsae, thanks for trying this experiment 20:00:08 Chris_Stewart_5: as a large difference between worst-case and average means we could be missing DoS opportunities where an attacker can force us into the worsr 20:00:10 wumpus: if it made it N fold faster, than reindex on a new version... might be something we could have happen. I think perhaps we'd want to finish your snapshooting work and other things at the same time. ... in any case it's just an expirement now. 20:00:16 even if it turns out it's not better it's good to know this for sure 20:00:37 it also has resulted in some other optimizations, e.g. the flushing optimization PR that we have right now. 20:00:41 Chris_Stewart_5: but it's really sad when you need to decrease your average performance in order to improve the worst case... because people don't observe the worst case 20:00:51 gmaxwell: if it was possible to convert the old database to the new database without a reindex (e.g. just rewriting) then an upgrade process would be acceptable. But a full reindex? no 20:01:02 Good, the meeting has run over, so all is well with the world. :) 20:01:08 #endmeeting