Enhance bumpfee to include inputs when targeting a feerate (wallet)


Host: jnewbery  -  PR author: instagibbs

Meeting Log

  113:01 <@jnewbery> Hi folks!
  213:01 <harding> Hi!
  313:01 <dmkathayat> Hello!
  413:01 <schmidty> hola
  513:01 <amiti> hi!
  613:01 <bilthon> hi there
  713: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!
  813:01 <kcalvinalvin> Hi
  913:01 <peevsie> hi!
 1013:02 <emzy> Hi
 1113:02 <ajonas> Hi!
 1213:02 <RubenSomsen> hi :)
 1313:02 <mryandao> hi
 1413:02 <merehap> Hey!
 1513:02 <MrPaz> Hi!
 1613:02 <aj> *yawn*
 1713:02 <TomA> Hi
 1813:02 <bitcoinerrrr> hey ya'll
 1913:02 <b10c> Hi!
 2013:02 <sebastianvstaa> hi
 2113: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
 2213:02 -!- bitcoinerrrr is now known as juscamarena
 2313:02 <moneyball> hi
 2413:02 <@jnewbery> aj! Glad you could make it!
 2513:02 -!- juscamarena is now known as juscamarenaaa
 2613:02 <fanquake> heh aj
 2713:03 <@jnewbery> and fanquake!
 2813:03 <aj> fanquake: woah
 2913:03 <udiWertheimer> Hi
 3013:03 <rafeeki> hi all
 3113:03 <@jnewbery> welcome to our antipodean insomniacs
 3213:03 <manjaroi3> hi
 3313:04 <ariard> hi!
 3413:04 <@jnewbery> I'll start by giving a very brief summary of the PR and then throw open to questions
 3513:04 -!- manjaroi3 is now known as justinmoon
 3613: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
 3713:05 <@jnewbery> previously that command would fail if there wasn't a change output, or the change output was too small
 3813: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
 3913: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)
 4013:06 <@jnewbery> I think that's enough summary from me. Did anyone have any questions about the PR?
 4113:06 <@jnewbery> don't be shy :)
 4213:06 <moneyball> thanks for that description. it more clearly explains it than what is in the PR summary.
 4313: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
 4413:07 <ecurrencyhodler> hi
 4513:07 <@jnewbery> yeah, instagibbs description is accurate, but assumes some knowledge of what bumpfee is already doing
 4613:08 <udiWertheimer> The description mentions negative value inputs. What are those?
 4713:08 <bilthon> yeah, this brief summary definitely cleared things up for me
 4813:08 <emzy> Good summary!
 4913:09 <b10c> What is knapsack coin selection? quick tl;dr?
 5013: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
 5113:09 <@jnewbery> I think that instagibbs means adding coins where the cost of spending the coin is greater than the value of the coin
 5213:10 <harding> b10c: are you familar with: https://en.wikipedia.org/wiki/Knapsack_problem ?
 5313:10 <moneyball> b10c: section 3.4.4 http://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf
 5413:10 <udiWertheimer> jnewbery: aaah gotcha. thanks!
 5513:10 <@jnewbery> knapsack coin selection is the old method for doing coin selection in the wallet (pre branch-and-bound)
 5613: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
 5713:11 <afig_> SHP0119410
 5813: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?
 5913: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
 6013:12 <b10c> thanks harding, moneyball
 6113:12 <harding> merehap: I don't understand the first part of your question, but privacy concerns should certainly come up on PR reviews.
 6213:12 <@jnewbery> merehap: definitely privacy concerns are in scope for PR reviews!
 6313:13 <@jnewbery> I think the question is whether adding a new coin allows a spy to correlate togther the inputs
 6413: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.
 6513:14 <ariard> oky thanks
 6613:14 <merehap> Yeah
 6713:14 <rafeeki> is it typical for a PR like this to come with a pre-written test like wallet_bumpfee.py?
 6813: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
 6913:14 <bilthon> what's up with the "concept ACK" replies down there
 7013:15 <mryandao> isnt that a similar risk to spending when you consolidate utxos without fee-bumping?
 7113: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
 7213:15 <merehap> Yeah, just curious how those trade-offs are exposed to the user.
 7313: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.
 7413:16 <merehap> Cool
 7513: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
 7613:17 <karimofthecrop> @jnewbery - can you repost your summary for those of us that weren't quite on time?
 7713: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
 7813: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.
 7913:17 <@jnewbery> thanks fanquake
 8013:17 <mryandao> the only bit i'm curious about is where the dust utxo gets destroyed and contributes to fee instead.
 8113: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?
 8213:18 <mryandao> and dust threshold may increase/decrease in the future?
 8313: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
 8413:18 <ecurrencyhodler> Copy pasted from jnewbery
 8513:18 <karimofthecrop> th
 8613:18 <karimofthecrop> thx
 8713:19 <@jnewbery> ...by adding a new input to the bumped transaction
 8813:20 <@jnewbery> mryandao: that bit is here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/feebumper.cpp#L184
 8913:20 <@jnewbery> yes, I suppose the dust threshold could change in future. It's a policy rule, not consensus
 9013:21 <mryandao> so yes, the idea here is my maybe-future-spendable dust gets chucked into fee and I overpaid.
 9113: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
 9213:21 <mryandao> but i guess, the dust is low enough to be immaterial
 9313: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.
 9413: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
 9513:22 <harding> (Or even somewhat above that limit, as eliminating change outputs brings a privacy benefit.)
 9613:23 <mryandao> mm ok - relay policy gotcha.
 9713:23 <dmkathayat> jnewbery: What does CoinControl's m_min_depth do?
 9813: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?
 9913:24 <@jnewbery> take a look at the definition of CoinControl here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coincontrol.h#L15
