Deprecate totalfee argument in `bumpfee` (
rpc) Jun 12, 2019
4 13:00 <michaelfolkson> Hey
11 13:01 <michaelfolkson> Nice attendance today. I thought everyone would be tired, jet lagged after Breaking
13 13:01 <sosthene> hi there
16 13:02 <jnewbery> It also demostrates a couple of interesting aspects of the build system, the RPC and the tests
17 13:03 <jnewbery> Did everyone have a chance to check out and build the PR?
18 13:03 <zenogais> Yep, got it building and ran through commits this morning
19 13:03 <michaelfolkson> I haven't built it but I've looked through it
20 13:03 <jnewbery> Great. Any questions?
21 13:03 <sosthene> no sorry, I'm still on my way back from breaking
22 13:04 * nothingmuch is building right now, but on a new setup so can't be sure tests are going to work
25 13:05 <michaelfolkson> So the reason why totalFee was included initially in the RPC was in case the tx change output had to be removed because it was dust? i don't understand why it was included in the RPC in the first place
26 13:05 * setpill heard about this @ breaking, just here to see how things go this time around :)
28 13:05 <nehan> hi! missed the message about the PR and haven't built yet but trying to do so now...
29 13:06 <sosthene> I have a very general question, about the process to deprecate a command or a feature, how is taken this kind of decision?
30 13:06 <zenogais> Yeah, seemed like the function move was more for code cleanup. But honestly not sure why it was moved.
31 13:06 <zenogais> nevermind, see the answer in the commit message
32 13:07 <zenogais> > "I first moved IsDeprecatedRPCEnabled because bitcoin-wallet doesn't link libbitcoin_server."
34 13:07 <jnewbery> right, if we search for rpc/server.cpp, you'll see that's part of libbitcoin_server
35 13:08 <jnewbery> and moving it to rpc/util.cpp moves it to libbitcoin_common, which is linked by the wallet
36 13:08 <zenogais> interesting
37 13:08 <zenogais> are these libs designed to be extracted at some point?
38 13:08 <jnewbery> There's a good write-up of the different libraries by ryanofksy somewhere. Just trying to find it now
39 13:08 <zenogais> would appreciate that!
40 13:09 <michaelfolkson> Me too
42 13:10 <jnewbery> Sorry, the comment was initially made by ryanofksy but the issue was opened by marcofalke, which was why I was struggling to find it
43 13:11 <jnewbery> So currently the code directory structure doesn't reflect the library organization. It'd be nice if we could fix that
44 13:11 <jb55> > totalFee argument is of questionable use why?
45 13:12 <zenogais> +1 to restructuring to reflect the library structure, would be much easier to follow intended organization.
46 13:12 <jnewbery> sosthene: same way for any change. Someone opens a PR if they think we should deprecate something. If it's considered controversial, then it can be discussed in a weekly IRC meeting
47 13:12 <michaelfolkson> <jb55>: I think because you can calculate the totalFee based on the fee rate so it doesn't need to be included in the RPC? Am I right?
48 13:13 <jnewbery> jb55: miners pick transactions based on feerate, not total fee. The idea of bumpfee is to increase the feerate, the totalfee is kind of irrelevant
50 13:14 <michaelfolkson> Why was it included initially?
51 13:14 <jnewbery> when we weren't able to add new inputs in bumpfee and just decrease the amount on the change output, then the total fee is a proxy for the feerate
52 13:14 <jnewbery> but after the change in #15557, the replacement tx might be a different size from the original tx
53 13:16 <setpill> totalFee is still relevant in some corner cases of profitability estimation
54 13:16 <jnewbery> michaelfolkson: I guess it might have been easier to include a totalFee option than a feeratedelta option
55 13:16 <setpill> e.g. CPFP of a tx that has been invalidated via RPF
56 13:17 <jnewbery> setpill: can you elaborate?
57 13:18 <setpill> e.g. tx A1 has been RBF-replaced with A2, but then B1 spends from A1. in the case of non-full-blocks, a rational miner doesn't care about feerate as long as totalFee(A1)+totalFee(B1) > totalFee(A2)
58 13:19 <jnewbery> ah, that's taken care of by RBF rule 3
60 13:20 <jnewbery> The replacement tx must have a total fee greater than the sum of the fees of *all* txs that it replaces
61 13:20 <setpill> that can't take into account B1 being created after A2 :)
62 13:20 <setpill> (yes I know this scenario violates policy)
63 13:20 <jnewbery> right, A1 might as well not exist after it's been replaced
64 13:21 <jnewbery> there's not really much we can do about that - a node can't track all of the txs that have been replaced by RBF
65 13:21 <jnewbery> for starters, it might not even see the replaced txs depending on whether it saw A1 or A2 first
66 13:21 <setpill> right, because of potential for memory exhaustion attack vectors?
67 13:22 <jnewbery> setpill: yes, that's a good reason
68 13:22 <jnewbery> we can't allow our resources to be used without tx fees to add a cost to that resource usage
69 13:23 <setpill> hmm so... isn't totalFee used to enforce rule 3 of RBF?
70 13:23 <jnewbery> if an attacker could produce many replacement txs and we kept hold of all of those versions, then they'd be able to use a lot of memory/bandwidth with very little fee cost
71 13:23 <jnewbery> that's why rule 4 exists - to add a 'relay cost' to RBF
72 13:23 <nothingmuch> there's another scenario, when a RBFable child tx spends an unconfirmed tx, specifying only feerate on the child tx might not have a predictable effect on the combined feerate
73 13:24 <nothingmuch> (but totalFee doesn't fix that)
74 13:24 <jnewbery> setpill: rule 3 of RBF is enforced by AcceptToMemoryPool() looking at the total descendant fee of the tx being replaced
75 13:25 <jnewbery> nothingmuch: yes - good point. If you're bumping the fee on a descendant tx, then you can't know the new/old feerate of the package without looking at all ancestors
77 13:27 <jnewbery> This makes the behaviour change, and it's just a one-line change. We add a call to IsDeprecatedRPCEnabled() when a totalFee option is passed to the RPC. Can anyone say what that function does?
78 13:29 <jnewbery> jonatack: you wrote the testcase for this, so I think you should probably be able to answer that :)
79 13:30 <setpill> seems to check whether a specific deprecated RPC (in this case totalFee) has been explicitly enabled
80 13:31 <jonatack> jnewbery: sorry was afk
81 13:31 <jnewbery> setpill: right - exactly. So why do we deprecate instead of just removing?
82 13:31 <michaelfolkson> Bitcoin Core policy?
83 13:32 <michaelfolkson> GIve people time to adjust with an error message
84 13:32 <jonatack> to give warning and time to users before breaking functionality
86 13:32 <jonatack> so they may adapt
87 13:33 <jnewbery> if anyone has a client that relies on an RPC behaviour and we deprecate it, then when they upgrade to the new version the client will break and they'll get given the error message.
88 13:33 <jnewbery> they can restart bitcoind with the `-deprecatedrpc=<thing>` option and they'll be able to continue for another version before the feature is removed
89 13:34 <jnewbery> the act of having to restart bitcoind with the `-deprecatedrpc` option forces the user to acknowledge the change
90 13:34 <michaelfolkson> Is it always removed over two versions? Deprecated in the next version and then removed in the version after that?
91 13:34 <zenogais> so ensures clients that depend on the feature that upgrade to this version definitely know about the impending deprecation
92 13:34 <jnewbery> michaelfolkson: yes, unless we forget to remove it in the n+1 version
93 13:35 <jnewbery> zenogais: exactly
94 13:35 <setpill> they could always opt not to upgrade, but deprecating before removal allows them to upgrade (and reap any security/performance/other benefits)
95 13:35 <jnewbery> it's easy to do this for RPC changes because we can provide an error directly to the user. It's more difficult to deprecate P2P behaviour safely, because there's not necessarily a way to warn users
96 13:36 <jnewbery> an example of that would be trying to deprecate/remove REJECT messages from P2P
97 13:36 <jonatack> often the principle is to deprecate for one or more minor releases, then remove in the next major release
98 13:37 <jnewbery> setpill: yes, and having one version of deprecation gives them plenty of time to update the client before the next version
99 13:37 <jnewbery> jonatack: yes, we wouldn't remove a feature in a minor release
100 13:37 <setpill> ... so bitcoin core versioning is semver with a 0. prefix?
101 13:38 <jnewbery> no, it's not semver
102 13:38 <jnewbery> it's just major releases are every 6-9 months and we have some number of minor releases between them
103 13:39 <setpill> sorry for tangent, this is not really relevant, I'll ask about it elsewhere :)
105 13:39 <zenogais> One last note on that commit: Looks like 5 tests in wallet_bumpfee.py rely on the newly deprecated behavior so behavior is re-enabled for that set of tests.
106 13:39 <setpill> ah, thanks
107 13:39 <jnewbery> thanks Marco
108 13:39 <jnewbery> zenogais: right, and a new test is added to test the deprecation logic (thanks jonatack!)
109 13:40 <jnewbery> zenogais: makes sense to do it that way and minimize the changes in this PR. As instagibbs notes, the tests can be changed when the functionality is actually removed
110 13:41 <peevsie> was there a good reason for totalFee to be specified in satoshis, while the error message and results from bumpfee are shown in BTC?
111 13:41 <zenogais> Yeah, makes sense. Keeps this PR compact and focused.
112 13:42 <jnewbery> I should note at this point that we previously almost never changed the RPC interface (prior to around 0.16). That made some things quite difficult because we were stuck with some awkward logic, eg around the `getinfo` RPC which reached into network, wallet, mining components
113 13:43 <jnewbery> it's nice for clients to have a stable RPC interface, but not having the freedom to change them had detrimental impacts for the project (eg not being able to fully separate the wallet from the node)
114 13:43 * lightlike kind of misses the "generate" rpc - it was so easy...
115 13:43 <jnewbery> so we've been a bit more active in changing RPCs in the last few releases (while using the deprecation step to hopefully make things less disruptive for clients)
116 13:44 <jonatack> Regarding removing totalFee from the bumpfee tests, it made sense to wait to see if there was consensus on the change
117 13:44 <jnewbery> lightlike: sorry about that! generate called into both wallet and mining code
118 13:44 <jonatack> before taking the next step
119 13:45 <jnewbery> peevsie: I guess we want the user to provide a number in satoshis because a fee will always be 0.0000something BTC
120 13:45 <nothingmuch> doesn't it make more sense to remove the tests only when the functionality is removed in the next release cycle? otherwise a regression might be introduced
121 13:45 <setpill> is there some step between deprecation warning and removal from code? e.g. a release in which the RPC call is not built by default (but can be included in a manual build)?
122 13:46 <setpill> er, RPC interface
123 13:46 <jnewbery> it's easier to get the number wrong if you're writing it as several zeros trailing the decimal point
124 13:46 <jnewbery> and overpay the fee by some orders of magnitude
125 13:46 <jnewbery> nothingmuch: yes, tests shouldn't be removed for deprecated behaviour, only once the functionality is totally removed
126 13:47 <michaelfolkson> <nothingmuch> And that's what happens here too. No tests are removed but one is added
127 13:48 <jnewbery> setpill: no. And it seems unlikely to me that a user would want to do a manual build rather than either just stay on the old version or update the client
128 13:48 <jnewbery> in general we don't want a bunch of #ifdefs litering the code for build configurations
129 13:48 <jnewbery> it makes it more difficult to maintain
130 13:49 <setpill> gotcha
131 13:50 <nothingmuch> michaelfolkson: yep, that was in response to jonatack's remark responding to zenogais (i think), not the PR itself
132 13:50 <michaelfolkson> Ah ok
133 13:50 <peevsie> jnewbery: makes sense, it just seems odd to have an error message that says e.g. "Insufficient totalFee, must be at least 0.00004316" instead of showing it in satoshis as well
134 13:51 <jnewbery> yeah, I agree. That doesn't seem like a very friendly error message
135 13:52 <zenogais> running build and tests again locally, if all goes well, ready to give this an ACK
136 13:52 <jnewbery> There's also a bit of historic context around difficulty using floats in the JSON RPC
137 13:52 <jnewbery> sdaftuar just reminded me of that
138 13:53 <jnewbery> 8 minutes left. If you've been waiting to ask your question, now would be a really good time to ask!
139 13:53 <wallet42> what's the current goal for floats/btc/satoshis per vbytes/tonal btc per kiloweight etc... unification?
140 13:54 <nehan> question about the new test: spend_one_input() is duplicated code. Is there a policy for that in tests?
141 13:55 <zenogais> jnewberry: IIRC ParseNonRFCJSONValue has some issues, I've briefly looked into it in the past
142 13:55 <xis10tial1> First time observer. Just want to say it is interesting.
144 13:56 <michaelfolkson> <zenogais> Are you convinced on the questionable use part? That seems to be the critical question. I need to reread and understand some of the explanations above on why it was useful. Because the bias should surely be on conservatism when removing something people might be using.
145 13:56 <jonatack> nothingmuch: I saw it as updating the tests more than removing tests but could be wrong, point taken
146 13:56 <jnewbery> nehan: if the new test was going to be around forever, then I'd ask for the duplicate code to be moved into test/functional/test_framework, but since we're going to remove the test in the next version it doesn't matter
148 13:58 <michaelfolkson> <xis10tial1> Cool. Attend again next week and come ready with a question :)
149 13:59 <jnewbery> wallet42: no particular plan that I'm aware of
150 13:59 <nothingmuch> btw, what's the reasoning for the magic number 110 in peer.generate? isn't 100 enough for coinbase maturity and rbf always enabled on regtest?
151 14:00 <jnewbery> nothingmuch: I think generate(101) would have been enough
152 14:00 <jnewbery> I don't think we need to nit the test too much since it's not going to be around for long
153 14:00 <jnewbery> ok, let's wrap it up there. Thanks everyone!
154 14:00 <zenogais> Thanks all!
155 14:00 <peevsie> thanks!
156 14:01 <nothingmuch> that was more of a curiosity question than a critique one, seems borrowed from other test =)
157 14:01 <nothingmuch> thanks!
158 14:01 <jonatack> nothingmuch: yes
159 14:01 <jnewbery> See you next week for #15481 - Restrict timestamp when mining a diff-adjustment block to prev-600
160 14:01 <jonatack> thanks!
161 14:01 <michaelfolkson> Thanks
162 14:01 <setpill> thanks
163 14:06 <moneyball> thanks jnewbery and all participants!