--- Day changed Wed Aug 14 2019 01:15 < fanquake> If anyone is interested in picking up a wallet PR, I've just close #14565 as "Up for grabs". 01:39 -!- setpill [~setpill@unaffiliated/setpill] has joined #bitcoin-core-pr-reviews 02:17 < jonatack> fanquake: Thanks! Can you re-confirm which PR? https://github.com/bitcoin/bitcoin/pull/14565 is merged. 02:18 < fanquake> jonatack: eh, meant #15032 02:19 < fanquake> https://github.com/bitcoin/bitcoin/pull/15032 02:20 < jonatack> fanquake: thanks 02:22 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 02:28 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has quit [Ping timeout: 252 seconds] 03:31 -!- jonatack [~jon@82.102.27.195] has joined #bitcoin-core-pr-reviews 03:50 -!- jonatack [~jon@82.102.27.195] has quit [Ping timeout: 245 seconds] 03:52 -!- jonatack [~jon@213.152.162.104] has joined #bitcoin-core-pr-reviews 04:08 -!- jonatack [~jon@213.152.162.104] has quit [Quit: WeeChat 2.3] 04:08 -!- jonatack [~jon@213.152.162.104] has joined #bitcoin-core-pr-reviews 05:10 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: harrigan] 05:30 -!- designwish [~designwis@51.ip-51-68-136.eu] has quit [Quit: ZNC - http://znc.in] 05:36 -!- designwish [~designwis@51.ip-51-68-136.eu] has joined #bitcoin-core-pr-reviews 05:44 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 06:25 -!- jonatack [~jon@213.152.162.104] has quit [Ping timeout: 245 seconds] 07:05 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has joined #bitcoin-core-pr-reviews 07:13 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: harrigan] 07:41 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-rvlwnnvpylfdhovi] has joined #bitcoin-core-pr-reviews 07:42 -!- hebasto [~hebasto@95.164.65.194] has joined #bitcoin-core-pr-reviews 09:13 < jnewbery> we'll get started in about 45 minutes 09:18 < jonatack> gd 09:19 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews 09:24 -!- michaelfolkson [~textual@bzq-60-168-31-122.red.bezeqint.net] has joined #bitcoin-core-pr-reviews 09:26 < jonatack> on the Bitcoin Core PR Review Club website, today's meeting is already displayed in the "Previous Meetings" list 09:27 < jonatack> ("gd" was inadvertent typing of a "git diff" alias) 09:44 -!- xsb [~xsb@93.176.165.11] has joined #bitcoin-core-pr-reviews 09:53 -!- probably_dillon [4516f113@user-12hds8j.cable.mindspring.com] has joined #bitcoin-core-pr-reviews 09:57 -!- lightlike [~lightlike@2001:16b8:576b:1f00:6c9c:4821:159a:c840] has joined #bitcoin-core-pr-reviews 10:00 < jnewbery> jonatack: thanks. Too late to change that now! 10:00 < jnewbery> hi 10:00 < nehan> hi 10:00 < jkczyz> hi 10:00 < lightlike> hello 10:00 < ariard> hi 10:00 < jnewbery> today's notes/questions are at: https://bitcoin-core-review-club.github.io/16115.html 10:01 < jonatack> hi 10:01 -!- hanhua [68850940@104.133.9.64] has joined #bitcoin-core-pr-reviews 10:01 < jnewbery> jkczyz: I see you already left review comments. Great! 10:01 < jnewbery> What were your overall thoughts on the PR? 10:02 < jnewbery> (open to anyone) 10:02 < digi_james> Hello 10:02 < jkczyz> Undecided if this is preferable to a command to print the args 10:03 < jnewbery> I think there's a PR that does that (RPC to print args) too 10:04 < jonatack> seems somewhat brittle, unfinished, and needs more test coverage 10:04 < jonatack> though good idea 10:04 < fjahr> Yeah, #15493 is the PR for print args 10:04 < jnewbery> jonatack: what part do you think is brittle 10:05 < jonatack> the blacklist part 10:05 < kanzure> hi 10:05 < jnewbery> fjahr: I don't think that's an RPC. That's a command line arg to pass to bitcoind 10:05 < michaelfolkson> I wasn't sure on the Concept ACK 10:06 < jnewbery> maybe 15493 was the one I was thinking of 10:06 < lightlike> certainly like the concept of writing the down this info. not sure if the second commit around rotating logs is needed (liked jkczyz comment about this). 10:06 < jnewbery> jkczyz: can you think of any advantage to logging to debug.log instead of an RPC? 10:06 < jonatack> jtczyz: yes, nice job on the review 10:07 < fjahr> jnewbery: that's the same number? 10:07 < jkczyz> jnewbery: Yeah, in cases where there is a crash under the given configuration 10:07 < jnewbery> fjahr: yes, I think I misremembered what that PR was doing 10:07 < fjahr> ah, got it 10:08 -!- DevJewek [~Wylatend@82.102.24.131] has joined #bitcoin-core-pr-reviews 10:08 < jnewbery> jkczyz: right. Sometimes debug.log is all we've got. A user says "something strange happened" and we don't know what the config was 10:09 < michaelfolkson> But are we expecting them to give the whole debug.log from beginning to end? The config will be right at the start 10:09 < jnewbery> lighlike: yeah, I agree with your point about the second commit. It doesn't need to be bundled into the same PR 10:09 < michaelfolkson> Surely better to provide them separately 10:10 < jnewbery> michaelfolkson: (re: are we expecting them...) perhaps, but better to log somewhere than nowhere 10:10 < jnewbery> first question: in a review comment, Carl Dong suggests that it’d be “very useful to have the effective command line configuration at-a-glance”. How does this differ from what has been implemented in this PR? Why would it be difficult to implement? 10:11 < michaelfolkson> Provide two different logs? You only need the recent output to see what the problem was in the debug log 10:11 < nehan> i assumed he meant the values for all commandline arguments instead of just user-passed in ones 10:12 < jnewbery> nehan: that's part of it 10:12 < nehan> as promag points out that seems difficult because initialization happens all over the place 10:12 < jnewbery> all commandline args have default values. This PR doesn't print what those default values are if nothing is passed in explicitly 10:13 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 10:13 < jnewbery> there are also commandline args that interact with each other, so if one is set then it affects others 10:13 < jnewbery> eg https://github.com/bitcoin/bitcoin/blob/a7aa809027633556dd3280c6e29ca98eb3235a3d/src/init.cpp#L765 10:13 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 10:14 < jnewbery> nehan: yes, it's difficult because gArgs is accessed all over the code, and the actual values are only read/calculated where they're needed 10:14 < jnewbery> Next question: Reviewers asked the PR author to add a blacklist of configuration that shouldn’t be logged. The author implemented that here. What are your thoughts about that implementation of a blacklist? 10:15 < jnewbery> (here: https://github.com/bitcoin/bitcoin/pull/16115/files#diff-c9721a8bab21576f98fa79fda2715408R967) 10:15 < nehan> it would be easy for someone adding a sensitive argument to forget to update that blacklist 10:16 < jnewbery> nehan: I agree! 10:16 -!- pinheadmz [matthewzip@gateway/vpn/nordvpn/pinheadmz] has quit [Quit: pinheadmz] 10:16 < nehan> i didn't look to see if there is a place in the code anyone adding an argument must update. if so, it would be good to add a comment there referencing the blacklist. 10:16 < jnewbery> any suggestions on a different implementation? 10:17 < jnewbery> nehan: good idea 10:17 < jonatack> a whitelist would be safer, though probably more involved 10:17 < jkczyz> Perhaps it could be built into the arg code. A bit saying if it should be blacklisted 10:17 < probably_dillon> a whitelist would suffer from the same "and what if someone forgets it?" problem 10:17 < fjahr> there should be a explicit declaration on each argument 10:17 < jnewbery> jonatack: yes. It suffers from the same problem (people forgetting to update it), but the impact is probably less bad 10:18 < jonatack> yes, not the same probable consequences 10:18 < fjahr> not when calling it, I mean as a type of something like taht in the code 10:18 < jnewbery> jkczyz,fjahr: yeah, perhaps a new 'sensitive data' flag could be added here: https://github.com/bitcoin/bitcoin/blob/a7aa809027633556dd3280c6e29ca98eb3235a3d/src/util/system.cpp#L542 10:19 < jnewbery> here's one existing flag: https://github.com/bitcoin/bitcoin/blob/a7aa809027633556dd3280c6e29ca98eb3235a3d/src/util/system.h#L138 10:20 < jnewbery> next question: What is Bitcoin Core log shrinking? Where is it implemented? 10:20 < nehan> ShrinkDebugFile() in logging.cpp 10:20 < jkczyz> Truncates start of log file if it grows too large (over 10MB * 110% = 11MB) 10:21 < michaelfolkson> Only on startup 10:21 < lightlike> called in init at startup (before logging anything else) 10:21 < jnewbery> nehan, jkczyz, michaelfolkson, lighlike: great answers :) 10:21 < jnewbery> any questions on what that's doing? 10:22 < michaelfolkson> What was the rationale for only shrinking on startup? 10:22 < jnewbery> If not, let's move on to the next question: What is log rotation? How can it be used for Bitcoin Core logs? 10:23 -!- MrRGnome [3242d136@S0106441c121a0669.rd.shawcable.net] has joined #bitcoin-core-pr-reviews 10:23 < michaelfolkson> Addresses problem of not shrinking the log after startup 10:23 < jkczyz> A tool for limiting the size of log files by breaking into multiple files 10:24 < jnewbery> fascinating. ShrinkDebugFile() was added by Satoshi: https://github.com/jnewbery/bitcoin/blob/0bbbee96b742e4ad0fd5e1d3c33e4bd2247e4445/util.cpp#L456 10:24 < jnewbery> jkczyz: right. Did you find where support for log rotation was added to bitcoin core? 10:25 < probably_dillon> is there something wrong with just relying a system's logrotate? 10:25 < jkczyz> jnewbery: I did not. Just assumed people used external tools for it 10:26 < jkczyz> jnewbery: I think I saw a PR where someone wanted to add it, though 10:26 < jnewbery> probably_dillon: that's exactly what happens. bitcoind does not rotate its own log files 10:26 < jnewbery> support for SIGHUP was added here: https://github.com/bitcoin/bitcoin/pull/917 10:26 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 10:26 < jnewbery> the PR author (LarryRuane) gives a good explanation of log rotation here: https://github.com/bitcoin/bitcoin/pull/16115#pullrequestreview-250179580 10:27 < jnewbery> External log rotation tools can be configured to prevent the log file from growing unbounded, see #917. Typically the way these tools work is, they first move (as in, the rename system call) debug.log to a different name.... 10:27 < jnewbery> We've already addressed the next question, but any other thoughts about "What is the purpose of the second commit in this PR (re-log configuration when debug.log is reopened)? What are your thoughts about that additional functionality?" 10:28 < nehan> 1) 200 seems arbitrary 2) I think the logrotator should support saving the startup log instead of doing this 10:29 < jnewbery> nehan: I agree with both of those 10:29 < nehan> now someone has to think about whether or not to put information into this special re-log thing 10:30 < lightlike> I wonder if there are many users how use logrotate and delete the old logs immediately, so that writing the options down again is needed. 10:30 < probably_dillon> nehan: do you mean implementing this as a new "boot config log" log? 10:30 < jnewbery> I also didn't like the fact that the log timestamps are no longer monotonic after this PR 10:30 < jnewbery> That's probably going to confuse some log parsers 10:31 < nehan> probably_di: i don't think saving (or reprinting) initial log information should be handled in bitcoind 10:31 < jonatack> nehan: agreed. brittle. 10:32 < jkczyz> If args are logged at startup, it might be useful providing docs on how to configure the logrotate properly 10:33 < jnewbery> jkczyz: I agree that this is better addressed with documentation than adding complexity to the code. I don't know where exactly that documentation should live 10:34 < jnewbery> There is already precedence for including user tips in the /docs directory, eg: https://github.com/bitcoin/bitcoin/blob/master/doc/reduce-memory.md 10:34 < jnewbery> ok, final question: What tests are included in this PR? Could any additional tests be added? 10:35 < jonatack> The PR includes one functional test that launches a node, passing one blacklisted arg and one whitelisted arg, and verifies they are in the debug log with the blacklisted arg as ****. 10:35 < fjahr> a functional test that is called unit test in the commit message 10:35 < jkczyz> could include a test for blacklisted args with '-regtest.' or other such prefixes 10:35 < jonatack> For one, I'd like to see some edge cases tested as well. 10:36 < michaelfolkson> Such as? 10:36 < jonatack> such as jkczyz's suggestion 10:37 < probably_dillon> testing the whole blacklist seems prudent. 10:37 -!- diogorsergio [~diogorser@212.36.34.126] has joined #bitcoin-core-pr-reviews 10:37 < jonatack> probably 10:37 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: harrigan] 10:37 < jonatack> probably_dillon: yes 10:38 < lightlike> the commit message is wrong ("unit test"), should be functional 10:39 < jnewbery> this also seems like a good candidate for unit tests, since the new functionality is in a single class (ArgsManager) 10:39 < jnewbery> There are already units tests for ArgsManager, eg https://github.com/bitcoin/bitcoin/blob/master/src/test/getarg_tests.cpp 10:39 < jonatack> Unit tests for the code changes would be nice, too, esp the blacklist 10:39 < jnewbery> ok, that's all I had. Did anyone else have any remaining questions/comments they wanted to bring up? 10:40 < lightlike> (sorry, missed comment by fjahr) 10:40 < michaelfolkson> If you were to use a sensitive flag you could run a test to ensure the flag catches all entries 10:40 < michaelfolkson> What are your thoughts on how this PR should progress @jnewbery? Suggest the flag on the PR? 10:41 < jkczyz> michaelfolkson: Was goind to say the same :) 10:41 < michaelfolkson> Or keep it in documentation? 10:41 < jnewbery> michaelfolkson: My main piece of feedback is: split the PR in two. No need to bundle two changes in the same PR 10:41 < michaelfolkson> Or continue improving the PR as designed? 10:41 < jnewbery> I'm a concept ACK on the first commit, but a weak concept NACK on the second 10:42 < nehan> when i ran this, my debug.log had the following: 2019-08-14T15:56:58Z Warning: Unsupported logging category -debug=cmdline1. 10:42 < nehan> i couldn't track down why that was happening. do you know? 10:43 < jnewbery> the `-debug` argument takes one or more logging categories. cmdline1 is not a valid logging category 10:43 < nehan> ok, thanks. it wasn't in the log mentioned in https://github.com/bitcoin/bitcoin/pull/16115#issue-283090468 10:43 < jnewbery> the log came from here: https://github.com/bitcoin/bitcoin/blob/a7aa809027633556dd3280c6e29ca98eb3235a3d/src/init.cpp#L1005 10:44 < jnewbery> I suspect you'd see the same thing on master 10:44 < jnewbery> any other questions? 10:45 < michaelfolkson> I have a question on the config. As a first time user I agonized over what in the config file I could change later and what I needed to get right before installing. Should this be made clearer? 10:45 < jnewbery> what do you mean 'before installing'? 10:45 < michaelfolkson> Some choices can't be reversed eg testnet choice, once pruned can't unprune 10:46 < michaelfolkson> Other choices can be changed later eg which nodes you connect to 10:46 < michaelfolkson> I don't know how many fit in that first category, probably not many 10:46 < jonatack> michaelfolkson: before IBD? 10:46 < michaelfolkson> Sorry yes 10:46 < jonatack> that seems like a great question 10:47 < jnewbery> Yeah, I'm not sure. I think `bitcoind --help` should probably give you the information you need. If you think the help text is lacking, then you should open a PR to improve it. 10:47 < fjahr> you could do both on the same machine using the same bitcoind, with a different datadir 10:47 < jnewbery> There's also this, that might help you: https://jlopp.github.io/bitcoin-core-config-generator/ 10:47 < jnewbery> (repo here: https://github.com/jlopp/bitcoin-core-config-generator) 10:48 < michaelfolkson> Yeah that is helpful. Doesn't address this currently 10:48 < michaelfolkson> That's good suggestion 10:48 < jonatack> maybe an initial node setup doc in the /doc dir 10:48 < wumpus> -prune and -txindex are probably the only options that you want to be sure to get right before ibd because they take quite some time to switch around 10:49 < jnewbery> wumpus: I think -txindex is much better now that it happens in a background thread (it can be switched on and off) 10:49 < wumpus> prune because it throws away blocks, txindex because it's a huge amont of work to build the index 10:49 < michaelfolkson> Cool, thanks. I remember it really slowed me down because I didn't want to make wrong choices that weren't easily reversible 10:50 < jnewbery> I think prune is the important one 10:50 < wumpus> jnewbery: it's definitely become better 10:50 < jnewbery> any final questions, or are people happy to wrap it up there? 10:50 < wumpus> still txindex can take quite a long time too. that it happens in the background doesn't make it faster :) 10:50 < jonatack> michaelfolkson: this state of mind can be precious for adding docs 10:51 < jonatack> initial perspective 10:51 < jnewbery> wumpus: yeah, it's still slow, but at least you don't have to reindex to add it, which I think was the case before 10:51 < michaelfolkson> Yeah should take advantage of it whilst I still have it ;) 10:52 < jnewbery> lots of great questions this week. Thanks everyone (and thanks wumpus for stopping by!) 10:52 < jnewbery> Here's your homework for next week: 10:52 < michaelfolkson> Yeah, was great. Thanks everyone 10:53 < jnewbery> - review https://bitcoin-core-review-club.github.io/15931.html. I'll try to get notes and questions up in the next couple of days 10:53 < jnewbery> - leave comments on this week's PR (https://github.com/bitcoin/bitcoin/pull/16115) 10:53 < jnewbery> - take a look through the open PRs (either https://github.com/bitcoin/bitcoin/pulls or https://bitcoinacks.com/), and see if there are any you think would be interesting for us to discuss 10:54 < jnewbery> I'm still waiting for my first comment in here: https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14 :) 10:54 < jnewbery> that's it. Thanks everyone! 10:54 < fjahr> thanks jnewbery 10:54 < wumpus> jnewbery: true, it used to be that even removing the index took a full reindex, at least removing it should be fast now ! 10:54 < wumpus> yes thanks everyone 10:55 < lightlike> thanks! 10:55 < michaelfolkson> Ah it is an issue . I was looking for that a couple of weeks ago haha 10:55 < jkczyz> yeah, good session! 10:55 < jonatack> jnewbery, wumpus: thanks! ... thanks, everyone! 10:55 < fanquake> That PSBT shuffle inputs PR will be a good one. 10:55 < nehan> thanks! 10:56 < jnewbery> fanquake: PSBT shuffle looks good, but a bit short for our meeting. I thought it might be good to discuss it alongside another PSBT or Privacy PR. Do you have any suggestions? 10:57 < jnewbery> (we can also discuss closed or merged PRs if they're interesting or instructive) 10:57 -!- DevJewek [~Wylatend@82.102.24.131] has quit [Quit: Leaving] 10:57 < fanquake> jnewbery: I’ll have a look. Can add some comments tomorrow. 10:58 < jnewbery> thanks! 11:01 -!- diogorsergio [~diogorser@212.36.34.126] has quit [Remote host closed the connection] 11:02 -!- xsb [~xsb@93.176.165.11] has quit [] 11:03 -!- michaelfolkson [~textual@bzq-60-168-31-122.red.bezeqint.net] has quit [Quit: Sleep mode] 11:04 -!- hanhua [68850940@104.133.9.64] has quit [Remote host closed the connection] 11:19 -!- probably_dillon [4516f113@user-12hds8j.cable.mindspring.com] has quit [Remote host closed the connection] 11:21 < jnewbery> meeting log posted: https://bitcoin-core-review-club.github.io/16115.html 11:30 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-rvlwnnvpylfdhovi] has quit [Quit: Connection closed for inactivity] 12:22 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Write error: Connection reset by peer] 12:43 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 13:03 -!- setpill [~setpill@unaffiliated/setpill] has quit [Quit: o/] 13:20 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving] 15:45 -!- pinheadmz [~matthewzi@207.189.24.149] has joined #bitcoin-core-pr-reviews 16:10 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-leumsttcdrujgspm] has joined #bitcoin-core-pr-reviews 16:50 -!- pinheadmz_ [~matthewzi@209.58.132.107] has joined #bitcoin-core-pr-reviews 16:52 -!- pinheadmz [~matthewzi@207.189.24.149] has quit [Ping timeout: 245 seconds] 16:54 -!- pinheadmz_ [~matthewzi@209.58.132.107] has quit [Read error: Connection reset by peer] 16:55 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 17:39 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has quit [Quit: pinheadmz] 18:14 -!- MrRGnome [3242d136@S0106441c121a0669.rd.shawcable.net] has quit [Remote host closed the connection] 18:25 -!- lightlike [~lightlike@2001:16b8:576b:1f00:6c9c:4821:159a:c840] has left #bitcoin-core-pr-reviews ["Leaving"] 18:28 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-leumsttcdrujgspm] has quit [Quit: Connection closed for inactivity] 20:33 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 21:14 -!- hebasto [~hebasto@95.164.65.194] has quit [Remote host closed the connection] 21:37 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has quit [Quit: pinheadmz] 21:54 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 22:08 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has quit [Ping timeout: 248 seconds] 23:20 -!- fox2p_ [~fox2p@181.215.46.112] has joined #bitcoin-core-pr-reviews 23:20 -!- fox2p [~fox2p@177.67.80.187] has quit [Ping timeout: 248 seconds]