12017-02-02T00:29:17 *** AaronvanW has quit IRC
22017-02-02T00:41:55 *** Squidicc is now known as squidicuz
32017-02-02T01:03:28 *** waxwing has joined #bitcoin-core-dev
42017-02-02T01:07:53 *** wasi has quit IRC
52017-02-02T01:47:07 *** waxwing has quit IRC
62017-02-02T01:47:32 *** Ylbam has quit IRC
72017-02-02T02:22:47 *** Chris_Stewart_5 has quit IRC
82017-02-02T02:39:32 *** Chris_Stewart_5 has joined #bitcoin-core-dev
92017-02-02T02:57:50 *** waxwing has joined #bitcoin-core-dev
102017-02-02T03:00:02 *** abpa has quit IRC
112017-02-02T03:00:03 *** dermoth has quit IRC
122017-02-02T03:00:49 *** dermoth has joined #bitcoin-core-dev
132017-02-02T03:18:21 *** CubicEarth has quit IRC
142017-02-02T03:19:26 *** CubicEarth has joined #bitcoin-core-dev
152017-02-02T04:51:58 *** chjj has quit IRC
162017-02-02T04:51:59 *** windsok has quit IRC
172017-02-02T04:53:53 *** chris2000 has joined #bitcoin-core-dev
182017-02-02T04:57:11 *** chris200_ has quit IRC
192017-02-02T05:00:08 *** dermoth has quit IRC
202017-02-02T05:00:59 *** dermoth has joined #bitcoin-core-dev
212017-02-02T05:01:37 *** windsok has joined #bitcoin-core-dev
222017-02-02T05:19:26 *** chjj has joined #bitcoin-core-dev
232017-02-02T05:21:43 *** abpa has joined #bitcoin-core-dev
242017-02-02T05:40:17 *** owowo has quit IRC
252017-02-02T05:45:01 *** owowo has joined #bitcoin-core-dev
262017-02-02T05:49:27 *** jtimon has quit IRC
272017-02-02T05:55:26 *** kadoban has quit IRC
282017-02-02T06:09:37 *** echonaut4 has joined #bitcoin-core-dev
292017-02-02T06:09:51 *** pavel_ has joined #bitcoin-core-dev
302017-02-02T06:11:31 *** sanada` has joined #bitcoin-core-dev
312017-02-02T06:12:22 *** paveljanik has quit IRC
322017-02-02T06:12:22 *** cysm has quit IRC
332017-02-02T06:12:48 *** paracyst_ has joined #bitcoin-core-dev
342017-02-02T06:12:58 *** echonaut has quit IRC
352017-02-02T06:12:58 *** bsm117532 has quit IRC
362017-02-02T06:12:58 *** sanada has quit IRC
372017-02-02T06:13:34 *** paracyst has quit IRC
382017-02-02T06:13:46 *** cysm has joined #bitcoin-core-dev
392017-02-02T06:40:28 <bitcoin-git> [bitcoin] sosochain opened pull request #9669: 0.13 (master...0.13) https://github.com/bitcoin/bitcoin/pull/9669
402017-02-02T06:41:23 <bitcoin-git> [bitcoin] fanquake closed pull request #9669: 0.13 (master...0.13) https://github.com/bitcoin/bitcoin/pull/9669
412017-02-02T06:45:32 *** pavel_ has quit IRC
422017-02-02T06:50:58 *** lclc has joined #bitcoin-core-dev
432017-02-02T07:00:55 *** Ylbam has joined #bitcoin-core-dev
442017-02-02T07:16:17 *** paveljanik has joined #bitcoin-core-dev
452017-02-02T07:16:18 *** paveljanik has joined #bitcoin-core-dev
462017-02-02T07:39:40 *** isle2983 has quit IRC
472017-02-02T07:48:17 *** waxwing has quit IRC
482017-02-02T08:04:39 *** BashCo has quit IRC
492017-02-02T08:26:40 *** BashCo has joined #bitcoin-core-dev
502017-02-02T08:55:02 <bitcoin-git> [bitcoin] laanwj closed pull request #9658: Add clang_format.py to help automate code style analysis (master...PR-clang-format) https://github.com/bitcoin/bitcoin/pull/9658
512017-02-02T08:55:08 *** AaronvanW has joined #bitcoin-core-dev
522017-02-02T09:00:57 <bitcoin-git> [bitcoin] laanwj closed pull request #9632: Add clang_static_analysis.py to help automate static analysis runs (master...PR-clang-static) https://github.com/bitcoin/bitcoin/pull/9632
532017-02-02T09:06:15 *** MarcoFalke has joined #bitcoin-core-dev
542017-02-02T09:13:48 <bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/77bd8c4cab67...e30d9287fd48
552017-02-02T09:13:49 <bitcoin-git> bitcoin/master 3eba88d Gregory Sanders: clarify listunspent amount description
562017-02-02T09:13:49 <bitcoin-git> bitcoin/master e30d928 Wladimir J. van der Laan: Merge #9663: [RPC] clarify listunspent amount description...
572017-02-02T09:14:05 <bitcoin-git> [bitcoin] laanwj closed pull request #9663: [RPC] clarify listunspent amount description (master...listoutput) https://github.com/bitcoin/bitcoin/pull/9663
582017-02-02T09:17:13 *** jannes has joined #bitcoin-core-dev
592017-02-02T09:19:44 <bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/e30d9287fd48...ae972a5e996a
602017-02-02T09:19:44 <bitcoin-git> bitcoin/master b9d95bd Douglas Roark: Fix various minor linearization script issues...
612017-02-02T09:19:45 <bitcoin-git> bitcoin/master ae972a5 Wladimir J. van der Laan: Merge #9580: Fix various minor linearization script issues...
622017-02-02T09:19:56 <bitcoin-git> [bitcoin] laanwj closed pull request #9580: Fix various minor linearization script issues (master...linearizefix) https://github.com/bitcoin/bitcoin/pull/9580
632017-02-02T09:43:32 *** rabidus_ is now known as rabidus
642017-02-02T09:43:53 *** Guyver2 has joined #bitcoin-core-dev
652017-02-02T10:00:50 *** lclc has quit IRC
662017-02-02T10:32:02 *** harrymm has quit IRC
672017-02-02T10:47:42 *** harrymm has joined #bitcoin-core-dev
682017-02-02T10:58:21 <bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/ae972a5e996a...4e19efba0331
692017-02-02T10:58:21 <bitcoin-git> bitcoin/master 8fc6989 practicalswift: Remove redundant semicolons
702017-02-02T10:58:22 <bitcoin-git> bitcoin/master 4e19efb Wladimir J. van der Laan: Merge #9556: Remove redundant semicolons...
712017-02-02T10:58:36 <bitcoin-git> [bitcoin] laanwj closed pull request #9556: Remove redundant semicolons (master...remove-redundant-braces) https://github.com/bitcoin/bitcoin/pull/9556
722017-02-02T11:41:34 *** lclc has joined #bitcoin-core-dev
732017-02-02T12:05:13 <bitcoin-git> [bitcoin] laanwj pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/4e19efba0331...7c93952feccb
742017-02-02T12:05:14 <bitcoin-git> bitcoin/master 3e900ac Matt Corallo: Require merge commits merge branches on top of other merge commits...
752017-02-02T12:05:14 <bitcoin-git> bitcoin/master ba94426 Matt Corallo: Test that pushes to bitcoin/bitcoin are signed per verify-commits
762017-02-02T12:05:15 <bitcoin-git> bitcoin/master 7c93952 Wladimir J. van der Laan: Merge #9656: Check verify-commits on pushes to master...
772017-02-02T12:05:31 <bitcoin-git> [bitcoin] laanwj closed pull request #9656: Check verify-commits on pushes to master (master...2017-01-fix-verify-commits) https://github.com/bitcoin/bitcoin/pull/9656
782017-02-02T12:14:45 <bitcoin-git> [bitcoin] laanwj opened pull request #9670: contrib: github-merge improvements (master...2017_01_ghmerge_update) https://github.com/bitcoin/bitcoin/pull/9670
792017-02-02T12:24:26 *** laurentmt has joined #bitcoin-core-dev
802017-02-02T12:24:46 *** laurentmt has quit IRC
812017-02-02T12:26:25 <bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/7c93952feccb...1c2edd9f6707
822017-02-02T12:26:25 <bitcoin-git> bitcoin/master 178454d Jorge Timón: Contrib: Add jtimon pgp keys for commit sigs and future gitian builds
832017-02-02T12:26:26 <bitcoin-git> bitcoin/master 1c2edd9 Wladimir J. van der Laan: Merge #9654: Add jtimon pgp keys for commit sigs and future gitian builds...
842017-02-02T12:26:41 <bitcoin-git> [bitcoin] laanwj closed pull request #9654: Add jtimon pgp keys for commit sigs and future gitian builds (master...jtimon-key-gpg) https://github.com/bitcoin/bitcoin/pull/9654
852017-02-02T13:08:48 *** MarcoFalke has quit IRC
862017-02-02T13:16:27 *** lclc has quit IRC
872017-02-02T13:36:48 *** laurentmt has joined #bitcoin-core-dev
882017-02-02T13:37:40 *** laurentmt has quit IRC
892017-02-02T13:38:20 *** CubicEarth has quit IRC
902017-02-02T14:01:58 *** isle2983 has joined #bitcoin-core-dev
912017-02-02T14:27:33 *** Sosumi has joined #bitcoin-core-dev
922017-02-02T14:27:56 <BlueMatt> <BlueMatt> #9650 (and, by extension, #9634) are probably 0.14
932017-02-02T14:27:58 <gribble> https://github.com/bitcoin/bitcoin/issues/9650 | Better handle invalid parameters to signrawtransaction by TheBlueMatt · Pull Request #9650 · bitcoin/bitcoin · GitHub
942017-02-02T14:28:00 <gribble> https://github.com/bitcoin/bitcoin/issues/9634 | Fail in DecodeHexTx if there is extra data at the end by jtimon · Pull Request #9634 · bitcoin/bitcoin · GitHub
952017-02-02T14:28:03 <BlueMatt> (ie need tag)
962017-02-02T14:28:33 *** waxwing has joined #bitcoin-core-dev
972017-02-02T14:29:04 <wumpus> tagged
982017-02-02T14:29:08 <BlueMatt> thanks
992017-02-02T14:29:40 <wumpus> 9634 seems ready, it would be nice if it had a test though - this seems like something that could regress early if it's not caught by the tests
1002017-02-02T14:53:34 *** jtimon has joined #bitcoin-core-dev
1012017-02-02T15:06:01 <wumpus> it doesn't necessarily need to hold up the pull request, but there's not been a reply from the author yet
1022017-02-02T15:11:51 *** handlex has joined #bitcoin-core-dev
1032017-02-02T15:34:12 *** handlex has quit IRC
1042017-02-02T15:46:28 *** Chris_Stewart_5 has quit IRC
1052017-02-02T15:47:30 *** handlex has joined #bitcoin-core-dev
1062017-02-02T15:52:06 *** handlex has joined #bitcoin-core-dev
1072017-02-02T16:02:01 *** handlex has quit IRC
1082017-02-02T16:02:59 *** Chris_Stewart_5 has joined #bitcoin-core-dev
1092017-02-02T16:07:31 *** laurentmt has joined #bitcoin-core-dev
1102017-02-02T16:07:36 *** laurentmt has quit IRC
1112017-02-02T16:34:35 <BlueMatt> wumpus: I'll take a look at doing that in a bit
1122017-02-02T16:35:48 *** waxwing has quit IRC
1132017-02-02T16:35:51 <BlueMatt> question, would anyone object to me trying to add something like https://github.com/TheBlueMatt/RelayNode/blob/next/c%2B%2B/preinclude.h to bitcoin? it makes atomics replaceable with either std::atomic or a custom thing which has valgrind annotations so that helgrind/drd actually work and dont spew spurious crap all the time
1142017-02-02T16:36:05 <BlueMatt> means atomics get declared with DECLARE_ATOMIC instead of std::atomic x, though
1152017-02-02T16:36:29 <BlueMatt> well, suppose they could be OUR_ATOMIC_OR_NOT, but either way
1162017-02-02T16:37:14 <sipa> why use macros?
1172017-02-02T16:37:27 <BlueMatt> because otherwise you override std::atomic?
1182017-02-02T16:37:40 <BlueMatt> i mean you can say all atomics are now of type MACRO_ATOMIC_TYPE
1192017-02-02T16:38:31 <sipa> no, if you're going to switch all places where atomocs are used to macros, why not switch them with a self defined class directly
1202017-02-02T16:38:52 <sipa> and when you don't need the self defined type, have it be typedefed to be std::atomic
1212017-02-02T16:39:03 <BlueMatt> yea, well same thing either way
1222017-02-02T16:42:03 <BlueMatt> also, due to the presence of _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE, that file would need to be included everywhere prior to the inclusion of any std includes
1232017-02-02T16:42:09 <BlueMatt> well, the atomics could be in a separate file
1242017-02-02T16:42:14 <BlueMatt> but...
1252017-02-02T16:48:42 *** waxwing has joined #bitcoin-core-dev
1262017-02-02T16:50:02 *** abpa has quit IRC
1272017-02-02T17:06:41 *** handlex has joined #bitcoin-core-dev
1282017-02-02T17:16:23 *** nickler has quit IRC
1292017-02-02T17:17:00 *** nickler has joined #bitcoin-core-dev
1302017-02-02T17:27:31 *** kadoban has joined #bitcoin-core-dev
1312017-02-02T17:37:45 <isle2983> please don't talk about coding style at the meeting
1322017-02-02T17:38:03 <isle2983> I have some automation pieces coming togeher, but it might take a little while to get it done
1332017-02-02T17:38:21 <wumpus> BlueMatt: wouldn't it be possible to do this with std::atomic? I like the move towards using standard primitives instead of self-defined ttpes
1342017-02-02T17:38:26 <isle2983> I am working near-full-time on this in the short term
1352017-02-02T17:39:10 <isle2983> a viable end objective might be a 'Nit Bot' that can analize the commits from a PR and detect a few categories of things
1362017-02-02T17:39:39 <isle2983> it looks like you can load a github acct auth token into the TravisCI account as an env var
1372017-02-02T17:39:56 <isle2983> so, securely the travisCI script can post content back to github
1382017-02-02T17:40:43 <wumpus> are you sure that's worth spending so much work on? there are some much more pressing issues than code style
1392017-02-02T17:41:20 <wumpus> we've considered using that but the TravisCI token stuff is easy to circumvent if it's used for testing pulls - the test script for the pull could just output the token or upload it somewhere
1402017-02-02T17:43:21 <isle2983> I am not sure it is the ultimate thing, but there is a cost to the project - you spent 20 minutes talking about it last week
1412017-02-02T17:43:44 <isle2983> and there is some annoyance at trivial pulls
1422017-02-02T17:43:46 <wumpus> that's a rarity, usually it doesn't come up at all
1432017-02-02T17:43:48 *** abpa has joined #bitcoin-core-dev
1442017-02-02T17:44:08 <isle2983> if it can be automated, it should be automated at some point
1452017-02-02T17:45:27 <isle2983> I am also of the school that says code is written once, and read 100s of times afterwards, so make reading easy
1462017-02-02T17:45:46 <isle2983> probably 100,000s of times afterwards for bitcoin
1472017-02-02T17:45:53 <wumpus> BlueMatt: in general the more 'you need to use this special type' rules a project has, the harder it is to contribute to. Though making it easier to valgrind/helgrind is certainly a valuable goal.
1482017-02-02T17:46:23 *** bsm117532 has joined #bitcoin-core-dev
1492017-02-02T17:49:15 <wumpus> isle2983: all too true, but I don't think most of the (superficial) code style makes much difference to that. Having a sensible design and the logic and the reason for things documented/commented is what is important to understanding difficult code
1502017-02-02T17:54:29 <isle2983> totally agreed, but I see that as all the more reason to filter out distractions so that attention can focus 100% on the important things
1512017-02-02T17:54:40 <wumpus> focusing too much on moving around spaces and such takes away the focus from what really matters, and it's much easier so it easily crowds out deeper thinking
1522017-02-02T17:55:30 <wumpus> well as I see it a 'style nit bot' would add distractions, not remove them
1532017-02-02T17:57:22 <gmaxwell> talking about pointless stuff is good for community. :)
1542017-02-02T17:57:40 <wumpus> unless it annoys people
1552017-02-02T17:58:50 <isle2983> in the short term, perhaps. but in the long term it would train submitters to get the PR right offline. Using common tools and scripts also lets the coders just integrate it into their text editor so they don't have to think.
1562017-02-02T17:59:16 <wumpus> there's two ways to go at this sanely: one is to use something like clang-format from the begining. For example in golang projects everyone uses the same formatter with the same style, preventing any disagreements about style. The other is to do best effort and just not worry too much about it. It's too late for the former.
1572017-02-02T17:59:31 <wumpus> our goal is not to 'train submitters'
1582017-02-02T18:00:52 <wumpus> it's just importing concerns that shouldn't be part of this, creating extra bureaucratic barriers
1592017-02-02T18:01:47 <BlueMatt> wumpus: yea, I tend to agree, I'm not sure I really want to do it, but if valgrind doesnt get an update to actually support std::atomic I might have to do it
1602017-02-02T18:03:57 <gmaxwell> wumpus: I wouldn't worry too much; thats what review is for.. and that is the kind of thing everyone will spot, no one should mind fixing up, especially since it would be visible in practice all over... Obviously preferable to not.
1612017-02-02T18:05:21 <wumpus> gmaxwell: I do worry about it. We've had this problem in the past with a certain person commenting on every pull about e.g. sorting include headers, whitespace, and so on, and it was incredibly annoying
1622017-02-02T18:07:09 <gmaxwell> ah, I was commenting more on the macro for atomics.
1632017-02-02T18:07:23 <gmaxwell> Yes. It certantly doesn't work if everyeone doesn't support it, especially if its merely cosmetic.
1642017-02-02T18:08:00 <wumpus> right, I'm not worried about using a custom type for std::atomic, it shouldn't be used that much anyway. THough it seems to be something a tool should just handle.
1652017-02-02T18:08:08 <gmaxwell> but "make valgrind usable to help us find data-races" is a substantial boon I hope we could all support. :)
1662017-02-02T18:09:52 <wumpus> sure
1672017-02-02T18:10:27 <wumpus> though it seems reasonably important to have it work for std::atomic so that the tool can analyse all projects using it, without needing special support at that side
1682017-02-02T18:13:58 *** lclc has joined #bitcoin-core-dev
1692017-02-02T18:18:30 <cfields> wumpus: i wonder if that one, specifically, gets solved with newer versions of std libraries. For ex, iirc newer libc++ adds attributes to std::mutex/std::lock, etc, so that we won't need the wrappers in threadsafety.h
1702017-02-02T18:18:43 <isle2983> I am happy to talk more about style automation on side channels - or also hear suggestions for what else I could be working on. I am in the learning phase and am open to anything that needs an extra brain and can help the project. Yielding now for important topics. Thanks.
1712017-02-02T18:19:14 *** Evel-Laptop has joined #bitcoin-core-dev
1722017-02-02T18:20:45 *** CubicEarth has joined #bitcoin-core-dev
1732017-02-02T18:21:53 *** paveljanik has quit IRC
1742017-02-02T18:22:32 <wumpus> isle2983: to be clear I'm not trying to bash your work, just trying to set expectation how these kind of things are received, so you don't spend a lot of work automating something that won't be used
1752017-02-02T18:23:49 <wumpus> cfields: indeed it could also be improved from the side of the library
1762017-02-02T18:24:13 <cfields> yes, found it: https://reviews.llvm.org/D14731
1772017-02-02T18:24:39 <cfields> i guess runtime tools wouldn't get access to those attributes, though
1782017-02-02T18:25:32 <wumpus> they could if it would be exposed in debug metadata in some way
1792017-02-02T18:26:28 <wumpus> isle2983: as for things to be working on, improving the tests is always very welcome :)
1802017-02-02T18:28:44 <Chris_Stewart_5> isle2983: If you are interested, I'm working on a new testing paradigm in core in #8469
1812017-02-02T18:28:47 <gribble> https://github.com/bitcoin/bitcoin/issues/8469 | [POC] Introducing property based testing to Core by Christewart · Pull Request #8469 · bitcoin/bitcoin · GitHub
1822017-02-02T18:29:59 <Chris_Stewart_5> I've found it to be a useful way to get familiar with the core codebase as well
1832017-02-02T18:30:16 <Chris_Stewart_5> wrt to what wumpus said about improving/adding tests
1842017-02-02T18:30:35 <isle2983> wumpus: hey no worries. I wouldn't want to work on a project that didn't have strong skepticism and pushback
1852017-02-02T18:31:04 <isle2983> Chris_Stewart_5: cool, I will take a peek. Thanks.
1862017-02-02T18:34:17 <sipa> BlueMatt: are relaxed reads from an atomic even recognizable from the binary?
1872017-02-02T18:34:39 <BlueMatt> relaxed I dont know, but certainly most reads/writes show up in the st as load()/store()
1882017-02-02T18:35:50 <BlueMatt> its unclear to me whether helgrind is just being overly conservative and reporting them anyway because they could still be logic-races, or whether helgrind also isnt taking into account the ordering there
1892017-02-02T18:36:16 <BlueMatt> its also unclear to me whether libc++ is doing the right thing with _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE set in their atomics and I just dont have that mapped to helgrind's equivalent
1902017-02-02T18:36:23 <BlueMatt> which may be an easy fix
1912017-02-02T18:36:41 <BlueMatt> (but that hadnt previously fixed it months ago when I was doing that for the relay network code)
1922017-02-02T18:37:19 *** handlex has joined #bitcoin-core-dev
1932017-02-02T18:38:08 *** lclc has quit IRC
1942017-02-02T18:38:14 *** handlex has quit IRC
1952017-02-02T18:46:15 *** Guyver2 has quit IRC
1962017-02-02T18:48:47 *** GAit has joined #bitcoin-core-dev
1972017-02-02T18:55:42 <bitcoin-git> [bitcoin] TheBlueMatt opened pull request #9671: Fix super-unlikely race introduced in 236618061a445d2cb11e72 (master...2017-02-fix-initnode-race) https://github.com/bitcoin/bitcoin/pull/9671
1982017-02-02T18:55:45 <BlueMatt> cfields: I blame you for ^, btw, you asked me to do it :p
1992017-02-02T18:59:19 <cfields> BlueMatt: In my defense I'm pretty sure i only said that it was ugly and that it should be cleaned up later, but I didn't complain when you went ahead and moved it either. So I'll take the blame :)
2002017-02-02T18:59:50 *** MarcoFalke has joined #bitcoin-core-dev
2012017-02-02T18:59:52 <BlueMatt> lol, yea, well /someone/ should have bothered to look at the implications :(
2022017-02-02T19:00:02 <wumpus> #startmeeting
2032017-02-02T19:00:02 <lightningbot> Meeting started Thu Feb 2 19:00:02 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
2042017-02-02T19:00:02 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
2052017-02-02T19:00:07 <jonasschnelli> hi
2062017-02-02T19:00:08 <BlueMatt> oh thats today?
2072017-02-02T19:00:18 <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
2082017-02-02T19:00:29 <sipa> yow.
2092017-02-02T19:00:37 <luke-jr> BlueMatt: no, it's fake news.
2102017-02-02T19:00:37 <wumpus> BlueMatt: it's thursday I hope?
2112017-02-02T19:00:38 <luke-jr> /s
2122017-02-02T19:00:49 <sipa> luke-jr: alternative news
2132017-02-02T19:00:52 <BlueMatt> wumpus: alternative facts
2142017-02-02T19:00:53 <BlueMatt> heh
2152017-02-02T19:00:59 <sipa> jinx
2162017-02-02T19:01:28 <sipa> i don't have topics
2172017-02-02T19:01:32 <wumpus> foremost topic would be what to still include in 0.14, as rc1 release is planned for monday
2182017-02-02T19:02:09 <gmaxwell> I propose not including any bugs.
2192017-02-02T19:02:13 <BlueMatt> I think the current list (+#9671) are going to be required for release, sooooo
2202017-02-02T19:02:15 <gribble> https://github.com/bitcoin/bitcoin/issues/9671 | Fix super-unlikely race introduced in 236618061a445d2cb11e72 by TheBlueMatt · Pull Request #9671 · bitcoin/bitcoin · GitHub
2212017-02-02T19:02:25 <jonasschnelli> These are the issues tagged for 0.14 : https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+is%3Aissue+milestone%3A0.14.0
2222017-02-02T19:02:34 <MarcoFalke> I think only remaining is the net stuff?
2232017-02-02T19:03:02 <cfields> rebasing 9609 now.
2242017-02-02T19:03:14 <instagibbs> hi
2252017-02-02T19:03:25 <BlueMatt> MarcoFalke: we added the signrawtransaction assertion too
2262017-02-02T19:03:25 <wumpus> do we have a fix for #9027 in the works yet?
2272017-02-02T19:03:26 <gribble> https://github.com/bitcoin/bitcoin/issues/9027 | Unbounded reorg memory usage · Issue #9027 · bitcoin/bitcoin · GitHub
2282017-02-02T19:03:27 <jonasschnelli> Imo the importmulti fixes can be done after 0.14
2292017-02-02T19:03:28 <BlueMatt> and importmulti
2302017-02-02T19:03:29 <cfields> probably a good time to call for release notes additions
2312017-02-02T19:03:33 <BlueMatt> wumpus: I vote we push that one
2322017-02-02T19:03:36 <BlueMatt> (to 0.15)
2332017-02-02T19:03:48 <wumpus> BlueMatt: ok, no problem with that
2342017-02-02T19:03:55 <BlueMatt> because not-regression, it turns out
2352017-02-02T19:04:03 <sipa> not regression?
2362017-02-02T19:04:07 <BlueMatt> slightly worse than it tused to be, but not so massively so that I think we definitely need to fix it asap
2372017-02-02T19:04:09 <gmaxwell> changing the API in the very next release would be unfortunate.
2382017-02-02T19:04:24 <achow101> are the three rpc things necessary for this release?
2392017-02-02T19:04:24 <BlueMatt> gmaxwell: context?
2402017-02-02T19:04:29 <BlueMatt> sipa: you're the one who decided it wasnt!
2412017-02-02T19:04:30 <jonasschnelli> gmaxwell: importmulti?
2422017-02-02T19:04:32 <sipa> gmaxwell: fixing 9027 does not need an api change afaik
2432017-02-02T19:04:42 <sipa> BlueMatt: oh!
2442017-02-02T19:05:04 <wumpus> we could disable importmulti if it isn't safe in time for the release
2452017-02-02T19:05:31 <sipa> or leave it undocumented?
2462017-02-02T19:05:44 <wumpus> depends on whether the issue of #9491 is serious enough
2472017-02-02T19:05:45 <gribble> https://github.com/bitcoin/bitcoin/issues/9491 | Importmulti api is confusing in a way that could lead to funds loss. · Issue #9491 · bitcoin/bitcoin · GitHub
2482017-02-02T19:05:58 <gmaxwell> fixing the fact that it's very easy to fail to rescan anything, when you thought it was... does.
2492017-02-02T19:06:22 <wumpus> yes undocumented or could add a "warning: experimental, API will likely change next release" in any case too
2502017-02-02T19:06:41 <jonasschnelli> Or we just fix 9491... seems not very complex?
2512017-02-02T19:06:51 <jonasschnelli> Can fix in rc2 if it's to late for monday?
2522017-02-02T19:07:26 <wumpus> sure
2532017-02-02T19:07:36 <wumpus> but there's no guarantee there is a rc2
2542017-02-02T19:07:40 <gmaxwell> okay, lets see where that goes in the next couple days.
2552017-02-02T19:07:47 <wumpus> I don't know how hard it is? it seems to have caused quite a discussion but no fix
2562017-02-02T19:07:50 <luke-jr> importmulti seems akin to importprivkey which shouldn't be used by users anyway?
2572017-02-02T19:07:57 <gmaxwell> We can hide it right before cutting RC1 if nothing else.
2582017-02-02T19:08:10 <wumpus> yes
2592017-02-02T19:08:19 <gmaxwell> ashame, as it's a nice improvement.
2602017-02-02T19:08:27 <sipa> i think the fix would be easy?
2612017-02-02T19:08:43 <gmaxwell> sure, that is why I said lets see.
2622017-02-02T19:08:52 <BlueMatt> who is working on it?
2632017-02-02T19:08:53 <achow101> can't you just change the default timestamp to be 0?
2642017-02-02T19:08:55 <gmaxwell> but we have a fallback if it doesn't get fixed.
2652017-02-02T19:08:58 <BlueMatt> "lets see" only works if someone does it :p
2662017-02-02T19:09:08 <gmaxwell> achow101: then there is no easy way to express now.
2672017-02-02T19:09:29 <jonasschnelli> Would a fix where we set the importmulti timestamp to 0 instead of "now" do it for 0.14?
2682017-02-02T19:09:37 <luke-jr> gmaxwell: using time() from your OS?
2692017-02-02T19:09:40 <jonasschnelli> *default timestamp
2702017-02-02T19:10:04 <achow101> -1 for "now"
2712017-02-02T19:10:05 <wumpus> or have no default at all and require a time to be specified
2722017-02-02T19:10:14 <jonasschnelli> wumpus: +1
2732017-02-02T19:10:19 <luke-jr> wumpus: no default at all is nice since it allows a default to be chosen later
2742017-02-02T19:10:31 <jonasschnelli> 0 as timestamp is very inefficient.
2752017-02-02T19:10:34 <gmaxwell> Lets not hash it out here, there is an issue.
2762017-02-02T19:10:45 <jonasschnelli> Okay. Lets comment there. Agree with gmaxwell
2772017-02-02T19:10:57 <gmaxwell> I agree with jonasschnelli in the sense there that we really have to stop assuming a full rescan is possible.
2782017-02-02T19:11:28 <wumpus> good point, yes
2792017-02-02T19:11:34 <jonasschnelli> Is also very inefficient if you have pruned or run hybrid SPV
2802017-02-02T19:11:37 <wumpus> it certainly shouldn't be the default
2812017-02-02T19:11:37 <gmaxwell> It takes many hours on my normal development system, and is still quite slow even on the fastest hardware available. But avoiding the rescan takes second seat to surprising the user. :)
2822017-02-02T19:11:40 <wumpus> it's inefficient and lazy
2832017-02-02T19:12:37 <gmaxwell> in my view, except for certan recover operations that are infrequently done-- rescan effectively doesn't work anymore (takes more time than converting your entire usage to a third party api...)
2842017-02-02T19:13:22 <wumpus> users of the API should be encouraged to keep track of key birthdates
2852017-02-02T19:13:31 *** jnewbery has joined #bitcoin-core-dev
2862017-02-02T19:14:05 <gmaxwell> if we define a new private key format in the not so far future, we should make sure its string clearly integrates a birthdate. :P
2872017-02-02T19:14:25 <BlueMatt> ok, so discuss on the issue....next topic?
2882017-02-02T19:14:26 <wumpus> a full rescan is indeed only something that should be done for infrequent recovery reasons
2892017-02-02T19:15:58 <wumpus> no other topics?
2902017-02-02T19:16:22 <MarcoFalke> shortest meeting ever
2912017-02-02T19:16:58 <wumpus> I had expected heated debates on what to include last-minute in 0.14 and why to delay the rc, what a disappointment! </s>
2922017-02-02T19:17:02 <BlueMatt> great, lets get 0.14 done so I can get back to writing code :)
2932017-02-02T19:17:10 <jonasschnelli> Heh.
2942017-02-02T19:17:13 <sdaftuar> let's talk about code style again
2952017-02-02T19:17:15 <BlueMatt> wumpus: I vote we push it back a month so we can do all the things we wanted to a month ago :p
2962017-02-02T19:17:25 <wumpus> BlueMatt: lol!
2972017-02-02T19:17:42 <BlueMatt> wait, i had something to talk about re: cde style
2982017-02-02T19:17:43 <BlueMatt> hum
2992017-02-02T19:17:54 <gmaxwell> BlueMatt: die
3002017-02-02T19:17:55 <sdaftuar> i'll get the baseball bat
3012017-02-02T19:17:56 <BlueMatt> oh, auto
3022017-02-02T19:18:02 <jonasschnelli> Bumpfee: is there a reason why the logic is in the rpcwallet.cpp and not in wallet.cpp?
3032017-02-02T19:18:13 <jonasschnelli> Makes it really hard to use in the gui...
3042017-02-02T19:18:24 <BlueMatt> jonasschnelli: please move it, agreed
3052017-02-02T19:18:25 <luke-jr> jonasschnelli: it can be moved
3062017-02-02T19:18:25 <sdaftuar> jonasschnelli: i think we can refactor as needed
3072017-02-02T19:18:29 <luke-jr> in 0.15*
3082017-02-02T19:18:33 <wumpus> jonasschnelli: because it's only used in rpcwallet.cpp, if you need it in a more general place move it
3092017-02-02T19:18:40 <jonasschnelli> Okay.
3102017-02-02T19:18:54 <sipa> BlueMatt: i am strongly in favor of auto.
3112017-02-02T19:18:59 * sipa hides
3122017-02-02T19:19:00 <wumpus> jonasschnelli: although moving everything to wallet.cpp isn't very nice either, we should refactor the wallet code some day
3132017-02-02T19:19:03 <luke-jr> I did suggest it earlier, but didn't seem like a blocker for merging
3142017-02-02T19:19:06 <wumpus> I'm also in favor of auto
3152017-02-02T19:19:15 <BlueMatt> sipa: it makes certain review much, much harder (I often grep for "everywhere X is used")
3162017-02-02T19:19:15 <luke-jr> wumpus: well, it's wallet code..
3172017-02-02T19:19:23 <BlueMatt> and have already missed things as a result
3182017-02-02T19:19:24 <wumpus> luke-jr: so? not all wallet code needs to be in one file
3192017-02-02T19:19:35 <wumpus> forbidding auto is just masochism
3202017-02-02T19:19:41 <jonasschnelli> wumpus: yes. Thats a good point.
3212017-02-02T19:19:42 <BlueMatt> wumpus: i wasnt voding forbidding it
3222017-02-02T19:19:46 <sipa> BlueMatt: introduce an incompatible change to the type, and recompile. tadaa, all places it is used
3232017-02-02T19:19:49 <BlueMatt> only carefully considering its use
3242017-02-02T19:19:50 <cfields> sipa: same. BlueMatt: maybe paste the thread in question?
3252017-02-02T19:19:56 <luke-jr> I like auto when the type is implied by some other type; eg, instead of xyz::value_type
3262017-02-02T19:20:05 <jtimon> yeah, but not forbidding it doesn't mean recommending it always either
3272017-02-02T19:20:10 <BlueMatt> https://github.com/bitcoin/bitcoin/pull/9609#discussion_r98335218
3282017-02-02T19:20:15 <wumpus> we have a whole src/wallet directory which could have tons of different implementation files for different facets of the wallet, instead of stashing it all into one file
3292017-02-02T19:20:39 <BlueMatt> (I believe gmaxwell's comment there was intedned for a different line)
3302017-02-02T19:20:44 <jonasschnelli> yes. Stuff like coin selection should be more modular
3312017-02-02T19:21:04 <wumpus> sure, as with any use of any c++ statement, use of auto should be measured
3322017-02-02T19:21:08 <MarcoFalke> Lets do it after priority removal
3332017-02-02T19:21:18 <MarcoFalke> Otherwise we step on each others toes
3342017-02-02T19:21:24 <wumpus> if you have some specific cases where it's bad to use auto, please document them
3352017-02-02T19:21:27 <BlueMatt> wumpus: mostly only things that are /actually/ a mile of text to type, imo
3362017-02-02T19:21:57 <sipa> BlueMatt: and not needing to change things all over the place when you turn a tuple into a struct
3372017-02-02T19:22:07 <sipa> or add a wrapper
3382017-02-02T19:22:08 <BlueMatt> sipa: I have no problem reviewing sed-based changes
3392017-02-02T19:22:16 <BlueMatt> in fact prefer that
3402017-02-02T19:22:18 <wumpus> BlueMatt: there's plenty of those - c++ is overly verbose, auto is a great advancement
3412017-02-02T19:22:21 <sipa> they're still annoying to fo
3422017-02-02T19:22:27 <BlueMatt> since I'm gonna go read every single place the change effected anyway
3432017-02-02T19:22:28 <BlueMatt> to review
3442017-02-02T19:22:30 <sipa> *to do
3452017-02-02T19:22:50 <sipa> and of course, let's consider on a case by case basis
3462017-02-02T19:22:55 <wumpus> right
3472017-02-02T19:22:56 <BlueMatt> wumpus: sure, iterators in iterators, np
3482017-02-02T19:23:03 <BlueMatt> yea, ok, whatever, I'll shut up
3492017-02-02T19:23:04 <sipa> but in my own preference, that is overwhelmingly the case
3502017-02-02T19:24:00 <cfields> well the specific case here is for loops. "for (auto& foo : bar)"
3512017-02-02T19:24:01 <MarcoFalke> Agree with BlueMatt, that auto should not be used unless necessary.
3522017-02-02T19:24:04 <cfields> any reason not to use auto there?
3532017-02-02T19:24:04 <jtimon> BlueMatt: the question is, do you have a general advice on when not to use auto?
3542017-02-02T19:24:22 <wumpus> it's never *necessary* auto is just nice
3552017-02-02T19:24:24 <BlueMatt> cfields: yes, so I can grep and review if the type's behavior changes in some way
3562017-02-02T19:24:36 <BlueMatt> jtimon: personally, if the type really, really doesnt matter
3572017-02-02T19:24:40 <luke-jr> cfields: if it's liable to produce bad results with bar changing under it
3582017-02-02T19:24:50 <BlueMatt> (which means very rarely use it)
3592017-02-02T19:24:56 <jtimon> BlueMatt: I'm afraid "doesn't matter" it's too vague here
3602017-02-02T19:25:04 <wumpus> I don't think this is going anywhere, too much isagreement
3612017-02-02T19:25:04 <BlueMatt> eg if you're taking an iterator and passing it through to another function
3622017-02-02T19:25:08 <wumpus> any other topics?
3632017-02-02T19:25:22 <wumpus> BlueMatt: function arguments can't use auto, right?
3642017-02-02T19:25:29 <sipa> indeed
3652017-02-02T19:25:50 <sipa> c++14 and later introduce some auto types in lambdas
3662017-02-02T19:25:53 <BlueMatt> wumpus: correct, but eg doing auto it = map.find(thing); if (it != ma.end()) DoThingWith(*it);
3672017-02-02T19:25:56 <BlueMatt> is like not a problem
3682017-02-02T19:26:23 <BlueMatt> auto it = map.find(thing); if (it != ma.end()) ILikePonies(it->second.rainbows); I do not like
3692017-02-02T19:26:24 <sipa> BlueMatt: how is that different from a for (const auto& x : container) {}
3702017-02-02T19:27:08 <BlueMatt> sipa: because in the specific case here the thing in the loop is not defined to take a specific type
3712017-02-02T19:27:12 <BlueMatt> it is templated
3722017-02-02T19:27:15 <jtimon> my question was, do you have a deductive method for finding the not ok cases instead of an inductive one for the "not a problem cases"?
3732017-02-02T19:27:21 <sipa> you can see that as an oblivious loop with iterators, and passing *it to a function that is tje body of the loop
3742017-02-02T19:27:38 <BlueMatt> jtimon: <BlueMatt> jtimon: personally, if the type really, really doesnt matter
3752017-02-02T19:28:12 <sipa> i see your point, but i don't think it weighs up against the benefitd
3762017-02-02T19:28:14 <sipa> *benefits
3772017-02-02T19:28:20 <wumpus> if you're iterating over some container, the type of container usually really doesn't matter, unless you make specific assumptions (but then you'd generally not be using a range for loop in the first place)
3782017-02-02T19:28:41 <BlueMatt> wumpus: imo if you are ever actually dereferencing the type you should not use auto
3792017-02-02T19:28:56 <sipa> BlueMatt: your own example dereferences...
3802017-02-02T19:28:57 <BlueMatt> if you're dereferencing the iterator to eg a pair or just taking the element and passing it to something else, ok
3812017-02-02T19:29:08 <BlueMatt> but if you're dereferencing it and accessing something inside it, no
3822017-02-02T19:29:27 <sipa> then we might as well not use it at all, i think
3832017-02-02T19:29:46 <jtimon> ok, I think I get what you mean by "doesn't matter" now
3842017-02-02T19:29:52 <BlueMatt> sipa: there are many places where you might do for (auto& thing: list) ActOn(thing);
3852017-02-02T19:29:55 <BlueMatt> thats reasonable
3862017-02-02T19:30:08 <sipa> requiring programmers to spell out redundant information just so you can grep for it seems extreme to me
3872017-02-02T19:30:27 <gmaxwell> sipa: so functions shouldn't have prototypes? :)
3882017-02-02T19:30:27 <BlueMatt> yes, I didnt expect people to agree with me...I have extreme distaste for auto, personally
3892017-02-02T19:30:33 <wumpus> yes, that' extreme, and not going tohhappen. Just use smarter tools.
3902017-02-02T19:30:42 <BlueMatt> wumpus: suggestions?
3912017-02-02T19:31:00 * BlueMatt would love a grep --allusesoftype thing
3922017-02-02T19:31:30 <wumpus> it should be fairly easy to implement using clang's parser, would be surprised if it doesn't exist
3932017-02-02T19:31:50 <gmaxwell> There is another side to it is that auto enables you to write code that acts on a type while having no idea of the type yourself. Which is safe 99% of the time and deadly the rest.
3942017-02-02T19:32:20 <gmaxwell> because in C++ not all operations which are catgorically unsafe on a type are actually stopped by typechecking. :(
3952017-02-02T19:33:41 <gmaxwell> I have an auto to a container... and then I extract an auto to an iterator on it and erase things. Is my code guilty of the sin of using an invalidated iterator? It depends on what container was in use, and that was hid by auto...
3962017-02-02T19:34:08 <gmaxwell> But... that sort of thing is an edge case, I'd love to see a realistic list of where auto is likely to cause problems, just to keep it in mind.
3972017-02-02T19:34:29 <wumpus> right - just keep it in mind while reviewing
3982017-02-02T19:34:43 <wumpus> and if there are well-defined cases where auto is dangerous, they should be documented in the developer notes
3992017-02-02T19:34:57 <BlueMatt> ehh, ok, well I go read all of wallet half the time reviewing wallet changes, i guess I'll just start doing that for net, too :p
4002017-02-02T19:35:13 <BlueMatt> (not a bad thing, that)
4012017-02-02T19:35:29 <gmaxwell> unfortunately, auto is most interesting when you have some horrible complex signature. But those are the cases where it is also more of an issue.
4022017-02-02T19:36:07 <cfields> gmaxwell: for(auto& : foo) doesn't give you an iterator though, just a reference. So imo that should be highly preferred when possible to avoid your example.
4032017-02-02T19:36:19 <wumpus> well no, it's most interesting for bog-standard loops, 99% of the cases. If you're doing anything horribly complex, that's probably where you should be careful
4042017-02-02T19:36:32 <jtimon> BlueMatt: we can agree that auto is totally fine for unittests too, right? :p
4052017-02-02T19:37:20 <cfields> (preferred over auto foo = bar.begin(), that is)
4062017-02-02T19:37:23 <gmaxwell> wumpus: well my point is that stating the type explicitly is just as easy as auto when it's simple and obvious.
4072017-02-02T19:37:29 <sipa> gmaxwell: when you have some horrid complex type signature, best practice is to introduce a typedef for it... that also results in succint usage, and lacks the review concerns that BlueMatt has i think
4082017-02-02T19:37:36 <jtimon> I agree it removes clarity some times
4092017-02-02T19:37:48 <wumpus> gmaxwell: it's *easy* but the point is to avoid unnecessary verbosity/typing, not so you can forget the type
4102017-02-02T19:37:54 <BlueMatt> sipa: yes, agreed, also means dont use auto in place, which some people like to do
4112017-02-02T19:38:10 <jtimon> but I don't have a clear criterion on when to use it like matt
4122017-02-02T19:38:14 <BlueMatt> wumpus: I'm generally 100% in favor of extra verbosity
4132017-02-02T19:38:25 <wumpus> e.g. to avoid having to type std::vector<std::Strring> for the zillionth time
4142017-02-02T19:38:25 <gmaxwell> fking java programmers. :P
4152017-02-02T19:38:27 <wumpus> BlueMatt: go use java
4162017-02-02T19:38:28 <BlueMatt> extra verbosity generally means less magic, which makes review easier
4172017-02-02T19:38:33 <BlueMatt> lol, i expected that....
4182017-02-02T19:38:36 <gmaxwell> (though on type signatures, I usually also prefer being explicit more often)
4192017-02-02T19:38:44 <wumpus> BlueMatt: that's not categorically true, more verbosity also means more distraction
4202017-02-02T19:38:51 <sipa> agree
4212017-02-02T19:38:58 <BlueMatt> wumpus: well, ok, more verbosity as long as it actually provides information
4222017-02-02T19:39:05 <sipa> nobody actually looks at what large type definitions contain
4232017-02-02T19:39:06 <wumpus> having lot's of boilerplate does *not* equal easier review
4242017-02-02T19:39:10 *** Evel-Laptop has quit IRC
4252017-02-02T19:39:11 <BlueMatt> public static void main(String[] args) {} probably doesnt provide more information
4262017-02-02T19:39:26 <wumpus> anyhow
4272017-02-02T19:39:29 <BlueMatt> sipa: I do!
4282017-02-02T19:39:35 <wumpus> any other topics? this is going the wrong way
4292017-02-02T19:39:40 <sipa> haha
4302017-02-02T19:39:44 <luke-jr> lol
4312017-02-02T19:40:23 <BlueMatt> soooo...endmeeting?
4322017-02-02T19:40:26 <wumpus> #endmeeting
4332017-02-02T19:40:26 <lightningbot> Meeting ended Thu Feb 2 19:40:26 2017 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
4342017-02-02T19:40:26 <lightningbot> Minutes: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-02-02-19.00.html
4352017-02-02T19:40:26 <lightningbot> Minutes (text): http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-02-02-19.00.txt
4362017-02-02T19:40:26 <lightningbot> Log: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-02-02-19.00.log.html
4372017-02-02T19:40:39 <gmaxwell> wumpus: your request is a little explicit, you could have just said... for auto meetingstep.
4382017-02-02T19:41:01 <BlueMatt> heh
4392017-02-02T19:41:03 <wumpus> gmaxwell: #endcurrentcontext
4402017-02-02T19:42:22 <jtimon> regarding https://github.com/bitcoin/bitcoin/pull/9609#issuecomment-276974228 I generally dislike habing the release branches differ from the master they branched from more than they need "artificially"
4412017-02-02T19:43:25 <jtimon> also for "we can fix that after the branch" types of things
4422017-02-02T19:44:31 <MarcoFalke> jtimon: master is for testing, release branches are for production
4432017-02-02T19:44:43 <jtimon> MarcoFalke: I know
4442017-02-02T19:44:54 <gmaxwell> The asserts don't leve enough record in any case, during development.
4452017-02-02T19:44:55 *** lclc has joined #bitcoin-core-dev
4462017-02-02T19:45:26 <MarcoFalke> jtimon: The thing is you may want asserts during testing, but you may not want them in production
4472017-02-02T19:45:31 <gmaxwell> I've had asserts fire and have had no idea which assert fired or why, only that my daemon went away. So a crash was introduced but we didn't even learn from it.
4482017-02-02T19:45:46 <MarcoFalke> jtimon: We can not compile without asserts, so this is the only way?
4492017-02-02T19:46:07 <jtimon> MarcoFalke: perhaps something like if(debug) ?
4502017-02-02T19:46:33 <gmaxwell> (moreover, differences in behavior between testing and production exposes you to bugs that the testing code fixes)
4512017-02-02T19:46:41 <gmaxwell> e.g. when an asserts check has a side effect.
4522017-02-02T19:47:33 <gmaxwell> In any case, asserts around the net stuff are also special because the testing there is woefully inadequate right now.
4532017-02-02T19:48:00 <gmaxwell> As evidenced by the fact that the version assert was there for months and only triggered a couple times, but yet was still triggerable!
4542017-02-02T19:48:36 <jtimon> yeah, I'm talking more generally about the "we can fix that after the branch" or let's do A and master and B in 0.14 attitude
4552017-02-02T19:48:44 <luke-jr> #9619 has plenty of ACKs; shall I go add test cases, or merge this and PR test cases separate?
4562017-02-02T19:48:46 <gribble> https://github.com/bitcoin/bitcoin/issues/9619 | Bugfix: RPC/Mining: GBT should return 1 MB sizelimit before segwit activates by luke-jr · Pull Request #9619 · bitcoin/bitcoin · GitHub
4572017-02-02T19:49:25 <BlueMatt> I do believe we need to start talking about making our assert infrastructure better - building test bins by default with DEBUG_LOCKORDER and many more asserts, with most of those asserts just printing warnings in production
4582017-02-02T19:50:47 <jtimon> yeah, and that doesn't require to have different code, maybe just change the value of a constant or something
4592017-02-02T19:52:15 <jtimon> or do all that stuff when -debug
4602017-02-02T19:52:58 <bitcoin-git> [bitcoin] isle2983 closed pull request #9459: Improvements to copyright_header.py and some minor copyright header tweaks. (master...PR-copyright-script-improve) https://github.com/bitcoin/bitcoin/pull/9459
4612017-02-02T19:53:25 <bitcoin-git> [bitcoin] isle2983 closed pull request #9603: Add basic_style.py to automate some style checking. (master...PR-basic-style) https://github.com/bitcoin/bitcoin/pull/9603
4622017-02-02T19:53:55 *** chjj has quit IRC
4632017-02-02T20:09:48 *** Chris_Stewart_5 has quit IRC
4642017-02-02T20:14:04 <jtimon> ugh, I missed the change in style... https://github.com/bitcoin/bitcoin/pull/9566#discussion_r98842239
4652017-02-02T20:15:39 <jtimon> there's a lot of code of the form
4662017-02-02T20:15:39 <jtimon> if (checkwhatever)
4672017-02-02T20:15:39 <jtimon> return false/error(...)/state.DoS(...)
4682017-02-02T20:15:39 <jtimon> that's no suddenly against our style...
4692017-02-02T20:15:56 <jtimon> s/that's no/that's now/
4702017-02-02T20:16:27 <sipa> jtimon: indeed. my preference is that anytime you're touching code (except simple move-onlys), you adapt the style of the code
4712017-02-02T20:16:39 <sipa> but no big refactors changing the style all over
4722017-02-02T20:17:56 <jtimon> right, my point is I would prefer that our rules for style was something that we're actually closer to achieve, even if that's something less strict
4732017-02-02T20:19:09 <jtimon> this is an invitation for someone to create a PR correcting some of those cases
4742017-02-02T20:21:01 *** chjj has joined #bitcoin-core-dev
4752017-02-02T20:22:27 *** Chris_Stewart_5 has joined #bitcoin-core-dev
4762017-02-02T20:27:40 <isle2983> it seems strange to me that the style is a moving target. is there some nuance that I am missing?
4772017-02-02T20:28:18 <isle2983> I would think that one of the off-the-shelf choices would be best.
4782017-02-02T20:28:47 <sipa> isle2983: the original satoshi code used a very arcane style, which many people dislike, and has in practice not been followed by contributors after satoshi left
4792017-02-02T20:29:10 <sipa> at some point we formalized the effective style people were using in the developer notes, but reviewers didn't actually enforce it
4802017-02-02T20:29:34 <sipa> plus, there was a strong "mimick the style around what you're editing" tendency, which doesn't really help converging
4812017-02-02T20:29:50 <sipa> i hope that going forward we start enforcing it
4822017-02-02T20:29:55 <sipa> in modified code, at least
4832017-02-02T20:30:05 <sipa> https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#developer-notes
4842017-02-02T20:36:00 <isle2983> yeah, that makes sense to me. but I am not going to poke the enforcement side too hard.
4852017-02-02T20:36:33 *** F0sea has joined #bitcoin-core-dev
4862017-02-02T20:36:41 <jtimon> hmm, I'm also not sure https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format enforces the "always braces except if everything in one line" rule
4872017-02-02T20:37:06 <sipa> i'm not sure that it can
4882017-02-02T20:37:42 <isle2983> the clang-format script in #9658 generates a 'score' metric
4892017-02-02T20:37:44 <gribble> https://github.com/bitcoin/bitcoin/issues/9658 | Add clang_format.py to help automate code style analysis by isle2983 · Pull Request #9658 · bitcoin/bitcoin · GitHub
4902017-02-02T20:38:03 <isle2983> so at the very least, we can watch it converge rather than diverge over time
4912017-02-02T20:39:39 <jtimon> yeah, it seems some (all?) of your tools would fit better in https://github.com/bitcoin-core/bitcoin-maintainer-tools from what other people said
4922017-02-02T20:40:10 <isle2983> yep
4932017-02-02T20:40:24 <isle2983> I am happy with that as a starting place
4942017-02-02T20:41:11 <jtimon> IMO, style rules aren't so useful until you start to enforce it with the help of automatic tools in the whole project, that's the only way you can really stop talking about style, which is the goal of having style rules in the first place, right?
4952017-02-02T20:41:20 <gmaxwell> sipa: clang-format (latest versions) can enforce that rule.
4962017-02-02T20:41:46 <gmaxwell> jtimon: I don't agree. Code style existed long before tools to do anything special with it.
4972017-02-02T20:42:00 <jtimon> gmaxwell: thanks for confirming, that's what I thought, including the exception for the single line ifs
4982017-02-02T20:42:21 <gmaxwell> I think automatic tools often handicap style discussions, because they enforce it stupidly to the point where they can irritate people and cause general opposition to standards.
4992017-02-02T20:42:29 *** handlex has joined #bitcoin-core-dev
5002017-02-02T20:42:38 <gmaxwell> jtimon: yes, including the exception, according to the docs. I have not tested it.
5012017-02-02T20:42:49 <jtimon> well, code style without automatic rules may save you some style discussions, but not all
5022017-02-02T20:42:56 <gmaxwell> (I don't have a current version of clang on my laptop -- another challenge with formating tools, they change.)
5032017-02-02T20:43:04 *** handlex has quit IRC
5042017-02-02T20:43:11 <jtimon> yep
5052017-02-02T20:43:18 <gmaxwell> jtimon: they give people a way to answer the question without first asking everyone else all the time. :)
5062017-02-02T20:44:00 <jtimon> absolutely
5072017-02-02T20:44:46 <gmaxwell> (I'm not opposed to tools, but they work MUcH better for single developers and single companies with rigidly enforced development workstation configs.. than they do for big projects)
5082017-02-02T20:46:27 <jtimon> true that, my very positive experience where with enforced development confgs per project, the project that had discussions about syle were those not enforcing the style automatically
5092017-02-02T20:46:58 <jtimon> s/experience where/experiences were
5102017-02-02T20:47:31 <isle2983> the trend does seem to be towards containerized environments for builds there the dependencies can be managed. That might be wrong for bitcoin, however.
5112017-02-02T20:47:35 <luke-jr> git-clang-format looks interesting
5122017-02-02T20:47:49 <isle2983> *where
5132017-02-02T20:49:03 <jtimon> luke-jr: https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format ? seems the same concept as MarcoFalke's script we don't use, no?
5142017-02-02T20:49:37 <luke-jr> I haven't followed all the discussion, but an upstreamed tool seems better than one we maintain
5152017-02-02T20:50:07 <jtimon> ie this one https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/master/clang-format.py but yeah, this seems to be more interesting as it integrates with git
5162017-02-02T20:51:51 *** Sosumi has quit IRC
5172017-02-02T20:52:09 <luke-jr> step 1: provide a config file so we can just use git-clang-format manually for correct output ;)
5182017-02-02T20:53:12 <cfields> luke-jr: even better, i assume it can be a commit hook?
5192017-02-02T20:53:51 <luke-jr> cfields: I would think so, but I haven't tried it
5202017-02-02T20:53:52 <cfields> (that's a bit dangerous though i suppose, if the script breaks and you lose your commit)
5212017-02-02T20:54:23 <luke-jr> repo-wide commit hooks seem like a complex enough thing with git that I'd make that a full step 2 though ;)
5222017-02-02T20:54:42 <cfields> heh, yes
5232017-02-02T21:17:28 *** BashCo has quit IRC
5242017-02-02T21:21:38 <MarcoFalke> luke-jr: It is a 1-1 copy of llvm
5252017-02-02T21:30:06 <luke-jr> >_<
5262017-02-02T21:30:12 <MarcoFalke> Eh, whatever. Misread the scorllback
5272017-02-02T21:30:38 <MarcoFalke> We could add the git-clang-format as well, so it is in our tree
5282017-02-02T21:35:33 *** BashCo has joined #bitcoin-core-dev
5292017-02-02T21:57:22 *** lclc has quit IRC
5302017-02-02T21:59:04 *** laurentmt has joined #bitcoin-core-dev
5312017-02-02T21:59:38 *** laurentmt has quit IRC
5322017-02-02T22:00:39 <luke-jr> I don't see how this is possible https://github.com/bitcoin/bitcoin/pull/9619#issuecomment-277096336
5332017-02-02T22:19:37 <MarcoFalke> Is there a way to bisect only on (signed) merge commits?
5342017-02-02T22:22:29 <MarcoFalke> Otherwise merging this will probably upset a few: https://github.com/bitcoin/bitcoin/pull/9657#issuecomment-277094673
5352017-02-02T22:23:57 <luke-jr> it wouldn't be the first time we have an untestable tree merged in history
5362017-02-02T22:24:09 <luke-jr> it's a potential security issue to keep in mind though
5372017-02-02T22:24:17 <luke-jr> (bisecting unsigned commits)
5382017-02-02T22:25:09 <MarcoFalke> Well, I hope when people review, they check that the intermediate commits don't add random binary blobs
5392017-02-02T22:26:45 <gmaxwell> MarcoFalke: code that totally backdoors your machine could be a couple lines of shellscript. And the concern there is just that you pull and some is slipped in and you run in bisect... it's worried me before, but didn't seem like there was anything to do over it.
5402017-02-02T22:27:18 <MarcoFalke> Still should be part of review to ensure this does not happen
5412017-02-02T22:27:36 <gmaxwell> it's not something review can control.
5422017-02-02T22:27:40 <kanzure> hi am i late
5432017-02-02T22:27:54 <gmaxwell> I do wish you could check in a .gitbisectuntestable which has a list of commits bisect should just skip over.
5442017-02-02T22:28:20 <MarcoFalke> Please explain why review can not control it
5452017-02-02T22:28:52 <sipa> gmaxwell, MarcoFalke: write a script that produces a list of all unsigned commits, and feed those to 'git bisect skip
5462017-02-02T22:28:55 <sipa> ?
5472017-02-02T22:30:23 <jtimon> BlueMatt: at this point, should I just close #9634 as included in #9650 ?
5482017-02-02T22:30:25 <gribble> https://github.com/bitcoin/bitcoin/issues/9634 | Fail in DecodeHexTx if there is extra data at the end by jtimon · Pull Request #9634 · bitcoin/bitcoin · GitHub
5492017-02-02T22:30:26 <gribble> https://github.com/bitcoin/bitcoin/issues/9650 | Better handle invalid parameters to signrawtransaction by TheBlueMatt · Pull Request #9650 · bitcoin/bitcoin · GitHub
5502017-02-02T22:30:42 <BlueMatt> jtimon: I suppose so
5512017-02-02T22:30:49 <gmaxwell> MarcoFalke: Because compromised code can hide things from reviewers. And there is no guarentee that things that hit the tree have been reviewed (particularly if a commiter is compromised). So if you get compromised you might merge bad commits. You can't see they're bad because the compromise hides them. Other people don't see they're bad because they only review the ultimate commit if at all.
5522017-02-02T22:30:55 <gmaxwell> .. and becuase once they run something in a bad commit they become compromised and can't see it either.
5532017-02-02T22:31:03 <jtimon> BlueMatt: ok, thanks for the tests!
5542017-02-02T22:32:11 <luke-jr> all the more reason to sandbox dev
5552017-02-02T22:32:14 <gmaxwell> especially since we don't require that PR that we merge be based on the current tip (it would be unreasnable to do so), the intermediate commits will often not match exactly what anyone has reviewed.
5562017-02-02T22:32:16 <bitcoin-git> [bitcoin] jtimon closed pull request #9634: Fail in DecodeHexTx if there is extra data at the end (master...upstream-fail-decode-tx) https://github.com/bitcoin/bitcoin/pull/9634
5572017-02-02T22:32:24 <gmaxwell> Even where the final result does.
5582017-02-02T22:32:47 <BlueMatt> jtimon: thanks for catching that I forgot to upstream this!
5592017-02-02T22:32:56 <jtimon> np
5602017-02-02T22:33:08 <luke-jr> it'd probably be more practical to review if we used merge commits instead of rebasing
5612017-02-02T22:33:24 <MarcoFalke> Which is why I proposed we don't corrupt merge commits
5622017-02-02T22:33:27 <MarcoFalke> See #8089
5632017-02-02T22:33:28 <gribble> https://github.com/bitcoin/bitcoin/issues/8089 | verify-commits should also check for malicious code in merge commits · Issue #8089 · bitcoin/bitcoin · GitHub
5642017-02-02T22:33:57 *** CubicEarth has quit IRC
5652017-02-02T22:34:36 <MarcoFalke> > Other people don't see they're bad because they only review the ultimate commit if at all.
5662017-02-02T22:34:44 <MarcoFalke> Review should always be done commit-by-commit
5672017-02-02T22:34:53 <gmaxwell> MarcoFalke: conflicts though is not the same as no difference at all.
5682017-02-02T22:35:29 <gmaxwell> Bascially if you require there be no difference then we must require every PR rebase and re-review every time another PR is merged that touches the same files (or at least functions).
5692017-02-02T22:35:48 <gmaxwell> Otherwise you can arrange the automatic resolution to add vulnerabilities.
5702017-02-02T22:36:33 <gmaxwell> and already people will review commits, but not review that what is ultimately merged were the same commits exactly (usually aren't, due to other changes)
5712017-02-02T22:38:25 <MarcoFalke> I don't understand how you end up with two different results when you merge the exact same commits.
5722017-02-02T22:38:53 <MarcoFalke> I mean you get a different commit hash when you don't adjust the time to time-of-merge and author to merge-commit-author etc
5732017-02-02T22:39:19 <MarcoFalke> but it should be possible to replay all merge commits and compare them to what peoples eyes reviewed
5742017-02-02T22:40:00 <jtimon> BlueMatt: you missed one garbate in 9650 :p
5752017-02-02T22:40:13 <BlueMatt> oh ffs, leave my spelling alone!
5762017-02-02T22:40:14 <BlueMatt> /s
5772017-02-02T22:41:40 <gmaxwell> MarcoFalke: If I review PR X which is on head Y (or which I merged to Q) that is not the same as reviewing PR X merged against Z. (particularly in adversarial condictions)
5782017-02-02T22:42:30 <gmaxwell> I think this is getting too abstract, for this discussion I think it suffices to point out that people will pull code into their local branches before reviewing it, because pulling it into their branch is how they go about reviewing it.
5792017-02-02T22:42:41 <MarcoFalke> Reviewers don't commit to "PR X merged against Z", they commit to "PR X".
5802017-02-02T22:43:17 <MarcoFalke> They can only commit to the merge commit when the merge actually happened in the master branch
5812017-02-02T22:45:07 <MarcoFalke> I mean I can merge a commit_A locally for testing, but when I publish my review I don't refer to my local merge commit by to commit_A
5822017-02-02T22:45:21 <gmaxwell> You're saying the same thing as me--- I think. Reviewers review PR X and so cannot be counted on strongly to catch malicious behavior in X which is only exposed by its merge in Z.
5832017-02-02T22:45:23 <MarcoFalke> /by/but/
5842017-02-02T22:45:52 <gmaxwell> MarcoFalke: right, and so your review /may/ be ineffective, particularly if X was maliciously constructed.
5852017-02-02T22:47:10 <bitcoin-git> [bitcoin] luke-jr opened pull request #9672: Opt-into-RBF for RPC & bitcoin-tx (master...rpc_rbf) https://github.com/bitcoin/bitcoin/pull/9672
5862017-02-02T22:47:11 *** jannes has quit IRC
5872017-02-02T22:47:32 * luke-jr kicks insta-conflict
5882017-02-02T22:48:47 <MarcoFalke> Oh I think we were talking about two completely unrelated things, I think. You were raising concerns about possible exploits that only arise after gits merge algorithm processed them. I was talking about maintainers appending commit/ ammending commits/ editing merge commits or reviewers skipping individual commits of pull request which happen to contain a +++ exploit.sh and ---exploit.sh
5892017-02-02T22:50:34 <gmaxwell> Right. and I am saying that I do not believe people review intermeadate commits in merges, because doing so cannot show issues that arose through the merge. They look at PRs and at end results. If you slip in extra commits that do not change the end results, I believe it is unlikely to get noticed.
5902017-02-02T22:50:39 <gmaxwell> Perhaps I'm wrong.
5912017-02-02T22:53:24 <MarcoFalke> I don't think you are wrong, so we should work towards making it easy such that a uncompromised machine with a helper script and some verified keys can validate the current state of the bitcoin-git repo
5922017-02-02T22:54:00 <luke-jr> ideally, but I think we need to prioritise getting reviews done over making reviews more effort
5932017-02-02T22:54:55 <MarcoFalke> Yeah, that is the downside.
5942017-02-02T23:31:35 *** dgenr8 has quit IRC
5952017-02-02T23:31:38 <bitcoin-git> [bitcoin] ryanofsky opened pull request #9673: Set correct metadata on bumpfee wallet transactions (master...pr/bumpfee-meta) https://github.com/bitcoin/bitcoin/pull/9673
5962017-02-02T23:47:09 *** CubicEarth has joined #bitcoin-core-dev
5972017-02-02T23:48:09 *** dgenr8 has joined #bitcoin-core-dev
5982017-02-02T23:51:49 *** CubicEarth has quit IRC