#15996 Deprecate totalfee argument in `bumpfee` (rpc)

https://github.com/bitcoin/bitcoin/15996

Host: jnewbery

Meeting Log

13:00 < jnewbery> hi!
13:00 < moneyball> hi
13:00  * zenogais waves
13:00 < michaelfolkson> Hey
13:00 < nothingmuch> hi
13:00 < sdupre> hi
13:01 < jonatack> hi
13:01 < ajonas> Hi
13:01 < amiti> hi
13:01 < lightlike> hi
13:01 < michaelfolkson> Nice attendance today. I thought everyone would be tired, jet lagged after Breaking
13:01 < jnewbery> This week we're looking at https://github.com/bitcoin/bitcoin/pull/15996
13:01 < sosthene> hi there
13:02 < ariard> hi!
13:02 < jnewbery> It's a pretty small change, but it's a follow-up to the PR we looked at in the first week (https://github.com/bitcoin/bitcoin/pull/15557)
13:02 < jnewbery> It also demostrates a couple of interesting aspects of the build system, the RPC and the tests
13:03 < jnewbery> Did everyone have a chance to check out and build the PR?
13:03 < zenogais> Yep, got it building and ran through commits this morning
13:03 < michaelfolkson> I haven't built it but I've looked through it
13:03 < jnewbery> Great. Any questions?
13:03 < sosthene> no sorry, I'm still on my way back from breaking
13:04  * nothingmuch is building right now, but on a new setup so can't be sure tests are going to work
13:05 < jnewbery> The first commit is https://github.com/bitcoin/bitcoin/pull/15996/commits/0c39465500af5309d6ca39686634c3f1e7d58d91 . It moves a function from src/rpc/server.cpp to src/rpc/util.cpp . Any idea why?
13:05 < kanzure> hi
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
13:05  * setpill heard about this @ breaking, just here to see how things go this time around :)
13:05 < peevsie> hi
13:05 < nehan> hi! missed the message about the PR and haven't built yet but trying to do so now...
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?
13:06 < zenogais> Yeah, seemed like the function move was more for code cleanup. But honestly not sure why it was moved.
13:06 < zenogais> nevermind, see the answer in the commit message
13:07 < zenogais> > "I first moved IsDeprecatedRPCEnabled because bitcoin-wallet doesn't link libbitcoin_server."
13:07 < jnewbery> First place to look is in https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.am, which is where the different libraries are defined
13:07 < jnewbery> right, if we search for rpc/server.cpp, you'll see that's part of libbitcoin_server
13:08 < jnewbery> and moving it to rpc/util.cpp moves it to libbitcoin_common, which is linked by the wallet
13:08 < zenogais> interesting
13:08 < zenogais>  are these libs designed to be extracted at some point?
13:08 < jnewbery> There's a good write-up of the different libraries by ryanofksy somewhere. Just trying to find it now
13:08 < zenogais> would appreciate that!
13:09 < michaelfolkson> Me too
13:10 < jnewbery> Ah. here it is: https://github.com/bitcoin/bitcoin/issues/15732
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
13:11 < jnewbery> So currently the code directory structure doesn't reflect the library organization. It'd be nice if we could fix that
13:11 < jb55> > totalFee argument is of questionable use    why?
13:12 < zenogais> +1 to restructuring to reflect the library structure, would be much easier to follow intended organization.
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
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?
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
13:13 < jb55> I see
13:14 < michaelfolkson> Why was it included initially?
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
13:14 < jnewbery> but after the change in #15557, the replacement tx might be a different size from the original tx
13:16 < setpill> totalFee is still relevant in some corner cases of profitability estimation
13:16 < jnewbery> michaelfolkson: I guess it might have been easier to include a totalFee option than a feeratedelta option
13:16 < setpill> e.g. CPFP of a tx that has been invalidated via RPF
13:17 < jnewbery> setpill: can you elaborate?
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)
13:19 < jnewbery> ah, that's taken care of by RBF rule 3
13:19 < jnewbery> https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki
13:20 < jnewbery> The replacement tx must have a total fee greater than the sum of the fees of *all* txs that it replaces
13:20 < setpill> that can't take into account B1 being created after A2 :)
13:20 < setpill> (yes I know this scenario violates policy)
13:20 < jnewbery> right, A1 might as well not exist after it's been replaced
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
13:21 < jnewbery> for starters, it might not even see the replaced txs depending on whether it saw A1 or A2 first
13:21 < setpill> right, because of potential for memory exhaustion attack vectors?
13:22 < jnewbery> setpill: yes, that's a good reason
13:22 < jnewbery> we can't allow our resources to be used without tx fees to add a cost to that resource usage
13:23 < setpill> hmm so... isn't totalFee used to enforce rule 3 of RBF?
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
13:23 < jnewbery> that's why rule 4 exists - to add a 'relay cost' to RBF
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
13:24 < nothingmuch> (but totalFee doesn't fix that)
13:24 < jnewbery> setpill: rule 3 of RBF is enforced by AcceptToMemoryPool() looking at the total descendant fee of the tx being replaced
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
13:27 < jnewbery> ok, let's move on to the second commit: https://github.com/bitcoin/bitcoin/pull/15996/commits/888b8e9b8f86862f2f6269934392bd7fbc4d59b1
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?
13:29 < jnewbery> jonatack: you wrote the testcase for this, so I think you should probably be able to answer that :)
13:30 < setpill> seems to check whether a specific deprecated RPC (in this case totalFee) has been explicitly enabled
13:31 < jonatack> jnewbery: sorry was afk
13:31 < jnewbery> setpill: right - exactly. So why do we deprecate instead of just removing?
13:31 < michaelfolkson> Bitcoin Core policy?
13:32 < michaelfolkson> GIve people time to adjust with an error message
13:32 < jonatack> to give warning and time to users before breaking functionality
13:32 < jnewbery> right
13:32 < jonatack> so they may adapt
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.
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
13:34 < jnewbery> the act of having to restart bitcoind with the `-deprecatedrpc` option forces the user to acknowledge the change
13:34 < michaelfolkson> Is it always removed over two versions? Deprecated in the next version and then removed in the version after that?
13:34 < zenogais> so ensures clients that depend on the feature that upgrade to this version definitely know about the impending deprecation
13:34 < jnewbery> michaelfolkson: yes, unless we forget to remove it in the n+1 version
13:35 < jnewbery> zenogais: exactly
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)
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
13:36 < jnewbery> an example of that would be trying to deprecate/remove REJECT messages from P2P
13:36 < jonatack> often the principle is to deprecate for one or more minor releases, then remove in the next major release
13:37 < jnewbery> setpill: yes, and having one version of deprecation gives them plenty of time to update the client before the next version
13:37 < jnewbery> jonatack: yes, we wouldn't remove a feature in a minor release
13:37 < setpill> ... so bitcoin core versioning is semver with a 0. prefix?
13:38 < jnewbery> no, it's not semver
13:38 < jnewbery> it's just major releases are every 6-9 months and we have some number of minor releases between them
13:39 < setpill> sorry for tangent, this is not really relevant, I'll ask about it elsewhere :)
13:39 < MarcoFalke> https://bitcoincore.org/en/lifecycle/#relationship-to-semver
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.
13:39 < setpill> ah, thanks
13:39 < jnewbery> thanks Marco
13:39 < jnewbery> zenogais: right, and a new test is added to test the deprecation logic (thanks jonatack!)
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
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?
13:41 < zenogais> Yeah, makes sense. Keeps this PR compact and focused.
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
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)
13:43  * lightlike kind of misses the "generate" rpc - it was so easy...
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)
13:44 < jonatack> Regarding removing totalFee from the bumpfee tests, it made sense to wait to see if there was consensus on the change
13:44 < jnewbery> lightlike: sorry about that! generate called into both wallet and mining code
13:44 < jonatack> before taking the next step
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
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
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)?
13:46 < setpill> er, RPC interface
13:46 < jnewbery> it's easier to get the number wrong if you're writing it as several zeros trailing the decimal point
13:46 < jnewbery> and overpay the fee by some orders of magnitude
13:46 < jnewbery> nothingmuch: yes, tests shouldn't be removed for deprecated behaviour, only once the functionality is totally removed
13:47 < michaelfolkson> <nothingmuch> And that's what happens here too. No tests are removed but one is added
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
13:48 < jnewbery> in general we don't want a bunch of #ifdefs litering the code for build configurations
13:48 < jnewbery> it makes it more difficult to maintain
13:49 < setpill> gotcha
13:50 < nothingmuch> michaelfolkson: yep, that was in response to jonatack's remark responding to zenogais (i think), not the PR itself
13:50 < michaelfolkson> Ah ok
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
13:51 < jnewbery> yeah, I agree. That doesn't seem like a very friendly error message
13:52 < zenogais> running build and tests again locally, if all goes well, ready to give this an ACK
13:52 < jnewbery> There's also a bit of historic context around difficulty using floats in the JSON RPC
13:52 < jnewbery> sdaftuar just reminded me of that
13:53 < jnewbery> 8 minutes left. If you've been waiting to ask your question, now would be a really good time to ask!
13:53 < wallet42> what's the current goal for floats/btc/satoshis per vbytes/tonal btc per kiloweight etc... unification?
13:54 < nehan> question about the new test: spend_one_input() is duplicated code. Is there a policy for that in tests?
13:55 < zenogais> jnewberry: IIRC ParseNonRFCJSONValue has some issues, I've briefly looked into it in the past
13:55 < xis10tial1> First time observer. Just want to say it is interesting.
13:55 < zenogais> Investigation is here: https://github.com/bitcoin/bitcoin/issues/14173#issuecomment-427605111
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.
13:56 < jonatack> nothingmuch: I saw it as updating the tests more than removing tests but could be wrong, point taken
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
13:57 < jnewbery> See the comment here: https://github.com/bitcoin/bitcoin/pull/15996#issuecomment-491888247
13:58 < michaelfolkson> <xis10tial1> Cool. Attend again next week and come ready with a question :)
13:59 < jnewbery> wallet42: no particular plan that I'm aware of
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?
14:00 < jnewbery> nothingmuch: I think generate(101) would have been enough
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
14:00 < jnewbery> ok, let's wrap it up there. Thanks everyone!
14:00 < zenogais> Thanks all!
14:00 < peevsie> thanks!
14:01 < nothingmuch> that was more of a curiosity question than a critique one, seems borrowed from other test =)
14:01 < nothingmuch> thanks!
14:01 < jonatack> nothingmuch: yes
14:01 < jnewbery> See you next week for #15481 - Restrict timestamp when mining a diff-adjustment block to prev-600
14:01 < jonatack> thanks!
14:01 < michaelfolkson> Thanks
14:01 < setpill> thanks
14:06 < moneyball> thanks jnewbery and all participants!