Enhance bumpfee to include inputs when targeting a feerate (
wallet) May 1, 2019
1 13:01 <@jnewbery> Hi folks!
3 13:01 <dmkathayat> Hello!
6 13:01 <bilthon> hi there
7 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!
12 13:02 <RubenSomsen> hi :)
18 13:02 <bitcoinerrrr> hey ya'll
20 13:02 <sebastianvstaa> hi
21 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
22 13:02 -!- bitcoinerrrr is now known as juscamarena
24 13:02 <@jnewbery> aj! Glad you could make it!
25 13:02 -!- juscamarena is now known as juscamarenaaa
26 13:02 <fanquake> heh aj
27 13:03 <@jnewbery> and fanquake!
28 13:03 <aj> fanquake: woah
29 13:03 <udiWertheimer> Hi
31 13:03 <@jnewbery> welcome to our antipodean insomniacs
34 13:04 <@jnewbery> I'll start by giving a very brief summary of the PR and then throw open to questions
35 13:04 -!- manjaroi3 is now known as justinmoon
36 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
37 13:05 <@jnewbery> previously that command would fail if there wasn't a change output, or the change output was too small
38 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
39 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)
40 13:06 <@jnewbery> I think that's enough summary from me. Did anyone have any questions about the PR?
41 13:06 <@jnewbery> don't be shy :)
42 13:06 <moneyball> thanks for that description. it more clearly explains it than what is in the PR summary.
43 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
44 13:07 <ecurrencyhodler> hi
45 13:07 <@jnewbery> yeah, instagibbs description is accurate, but assumes some knowledge of what bumpfee is already doing
46 13:08 <udiWertheimer> The description mentions negative value inputs. What are those?
47 13:08 <bilthon> yeah, this brief summary definitely cleared things up for me
48 13:08 <emzy> Good summary!
49 13:09 <b10c> What is knapsack coin selection? quick tl;dr?
50 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
51 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
54 13:10 <udiWertheimer> jnewbery: aaah gotcha. thanks!
55 13:10 <@jnewbery> knapsack coin selection is the old method for doing coin selection in the wallet (pre branch-and-bound)
56 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
57 13:11 <afig_> SHP0119410
58 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?
59 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
60 13:12 <b10c> thanks harding, moneyball
61 13:12 <harding> merehap: I don't understand the first part of your question, but privacy concerns should certainly come up on PR reviews.
62 13:12 <@jnewbery> merehap: definitely privacy concerns are in scope for PR reviews!
63 13:13 <@jnewbery> I think the question is whether adding a new coin allows a spy to correlate togther the inputs
64 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.
65 13:14 <ariard> oky thanks
67 13:14 <rafeeki> is it typical for a PR like this to come with a pre-written test like wallet_bumpfee.py?
68 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
69 13:14 <bilthon> what's up with the "concept ACK" replies down there
70 13:15 <mryandao> isnt that a similar risk to spending when you consolidate utxos without fee-bumping?
71 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
72 13:15 <merehap> Yeah, just curious how those trade-offs are exposed to the user.
73 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.
75 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
76 13:17 <karimofthecrop> @jnewbery - can you repost your summary for those of us that weren't quite on time?
77 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
79 13:17 <@jnewbery> thanks fanquake
80 13:17 <mryandao> the only bit i'm curious about is where the dust utxo gets destroyed and contributes to fee instead.
81 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?
82 13:18 <mryandao> and dust threshold may increase/decrease in the future?
83 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
84 13:18 <ecurrencyhodler> Copy pasted from jnewbery
85 13:18 <karimofthecrop> th
86 13:18 <karimofthecrop> thx
87 13:19 <@jnewbery> ...by adding a new input to the bumped transaction
89 13:20 <@jnewbery> yes, I suppose the dust threshold could change in future. It's a policy rule, not consensus
90 13:21 <mryandao> so yes, the idea here is my maybe-future-spendable dust gets chucked into fee and I overpaid.
91 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
92 13:21 <mryandao> but i guess, the dust is low enough to be immaterial
93 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.
94 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
95 13:22 <harding> (Or even somewhat above that limit, as eliminating change outputs brings a privacy benefit.)
96 13:23 <mryandao> mm ok - relay policy gotcha.
97 13:23 <dmkathayat> jnewbery: What does CoinControl's m_min_depth do?
98 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?
100 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.
101 13:25 <@jnewbery> the idea of CoinControl is that it can be used to control how a transaction is constructed by the wallet
102 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
103 13:26 <kcalvinalvin> I don't think I'm quite understanding the policy rules. if (dust output > 0); return false; is this correct?
104 13:26 <@jnewbery> m_min_depth just says "don't select coins that have fewer than this many confirmations as inputs"
105 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
106 13:27 <harding> aj: agreed!
107 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
108 13:28 <mryandao> we can only bumpfee once?
111 13:29 <@jnewbery> I really appreciate it when someone reviews my PRs and provides additional tests
112 13:29 <mryandao> src/wallet/feebumper.cpp#299
113 13:29 <merehap> harding: Yeah, very helpful.
114 13:29 <@jnewbery> harding: awesome. Thanks!
115 13:30 <@jnewbery> A comment like that in the PR is really helpful in review: "here's what I tested an my methodology"
116 13:30 <@jnewbery> mryandoa: no, it's possible to bump multiple times
118 13:31 <@jnewbery> each subsequent bump must fulfil those conditions, eg each must have a feerate higher than the previous
119 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.
120 13:32 <mryandao> jnewbery: thanks for that.
121 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
122 13:32 <rafeeki> harding/jnewbery/aj: thanks so much! great feedback and examples
123 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.
124 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
125 13:34 <@jnewbery> ymmv, but the current difftools I use are meld on linux and opendiff on mac
126 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
127 13:35 <mryandao> my tagger is broken, even browsing the python test scripts, the tags point back to the cpp source
128 13:35 <karimofthecrop> jnewbery: what do you mean "ACK the commit hash of the HEAD commit" - where do you ACK? On github?
129 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?
130 13:35 <dmkathayat> Great, thanks!
131 13:35 <ariard> s/I've/have/
134 13:36 <@jnewbery> this means: I've reviewed the branch that ends in commit 184f878, and I think it's ready for merge
135 13:36 <karimofthecrop> I see, I hadn't noticed hashes in previous ACKs. Is this considerd best practice?
136 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
137 13:36 <instagibbs> It's useful when a force push happens and links to old commits are lost on github
138 13:36 <@jnewbery> If I took that commit hash from github, they could be lying to me
139 13:37 <instagibbs> (that too)
140 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
142 13:37 <instagibbs> although unless you gpg sign the ACK, they could just modify what you're saying :)
143 13:38 <fanquake> I still need to improve the docs, and push up some more stuff I have sitting locally.
144 13:38 <ariard> fanquake: thanks bookmarked, maybe could be linked at the end of productivity.md?
145 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 :)
146 13:38 <b10c> fanquake: thanks!
147 13:38 <karimofthecrop> does anyone devel in a docker container instead of a vagrant box?
148 13:39 <fanquake> ariard: I don't think that'd be the right place for personal type repositories.
149 13:39 <karimofthecrop> I have seen some docker containers, wondering if there is a preferred/blessed one.
151 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
152 13:40 <ariard> but yes we have all different setups
153 13:40 <instagibbs> a bit like herding cats, everyone has their own setup
154 13:41 <ariard> and workflow
155 13:41 <@jnewbery> yeah, I think there probably is no royal road to your dev setup. You build it up over the years
156 13:41 <karimofthecrop> instagibbs: has there ever been problems due to inconsistent environments?
157 13:41 <amiti> would love to hear about some of your personal setups
158 13:42 <instagibbs> karimofthecrop, rarely? I mean, I say this as someone running a very common OS in developer circles
159 13:42 <fanquake> karimofthecrop: infrequently bugs have made it into the codebase due to lack of testing on certain OSes.
160 13:42 <karimofthecrop> is there a need for continuous integration to avoid that?
161 13:42 <emzy> Some OS that is missing testing?
162 13:42 <@jnewbery> amit: vagrant, vim+various Tim Pope plugins, git :)
163 13:43 <@jnewbery> *amiti
164 13:43 <mryandao> all tests passed on my end.
166 13:44 <@jnewbery> haha thanks ariard. That's more about generally contributing than specific tools
168 13:45 <amiti> hm yeah, have read that piece, but am more interested in build environments
169 13:45 <ariard> well at least read the sharpening the tool part sometimes ago, was helpful thanks
170 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.
171 13:46 <@jnewbery> we now have Travis and appveyor CIs for testing in linux/windows, although appveyor is still a bit flakey I think
172 13:46 <ariard> yes build is quicly a bottleneck when you want to parallelize your workflow
173 13:46 <afig_> wish i could be a little more involved in the chat but i am currently at work. This is helpful!
174 13:47 <fanquake> We've got at least one BSD CI now, but it's not integrated into the repo.
175 13:47 <karimofthecrop> fanquake: it would seem that a) this isn't a big problem and b) not many nodes on freebsd :P
176 13:47 <@jnewbery> afig_: yeah, it's impossible to have a time that's convenient for everyone. Thanks for logging in anyway
177 13:47 <karimofthecrop> what afig_ said. very useful!
178 13:47 <kcalvinalvin> What distros are must checks for a PR
180 13:49 <instagibbs> kcalvinalvin, in general most changes won't require careful checks of that kind. Unless it involves low-level networking, GUI, ???
181 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
182 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!
184 13:51 <karimofthecrop> jnewbery: how would one go about picking a PR that could use some testing love, as you and harding suggested earlier?
185 13:51 <karimofthecrop> there are 294 currently open!
186 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
188 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?
191 13:54 <karimofthecrop> is it fair to suspect that "needs rebase" tag inidcates a) has enough love and/or b) is kinds stuck?
192 13:54 <karimofthecrop> ^kinds^kinda^
193 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
194 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
195 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
196 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.
197 13:57 <@jnewbery> but definitely feel free to prod authors. You can leave a comment saying "I
198 13:57 <@jnewbery> 'd like to review this, but I want to make sure that it's still being actively maintained"
199 13:57 <@jnewbery> I think that's fine. I wouldn't mind recieving that comment on my PRs
200 13:57 <jonatack> hi! just learned about this excellent initiative. is there a link to a log of this chat?
201 13:57 <karimofthecrop> prod via public comment? or dm?
202 13:58 <@jnewbery> either is fine I think
203 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.
204 13:58 <@jnewbery> other contributors: feel free to chime in if you don't think that's a good idea
205 13:59 <jonatack> thank you
206 13:59 <@jnewbery> harding: that'd be awesome (as long as other people think it'd be useful)
207 13:59 <kcalvinalvin> thanks for this, it was really helpful. Will be back next week
208 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.
210 14:00 <@jnewbery> Thanks everyone!
211 14:00 <mryandao> the assumeutxo one looks like a big one
212 14:00 <jamesc> Thanks jnewbery!
213 14:00 <b10c> jnewbery: thank you!
214 14:00 <MrPaz> thank you!
215 14:00 <harding> jnewbery: thank you!
216 14:00 <merehap> Thanks! This was very helpful. Love the idea and thanks for taking the initiative to run this!
217 14:00 <sebastianvstaa> thanks jnewberry! that was very helpful. will be back next week.
218 14:00 <peevsie> This was great, thanks!
219 14:00 <emzy> Thanks jnewbery
220 14:00 <ecurrencyhodler> Thank you!!
221 14:01 <Lightlike> Thank you, very helpful!
222 14:01 <karimofthecrop> +1 all the thanks :D
223 14:01 <bilthon> thanks jnewbery, got sidetracked there with some of the links you guys sent, definitely will be back here next week
224 14:01 <rafeeki> thanks all! Looking forward to next week
225 14:01 <RubenSomsen> Thanks John :)
226 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
227 14:02 <@jnewbery> Feel free to leave feedback in this IRC channel. I'll be monitoring through the week