First meeting for the Bitcoin Core PR Review Club, hosted by John Newbery

bitcoin-core-pr-reviews-2019-05-01.txt

13:01 <@jnewbery> Hi folks!
13:01 < harding> Hi!
13:01 < dmkathayat> Hello!
13:01 < schmidty> hola
13:01 < amiti> hi!
13:01 < bilthon> hi there
13:01 <@jnewbery> We usually start Bitcoin Core IRC meetings with a 'hi' so it's clear who's at keyboard. Feel free to say hi here!
13:01 < kcalvinalvin> Hi
13:01 < peevsie> hi!
13:02 < emzy> Hi
13:02 < ajonas> Hi!
13:02 < RubenSomsen> hi :)
13:02 < mryandao> hi
13:02 < merehap> Hey!
13:02 < MrPaz> Hi!
13:02 < aj> *yawn*
13:02 < TomA> Hi
13:02 < bitcoinerrrr> hey ya'll
13:02 < b10c> Hi!
13:02 < sebastianvstaa> hi
13:02 <@jnewbery> I expect we'll take a couple of weeks to figure out the format. We can iterate as we figure out what works best
13:02 -!- bitcoinerrrr is now known as juscamarena
13:02 < moneyball> hi
13:02 <@jnewbery> aj! Glad you could make it!
13:02 -!- juscamarena is now known as juscamarenaaa
13:02 < fanquake> heh aj
13:03 <@jnewbery> and fanquake!
13:03 < aj> fanquake: woah
13:03 < udiWertheimer> Hi
13:03 < rafeeki> hi all
13:03 <@jnewbery> welcome to our antipodean insomniacs
13:03 < manjaroi3> hi
13:04 < ariard> hi!
13:04 <@jnewbery> I'll start by giving a very brief summary of the PR and then throw open to questions
13:04 -!- manjaroi3 is now known as justinmoon
13:05 <@jnewbery> bumpfee is a command in the Bitcoin Core wallet to create an RBF transaction. It takes an unconfirmed transactions and then 'bumps' the fee on it by reducing the value of the change output
13:05 <@jnewbery> previously that command would fail if there wasn't a change output, or the change output was too small
13:05 <@jnewbery> this PR allows the fee to be bumped on transactions without change or with small change by adding a new input to the bumped transaction
13:06 <@jnewbery> (I choose it a couple of weeks ago before it was merged, but I think it's still useful to review even though it's now merged)
13:06 <@jnewbery> I think that's enough summary from me. Did anyone have any questions about the PR?
13:06 <@jnewbery> don't be shy :)
13:06 < moneyball> thanks for that description. it more clearly explains it than what is in the PR summary.
13:07 < bilthon> oh this is a very nice feature, I had a tx with no change stuck in the mempool for days once because I could not bump its fee
13:07 < ecurrencyhodler> hi
13:07 <@jnewbery> yeah, instagibbs description is accurate, but assumes some knowledge of what bumpfee is already doing
13:08 < udiWertheimer> The description mentions negative value inputs. What are those?
13:08 < bilthon> yeah, this brief summary definitely cleared things up for me
13:08 < emzy> Good summary!
13:09 < b10c> What is knapsack coin selection? quick tl;dr?
13:09 <@jnewbery> udiWertheimer: good question! When selecting coins to include in a transaction, each coin has a value, but there's also the cost associated with spending that coin, because it adds to the weight of the transaction
13:09 <@jnewbery> I think that instagibbs means adding coins where the cost of spending the coin is greater than the value of the coin
13:10 < harding> b10c: are you familar with: https://en.wikipedia.org/wiki/Knapsack_problem ?
13:10 < moneyball> b10c: section 3.4.4 http://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf
13:10 < udiWertheimer> jnewbery: aaah gotcha. thanks!
13:10 <@jnewbery> knapsack coin selection is the old method for doing coin selection in the wallet (pre branch-and-bound)
13:10 < ariard> Not on this PR specifically, but would you set out what process are you following when you review any PR on core, are you reading functions comment first? checking code flow change? etc
13:11 < afig_> SHP0119410
13:11 < merehap> Is there a privacy trade-off in adding the initially unselected output to bump the fee rate (i.e. mixing different outputs)? If there is, would that kind of topic come up during the PR review?
13:11 <@jnewbery> ariard: I'll read the PR description, then do a quick skim through the PR branch commit-wise to get an idea of the overall shape of the changes, and then go through each commit in more detail
13:12 < b10c> thanks harding, moneyball
13:12 < harding> merehap: I don't understand the first part of your question, but privacy concerns should certainly come up on PR reviews.
13:12 <@jnewbery> merehap: definitely privacy concerns are in scope for PR reviews!
13:13 <@jnewbery> I think the question is whether adding a new coin allows a spy to correlate togther the inputs
13:14 < merehap> When bumping the fee rate and including a new output, you are linking previously unlinked outputs (by my understanding). Probably not important to go into depth here though.
13:14 < ariard> oky thanks
13:14 < merehap> Yeah
13:14 < rafeeki> is it typical for a PR like this to come with a pre-written test like wallet_bumpfee.py?
13:14 < udiWertheimer> merehap: unfortunately there’s usually some privacy-related trade off with bumping fees. Even if you don’t add an input, you will expose which of the outputs is your change output (the one that changes), which otherwise would be harder to figure out
13:14 < bilthon> what's up with the "concept ACK" replies down there
13:15 < mryandao> isnt that a similar risk to spending when you consolidate utxos without fee-bumping?
13:15 <@jnewbery> It's a good consideration. Whenever you spend multiple inputs in the same transaction, you're revealing some information about the ownership of those coins (module coinjoin, P2EP, etc). That's true for normal transactions and bumped transactions
13:15 < merehap> Yeah, just curious how those trade-offs are exposed to the user.
13:15 < harding> merehap: ah.  That's an interesting question.  I think it didn't come up in the review because linknig together inputs in inherent in sending transactions at all, whether the user is sending a new transaction or bumping the fee of an existing transaction.
13:16 < merehap> Cool
13:16 <@jnewbery> rafeeki: our test coverage in the wallet with functional tests is pretty good. I'd expect all big new features to include tests
13:17 < karimofthecrop> @jnewbery - can you repost your summary for those of us that weren't quite on time?
13:17 <@jnewbery> bumpfee was added a couple of years ago and had pretty good test coverage, so it was pretty straightforward for instagibbs to add new tests
13:17 < fanquake> bilthon: Checkout https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review. It's basically a way of saying you agree with the changes, but haven't done much, if any review yet.
13:17 <@jnewbery> thanks fanquake
13:17 < mryandao> the only bit i'm curious about is where the dust utxo gets destroyed and contributes to fee instead.
13:17 < MrPaz> so if I understand correctly, before you couldn't increase fee because there weren't enough sats in input and there was simply no way to add a new input.  now there is a way to add new input to bump fee, and it will look for uneconomic inputs to use in bumping fee?
13:18 < mryandao> and dust threshold may increase/decrease in the future?
13:18 < ecurrencyhodler> karimofthecrop: bumpfee is a command in the Bitcoin Core wallet to create an RBF transaction. It takes an unconfirmed transactions and then 'bumps' the fee on it by reducing the value of the change output  previously that command would fail if there wasn't a change output, or the change output was too small  this PR allows the fee to be bumped on transactions without change or with small change by adding a n
13:18 < ecurrencyhodler> Copy pasted from jnewbery
13:18 < karimofthecrop> th
13:18 < karimofthecrop> thx
13:19 <@jnewbery> ...by adding a new input to the bumped transaction
13:20 <@jnewbery> mryandao: that bit is here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/feebumper.cpp#L184
13:20 <@jnewbery> yes, I suppose the dust threshold could change in future. It's a policy rule, not consensus
13:21 < mryandao> so yes, the idea here is my maybe-future-spendable dust gets chucked into fee and I overpaid.
13:21 <@jnewbery> MrPaz: it will look for any inputs to add to the transactions. It just happens that it might pick uneconomical inputs as part of that knapsack search
13:21 < mryandao> but i guess, the dust is low enough to be immaterial
13:22 < harding> mryandao: I don't think that's something this PR changes, as it's Bitcoin Core's existing wallet policy to not send outputs that may cost more to spend than they're worth.
13:22 <@jnewbery> mryandao: the idea is to not reduce that change output to below dust so that the replacement transaction is able to be relayed
13:22 < harding> (Or even somewhat above that limit, as eliminating change outputs brings a privacy benefit.)
13:23 < mryandao> mm ok - relay policy gotcha.
13:23 < dmkathayat> jnewbery: What does CoinControl's m_min_depth do?
13:24 < rafeeki> jnewbery: thanks! I liked the idea of starting to contribute just by running/writing test cases. It surprised me that this one was so formulaic. I guess in future weeks we will see other examples that may need more testing?
13:24 <@jnewbery> take a look at the definition of CoinControl here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coincontrol.h#L15
13:24 < harding> rafeeki: I think this PR probably could've used more tests, and contributions to testing are welcome independently from the PRs that implement the features.
13:25 <@jnewbery> the idea of CoinControl is that it can be used to control how a transaction is constructed by the wallet
13:25 <@jnewbery> I believe it's grown over the years, but I guess it was originally just for selecting which coins to include as inputs in the transaction
13:26 < kcalvinalvin> I don't think I'm quite understanding the policy rules. if (dust output > 0); return false; is this correct?
13:26 <@jnewbery> m_min_depth just says "don't select coins that have fewer than this many confirmations as inputs"
13:26 < aj> harding: while you're reviewing adding tests yourself help you understand the behaviour, and you can send them to the author who can add them to the PR too. super helpful in my opinion
13:27 < harding> aj: agreed!
13:27 <@jnewbery> refeeki/harding: manual testing of new features is always welcome. And like aj says, contributing automated tests to the PR author is a really helpful way to start contributing
13:28 < mryandao> we can only bumpfee once?
13:28 <@jnewbery> eg one of my early contributions here: https://github.com/bitcoin/bitcoin/pull/9484#issuecomment-272547796 was adding tests for a new feature which didn't have test coverage
13:28 < harding> In case it's helpful to anyone, I took a quick look at the PR earlier and made notes about what I'd test for it: https://gist.github.com/harding/168f82e7986a1befb0e785957fb600dd
13:29 <@jnewbery> I really appreciate it when someone reviews my PRs and provides additional tests
13:29 < mryandao> src/wallet/feebumper.cpp#299
13:29 < merehap> harding: Yeah, very helpful.
13:29 <@jnewbery> harding: awesome. Thanks!
13:30 <@jnewbery> A comment like that in the PR is really helpful in review: "here's what I tested an my methodology"
13:30 <@jnewbery> mryandoa: no, it's possible to bump multiple times
13:30 <@jnewbery> BIP 125 describes bitcoin core's mempool policy for allowing txs to be replaced : https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki
13:31 <@jnewbery> each subsequent bump must fulfil those conditions, eg each must have a feerate higher than the previous
13:32 < dmkathayat> Not specific to this PR, but do you review PRs directly on github or on your terminal? I'm finding it tricky to just read through the line changes on github.
13:32 < mryandao> jnewbery: thanks for that.
13:32 <@jnewbery> I never review on github, partly because the diff view is not very helpful, and partly to avoid trusting a third party
13:32 < rafeeki> harding/jnewbery/aj: thanks so much! great feedback and examples
13:32 < moneyball> bilthon: concept ACK just means that the reviewer acknowledges and agrees with the concept of the change, but is not (yet) confirming they've looked at the code or tested it. this can be a valuable signal to a PR author to let them know that the PR has merit and is headed in the right direction.
13:33 <@jnewbery> I check out the branch locally and then run a difftool on each commit in turn, and then ACK the commit hash of the HEAD commit
13:34 <@jnewbery> ymmv, but the current difftools I use are meld on linux and opendiff on mac
13:34 < aj> i find skimming on github helpful, but yeah, always actually review locally... i find "gitk" useful for getting an overview of changes
13:35 < mryandao> my tagger is broken, even browsing the python test scripts, the tags point back to the cpp source
13:35 < karimofthecrop> jnewbery: what do you mean "ACK the commit hash of the HEAD commit" - where do you ACK? On github?
13:35 < ariard> jnewbery: which are the good bitcoin dev tooling repos? Seems like everyone I've its own? Are they listed somewhere to dig in?
13:35 < dmkathayat> Great, thanks!
13:35 < ariard> s/I've/have/
13:35 <@jnewbery> karimofthecrop: here for example: https://github.com/bitcoin/bitcoin/pull/15557#issuecomment-482139144
13:36 < harding> ariard: https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md has some tips on tools and tricks
13:36 <@jnewbery> this means: I've reviewed the branch that ends in commit 184f878, and I think it's ready for merge
13:36 < karimofthecrop> I see, I hadn't noticed hashes in previous ACKs. Is this considerd best practice?
13:36 <@jnewbery> that commit hash is from my local checkout of the branch, so unless my local tools are compromised, I know I'm ACKing the exact changes
13:36 < instagibbs> It's useful when a force push happens and links to old commits are lost on github
13:36 <@jnewbery> If I took that commit hash from github, they could be lying to me
13:37 < instagibbs> (that too)
13:37 < ariard> harding: thanks this doc is great, was more thinking to pre-push git script to be sure I don't forget any tests before to PR
13:37 < fanquake> ariard I have some tools, info & random stuff in https://github.com/fanquake/core-review. Has guides on how to gitian build, review certain types of PRs etc.
13:37 < instagibbs> although unless you gpg sign the ACK, they could just modify what you're saying :)
13:38 < fanquake> I still need to improve the docs, and push up some more stuff I have sitting locally.
13:38 < ariard> fanquake: thanks bookmarked, maybe could be linked at the end of productivity.md?
13:38 <@jnewbery> instagibbs: indeed. If you want to fully remove trust, you can go the full MarcoFalke and sign/opentimestamp all of your review comments :)
13:38 < b10c> fanquake: thanks!
13:38 < karimofthecrop> does anyone devel in a docker container instead of a vagrant box?
13:39 < fanquake> ariard: I don't think that'd be the right place for personal type repositories.
13:39 < karimofthecrop> I have seen some docker containers, wondering if there is a preferred/blessed one.
13:39 < fanquake> There is also https://github.com/bitcoin-core/bitcoin-maintainer-tools, if your looking for something more official.
13:40 < ariard> okay got it, it just to be aware of good practices to avoid having to rewrite same scripts it has already been done
13:40 < ariard> but yes we have all different setups
13:40 < instagibbs> a bit like herding cats, everyone has their own setup
13:41 < ariard> and workflow
13:41 <@jnewbery> yeah, I think there probably is no royal road to your dev setup. You build it up over the years
13:41 < karimofthecrop> instagibbs: has there ever been problems due to inconsistent environments?
13:41 < amiti> would love to hear about some of your personal setups
13:42 < instagibbs> karimofthecrop, rarely? I mean, I say this as someone running a very common OS in developer circles
13:42 < fanquake> karimofthecrop: infrequently bugs have made it into the codebase due to lack of testing on certain OSes.
13:42 < karimofthecrop> is there a need for continuous integration to avoid that?
13:42 < emzy> Some OS that is missing testing?
13:42 <@jnewbery> amit: vagrant, vim+various Tim Pope plugins, git :)
13:43 <@jnewbery> *amiti
13:43 < mryandao> all tests passed on my end.
13:44 < ariard> jnewbery's great piece on tooling https://bitcointechtalk.com/contributing-to-bitcoin-core-a-personal-account-35f3a594340b !
13:44 <@jnewbery> haha thanks ariard. That's more about generally contributing than specific tools
13:45 < fanquake> emzy: One example was https://github.com/bitcoin/bitcoin/pull/9598, which broke compilation on FreeBSD. Later fixed in https://github.com/bitcoin/bitcoin/pull/13503.
13:45 < amiti> hm yeah, have read that piece, but am more interested in build environments
13:45 < ariard> well at least read the sharpening the tool part sometimes ago, was helpful thanks
13:46 < fanquake> amiti: I test on macOS, then use a mixture of Docker & vagrant to test on other OS's. Generally build related stuff.
13:46 <@jnewbery> we now have Travis and appveyor CIs for testing in linux/windows, although appveyor is still a bit flakey I think
13:46 < ariard> yes build is quicly a bottleneck when you want to parallelize your workflow
13:46 < afig_> wish i could be a little more involved in the chat but i am currently at work. This is helpful!
13:47 < fanquake> We've got at least one BSD CI now, but it's not integrated into the repo.
13:47 < karimofthecrop> fanquake: it would seem that a) this isn't a big problem and b) not many nodes on freebsd :P
13:47 <@jnewbery> afig_: yeah, it's impossible to have a time that's convenient for everyone. Thanks for logging in anyway
13:47 < karimofthecrop> what afig_ said. very useful!
13:47 < kcalvinalvin> What distros are must checks for a PR
13:48 < fanquake> kcalvinalvin: rough list for build related changes https://github.com/fanquake/core-review/blob/master/operating-systems.md
13:49 < instagibbs> kcalvinalvin, in general most changes won't require careful checks of that kind. Unless it involves low-level networking, GUI, ???
13:50 <@jnewbery> kcalvinalvin: it's a good question. In this case, the changes are all at a higher application-level, so very unlikely to have portability issues
13:50 <@jnewbery> ok, we'll wrap up in 10 minutes. If you have any questions that you've been holding back on, now's the time!
13:50 < fanquake> Also depends on what the expectations are in regards to building with provided libraries or depends. i.e qt is somewhat broken on debian atm: https://github.com/bitcoin/bitcoin/issues/15453
13:51 < karimofthecrop> jnewbery: how would one go about picking a PR that could use some testing love, as you and harding suggested earlier?
13:51 < karimofthecrop> there are 294 currently open!
13:52 <@jnewbery> I'm not sure if there's a quick way to identify which PRs could use additional testing without looking at them. In general, I'd recommend that you pick one subsystem that you're interested in (eg wallet, testing framework, rpc, net), and try to survey all the open PRs in that area, then identify which ones you think need additional testing/review
13:53 < fanquake> jnewbery: maybe worth mentioning https://github.com/bitcoin/bitcoin/projects/8 ?
13:54 < mryandao> test/functional/wallet_bumpfee.py#313 -- are we unlocking the wallet again after the rpc error was thrown when trying to bump fee?
13:54 < emzy> Is this helping?  https://bitcoinacks.com/
13:54 < harding> karimofthecrop: there's also https://gist.github.com/jnewbery/6e2797a6f484de59aefc849a6b184008 , which has the PRs planned to be discussed here in the next couple weeks.  I don't know if any of them need more tests written, but it'd be worth checking.
13:54 < karimofthecrop> is it fair to suspect that "needs rebase" tag inidcates a) has enough love and/or b) is kinds stuck?
13:54 < karimofthecrop> ^kinds^kinda^
13:54 <@jnewbery> or feel free to ask in #bitcoin-core-dev. An "I'm interested in helping test wallet/net/etc PRs and I'm looking for PRs to help on" would definitely be well received
13:55 <@jnewbery> fanquake: yes, those are high-priority for review PRs. You should expect to see more action on those than the average PR. Definitely worth knowing what's on there and what other people are looking at
13:56 <@jnewbery> karimofthecrop: if a PR has needed rebase for a while (say more than a week or so), then I interpret it as the author not actively working on it, so I tend to avoid review until it's more active
13:56 < harding> karimofthecrop: for how much love a PR has, you're probably looking for how many ACKs it has from well-know contributors.
13:57 <@jnewbery> but definitely feel free to prod authors. You can leave a comment saying "I
13:57 <@jnewbery> 'd like to review this, but I want to make sure that it's still being actively maintained"
13:57 <@jnewbery> I think that's fine. I wouldn't mind recieving that comment on my PRs
13:57 < jonatack> hi! just learned about this excellent initiative. is there a link to a log of this chat?
13:57 < karimofthecrop> prod via public comment? or dm?
13:58 <@jnewbery> either is fine I think
13:58 < harding> jonatack: AFAIK, we don't have logging setup, but I can put up something in a couple minutes when the chat is over.
13:58 <@jnewbery> other contributors: feel free to chime in if you don't think that's a good idea
13:59 < jonatack> thank you
13:59 <@jnewbery> harding: that'd be awesome (as long as other people think it'd be useful)
13:59 < kcalvinalvin> thanks for this, it was really helpful. Will be back next week
13:59 < fanquake> jnewbery: I agree either is fine. On GH just has the advantage of other interested parties also seeing that someone has reached out.
14:00 <@jnewbery> ok, let's wrap it up here. Upcoming PRs to review are here: https://gist.github.com/jnewbery/6e2797a6f484de59aefc849a6b184008
14:00 <@jnewbery> Thanks everyone!
14:00 < mryandao> the assumeutxo one looks like a big one
14:00 < jamesc> Thanks jnewbery!
14:00 < b10c> jnewbery: thank you!
14:00 < MrPaz> thank you!
14:00 < harding> jnewbery: thank you!
14:00 < merehap> Thanks! This was very helpful. Love the idea and thanks for taking the initiative to run this!
14:00 < sebastianvstaa> thanks jnewberry! that was very helpful. will be back next week.
14:00 < peevsie> This was great, thanks!
14:00 < emzy> Thanks jnewbery
14:00 < ecurrencyhodler> Thank you!!
14:01 < Lightlike> Thank you, very helpful!
14:01 < karimofthecrop> +1 all the thanks :D
14:01 < bilthon> thanks jnewbery, got sidetracked there with some of the links you guys sent, definitely will be back here next week
14:01 < rafeeki> thanks all! Looking forward to next week
14:01 < RubenSomsen> Thanks John :)
14:01 <@jnewbery> Feedback is most definitely welcome. Also let me know if you have suggestions for PRs to cover. The aim is for manageable sized PRs (ie 100-150 LOC change seems about right), not too much contextual knowledge required, and trying to cover all the different components
14:02 <@jnewbery> Feel free to leave feedback in this IRC channel. I'll be monitoring through the week

After the questions about dev env setup, I created this: https://gist.github.com/karimofthecrop/4069e509e184a000a84f0ad7b3423cc6