10013: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.
10113:25 <@jnewbery> the idea of CoinControl is that it can be used to control how a transaction is constructed by the wallet
10213: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
10313:26 <kcalvinalvin> I don't think I'm quite understanding the policy rules. if (dust output > 0); return false; is this correct?
10413:26 <@jnewbery> m_min_depth just says "don't select coins that have fewer than this many confirmations as inputs"
10513: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
10613:27 <harding> aj: agreed!
10713: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
10813:28 <mryandao> we can only bumpfee once?
10913: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
11013: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
11113:29 <@jnewbery> I really appreciate it when someone reviews my PRs and provides additional tests
11213:29 <mryandao> src/wallet/feebumper.cpp#299
11313:29 <merehap> harding: Yeah, very helpful.
11413:29 <@jnewbery> harding: awesome. Thanks!
11513:30 <@jnewbery> A comment like that in the PR is really helpful in review: "here's what I tested an my methodology"
11613:30 <@jnewbery> mryandoa: no, it's possible to bump multiple times
11713: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
11813:31 <@jnewbery> each subsequent bump must fulfil those conditions, eg each must have a feerate higher than the previous
11913: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.
12013:32 <mryandao> jnewbery: thanks for that.
12113:32 <@jnewbery> I never review on github, partly because the diff view is not very helpful, and partly to avoid trusting a third party
12213:32 <rafeeki> harding/jnewbery/aj: thanks so much! great feedback and examples
12313: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.
12413: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
12513:34 <@jnewbery> ymmv, but the current difftools I use are meld on linux and opendiff on mac
12613:34 <aj> i find skimming on github helpful, but yeah, always actually review locally... i find "gitk" useful for getting an overview of changes
12713:35 <mryandao> my tagger is broken, even browsing the python test scripts, the tags point back to the cpp source
12813:35 <karimofthecrop> jnewbery: what do you mean "ACK the commit hash of the HEAD commit" - where do you ACK? On github?
12913: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?
13013:35 <dmkathayat> Great, thanks!
13113:35 <ariard> s/I've/have/
13213:35 <@jnewbery> karimofthecrop: here for example: https://github.com/bitcoin/bitcoin/pull/15557#issuecomment-482139144
13313:36 <harding> ariard: https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md has some tips on tools and tricks
13413:36 <@jnewbery> this means: I've reviewed the branch that ends in commit 184f878, and I think it's ready for merge
13513:36 <karimofthecrop> I see, I hadn't noticed hashes in previous ACKs. Is this considerd best practice?
13613: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
13713:36 <instagibbs> It's useful when a force push happens and links to old commits are lost on github
13813:36 <@jnewbery> If I took that commit hash from github, they could be lying to me
13913:37 <instagibbs> (that too)
14013: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
14113: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.
14213:37 <instagibbs> although unless you gpg sign the ACK, they could just modify what you're saying :)
14313:38 <fanquake> I still need to improve the docs, and push up some more stuff I have sitting locally.
14413:38 <ariard> fanquake: thanks bookmarked, maybe could be linked at the end of productivity.md?
14513: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 :)
14613:38 <b10c> fanquake: thanks!
14713:38 <karimofthecrop> does anyone devel in a docker container instead of a vagrant box?
14813:39 <fanquake> ariard: I don't think that'd be the right place for personal type repositories.
14913:39 <karimofthecrop> I have seen some docker containers, wondering if there is a preferred/blessed one.
15013:39 <fanquake> There is also https://github.com/bitcoin-core/bitcoin-maintainer-tools, if your looking for something more official.
15113: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
15213:40 <ariard> but yes we have all different setups
15313:40 <instagibbs> a bit like herding cats, everyone has their own setup
15413:41 <ariard> and workflow
15513:41 <@jnewbery> yeah, I think there probably is no royal road to your dev setup. You build it up over the years
15613:41 <karimofthecrop> instagibbs: has there ever been problems due to inconsistent environments?
15713:41 <amiti> would love to hear about some of your personal setups
15813:42 <instagibbs> karimofthecrop, rarely? I mean, I say this as someone running a very common OS in developer circles
15913:42 <fanquake> karimofthecrop: infrequently bugs have made it into the codebase due to lack of testing on certain OSes.
16013:42 <karimofthecrop> is there a need for continuous integration to avoid that?
16113:42 <emzy> Some OS that is missing testing?
16213:42 <@jnewbery> amit: vagrant, vim+various Tim Pope plugins, git :)
16313:43 <@jnewbery> *amiti
16413:43 <mryandao> all tests passed on my end.
16513:44 <ariard> jnewbery's great piece on tooling https://bitcointechtalk.com/contributing-to-bitcoin-core-a-personal-account-35f3a594340b !
16613:44 <@jnewbery> haha thanks ariard. That's more about generally contributing than specific tools
16713: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.
16813:45 <amiti> hm yeah, have read that piece, but am more interested in build environments
16913:45 <ariard> well at least read the sharpening the tool part sometimes ago, was helpful thanks
17013:46 <fanquake> amiti: I test on macOS, then use a mixture of Docker & vagrant to test on other OS's. Generally build related stuff.
17113:46 <@jnewbery> we now have Travis and appveyor CIs for testing in linux/windows, although appveyor is still a bit flakey I think
17213:46 <ariard> yes build is quicly a bottleneck when you want to parallelize your workflow
17313:46 <afig_> wish i could be a little more involved in the chat but i am currently at work. This is helpful!
17413:47 <fanquake> We've got at least one BSD CI now, but it's not integrated into the repo.
17513:47 <karimofthecrop> fanquake: it would seem that a) this isn't a big problem and b) not many nodes on freebsd :P
17613:47 <@jnewbery> afig_: yeah, it's impossible to have a time that's convenient for everyone. Thanks for logging in anyway
17713:47 <karimofthecrop> what afig_ said. very useful!
17813:47 <kcalvinalvin> What distros are must checks for a PR
17913:48 <fanquake> kcalvinalvin: rough list for build related changes https://github.com/fanquake/core-review/blob/master/operating-systems.md
18013:49 <instagibbs> kcalvinalvin, in general most changes won't require careful checks of that kind. Unless it involves low-level networking, GUI, ???
18113: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
18213: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!
18313: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
18413:51 <karimofthecrop> jnewbery: how would one go about picking a PR that could use some testing love, as you and harding suggested earlier?
18513:51 <karimofthecrop> there are 294 currently open!
18613: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
18713:53 <fanquake> jnewbery: maybe worth mentioning https://github.com/bitcoin/bitcoin/projects/8 ?
18813: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?
18913:54 <emzy> Is this helping? https://bitcoinacks.com/
19013: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.
19113:54 <karimofthecrop> is it fair to suspect that "needs rebase" tag inidcates a) has enough love and/or b) is kinds stuck?
19213:54 <karimofthecrop> ^kinds^kinda^
19313: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
19413: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
19513: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
19613:56 <harding> karimofthecrop: for how much love a PR has, you're probably looking for how many ACKs it has from well-know contributors.
19713:57 <@jnewbery> but definitely feel free to prod authors. You can leave a comment saying "I
19813:57 <@jnewbery> 'd like to review this, but I want to make sure that it's still being actively maintained"
19913:57 <@jnewbery> I think that's fine. I wouldn't mind recieving that comment on my PRs
20013:57 <jonatack> hi! just learned about this excellent initiative. is there a link to a log of this chat?
20113:57 <karimofthecrop> prod via public comment? or dm?
20213:58 <@jnewbery> either is fine I think
20313: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.
20413:58 <@jnewbery> other contributors: feel free to chime in if you don't think that's a good idea
20513:59 <jonatack> thank you
20613:59 <@jnewbery> harding: that'd be awesome (as long as other people think it'd be useful)
20713:59 <kcalvinalvin> thanks for this, it was really helpful. Will be back next week
20813: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.
20914:00 <@jnewbery> ok, let's wrap it up here. Upcoming PRs to review are here: https://gist.github.com/jnewbery/6e2797a6f484de59aefc849a6b184008
21014:00 <@jnewbery> Thanks everyone!
21114:00 <mryandao> the assumeutxo one looks like a big one
21214:00 <jamesc> Thanks jnewbery!
21314:00 <b10c> jnewbery: thank you!
21414:00 <MrPaz> thank you!
21514:00 <harding> jnewbery: thank you!
21614:00 <merehap> Thanks! This was very helpful. Love the idea and thanks for taking the initiative to run this!
21714:00 <sebastianvstaa> thanks jnewberry! that was very helpful. will be back next week.
21814:00 <peevsie> This was great, thanks!
21914:00 <emzy> Thanks jnewbery
22014:00 <ecurrencyhodler> Thank you!!
22114:01 <Lightlike> Thank you, very helpful!
22214:01 <karimofthecrop> +1 all the thanks :D
22314:01 <bilthon> thanks jnewbery, got sidetracked there with some of the links you guys sent, definitely will be back here next week
22414:01 <rafeeki> thanks all! Looking forward to next week
22514:01 <RubenSomsen> Thanks John :)
22614: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
22714:02 <@jnewbery> Feel free to leave feedback in this IRC channel. I'll be monitoring through the week