Deprecate totalfee argument in `bumpfee` (rpc/rest/zmq)

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

Host: jnewbery  -  PR author: instagibbs

Meeting Log

  113:00 <jnewbery> hi!
  213:00 <moneyball> hi
  313:00 * zenogais waves
  413:00 <michaelfolkson> Hey
  513:00 <nothingmuch> hi
  613:00 <sdupre> hi
  713:01 <jonatack> hi
  813:01 <ajonas> Hi
  913:01 <amiti> hi
 1013:01 <lightlike> hi
 1113:01 <michaelfolkson> Nice attendance today. I thought everyone would be tired, jet lagged after Breaking
 1213:01 <jnewbery> This week we're looking at https://github.com/bitcoin/bitcoin/pull/15996
 1313:01 <sosthene> hi there
 1413:02 <ariard> hi!
 1513: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)
 1613:02 <jnewbery> It also demostrates a couple of interesting aspects of the build system, the RPC and the tests
 1713:03 <jnewbery> Did everyone have a chance to check out and build the PR?
 1813:03 <zenogais> Yep, got it building and ran through commits this morning
 1913:03 <michaelfolkson> I haven't built it but I've looked through it
 2013:03 <jnewbery> Great. Any questions?
 2113:03 <sosthene> no sorry, I'm still on my way back from breaking
 2213:04 * nothingmuch is building right now, but on a new setup so can't be sure tests are going to work
 2313: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?
 2413:05 <kanzure> hi
 2513: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
 2613:05 * setpill heard about this @ breaking, just here to see how things go this time around :)
 2713:05 <peevsie> hi
 2813:05 <nehan> hi! missed the message about the PR and haven't built yet but trying to do so now...
 2913: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?
 3013:06 <zenogais> Yeah, seemed like the function move was more for code cleanup. But honestly not sure why it was moved.
 3113:06 <zenogais> nevermind, see the answer in the commit message
 3213:07 <zenogais> > "I first moved IsDeprecatedRPCEnabled because bitcoin-wallet doesn't link libbitcoin_server."
 3313: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
 3413:07 <jnewbery> right, if we search for rpc/server.cpp, you'll see that's part of libbitcoin_server
 3513:08 <jnewbery> and moving it to rpc/util.cpp moves it to libbitcoin_common, which is linked by the wallet
 3613:08 <zenogais> interesting
 3713:08 <zenogais> are these libs designed to be extracted at some point?
 3813:08 <jnewbery> There's a good write-up of the different libraries by ryanofksy somewhere. Just trying to find it now
 3913:08 <zenogais> would appreciate that!
 4013:09 <michaelfolkson> Me too
 4113:10 <jnewbery> Ah. here it is: https://github.com/bitcoin/bitcoin/issues/15732
 4213: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
 4313:11 <jnewbery> So currently the code directory structure doesn't reflect the library organization. It'd be nice if we could fix that
 4413:11 <jb55> > totalFee argument is of questionable use why?
 4513:12 <zenogais> +1 to restructuring to reflect the library structure, would be much easier to follow intended organization.
 4613: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
 4713: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?
 4813: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
 4913:13 <jb55> I see
 5013:14 <michaelfolkson> Why was it included initially?
 5113: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
 5213:14 <jnewbery> but after the change in #15557, the replacement tx might be a different size from the original tx
 5313:16 <setpill> totalFee is still relevant in some corner cases of profitability estimation
 5413:16 <jnewbery> michaelfolkson: I guess it might have been easier to include a totalFee option than a feeratedelta option
 5513:16 <setpill> e.g. CPFP of a tx that has been invalidated via RPF
 5613:17 <jnewbery> setpill: can you elaborate?
 5713: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)
 5813:19 <jnewbery> ah, that's taken care of by RBF rule 3
 5913:19 <jnewbery> https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki
 6013:20 <jnewbery> The replacement tx must have a total fee greater than the sum of the fees of *all* txs that it replaces
 6113:20 <setpill> that can't take into account B1 being created after A2 :)
 6213:20 <setpill> (yes I know this scenario violates policy)
 6313:20 <jnewbery> right, A1 might as well not exist after it's been replaced
 6413: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
 6513:21 <jnewbery> for starters, it might not even see the replaced txs depending on whether it saw A1 or A2 first
 6613:21 <setpill> right, because of potential for memory exhaustion attack vectors?
 6713:22 <jnewbery> setpill: yes, that's a good reason
 6813:22 <jnewbery> we can't allow our resources to be used without tx fees to add a cost to that resource usage
 6913:23 <setpill> hmm so... isn't totalFee used to enforce rule 3 of RBF?
 7013: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
 7113:23 <jnewbery> that's why rule 4 exists - to add a 'relay cost' to RBF
 7213: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
 7313:24 <nothingmuch> (but totalFee doesn't fix that)
 7413:24 <jnewbery> setpill: rule 3 of RBF is enforced by AcceptToMemoryPool() looking at the total descendant fee of the tx being replaced
 7513: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
 7613:27 <jnewbery> ok, let's move on to the second commit: https://github.com/bitcoin/bitcoin/pull/15996/commits/888b8e9b8f86862f2f6269934392bd7fbc4d59b1
 7713: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?
 7813:29 <jnewbery> jonatack: you wrote the testcase for this, so I think you should probably be able to answer that :)
 7913:30 <setpill> seems to check whether a specific deprecated RPC (in this case totalFee) has been explicitly enabled
 8013:31 <jonatack> jnewbery: sorry was afk
 8113:31 <jnewbery> setpill: right - exactly. So why do we deprecate instead of just removing?
 8213:31 <michaelfolkson> Bitcoin Core policy?
 8313:32 <michaelfolkson> GIve people time to adjust with an error message
 8413:32 <jonatack> to give warning and time to users before breaking functionality
 8513:32 <jnewbery> right
 8613:32 <jonatack> so they may adapt
 8713: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.
 8813: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
 8913:34 <jnewbery> the act of having to restart bitcoind with the `-deprecatedrpc` option forces the user to acknowledge the change
 9013:34 <michaelfolkson> Is it always removed over two versions? Deprecated in the next version and then removed in the version after that?
 9113:34 <zenogais> so ensures clients that depend on the feature that upgrade to this version definitely know about the impending deprecation
 9213:34 <jnewbery> michaelfolkson: yes, unless we forget to remove it in the n+1 version
 9313:35 <jnewbery> zenogais: exactly
 9413:35 <setpill> they could always opt not to upgrade, but deprecating before removal allows them to upgrade (and reap any security/performance/other benefits)
 9513: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
 9613:36 <jnewbery> an example of that would be trying to deprecate/remove REJECT messages from P2P
 9713:36 <jonatack> often the principle is to deprecate for one or more minor releases, then remove in the next major release
 9813:37 <jnewbery> setpill: yes, and having one version of deprecation gives them plenty of time to update the client before the next version
 9913:37 <jnewbery> jonatack: yes, we wouldn't remove a feature in a minor release
10013:37 <setpill> ... so bitcoin core versioning is semver with a 0. prefix?
10113:38 <jnewbery> no, it's not semver
10213:38 <jnewbery> it's just major releases are every 6-9 months and we have some number of minor releases between them
10313:39 <setpill> sorry for tangent, this is not really relevant, I'll ask about it elsewhere :)
10413:39 <MarcoFalke> https://bitcoincore.org/en/lifecycle/#relationship-to-semver
10513: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.
10613:39 <setpill> ah, thanks
10713:39 <jnewbery> thanks Marco
10813:39 <jnewbery> zenogais: right, and a new test is added to test the deprecation logic (thanks jonatack!)
10913: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
11013: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?
11113:41 <zenogais> Yeah, makes sense. Keeps this PR compact and focused.
11213: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
11313: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)
11413:43 * lightlike kind of misses the "generate" rpc - it was so easy...
11513: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)
11613:44 <jonatack> Regarding removing totalFee from the bumpfee tests, it made sense to wait to see if there was consensus on the change
11713:44 <jnewbery> lightlike: sorry about that! generate called into both wallet and mining code
11813:44 <jonatack> before taking the next step
11913:45 <jnewbery> peevsie: I guess we want the user to provide a number in satoshis because a fee will always be 0.0000something BTC
12013: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
12113: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)?
12213:46 <setpill> er, RPC interface
12313:46 <jnewbery> it's easier to get the number wrong if you're writing it as several zeros trailing the decimal point
12413:46 <jnewbery> and overpay the fee by some orders of magnitude
12513:46 <jnewbery> nothingmuch: yes, tests shouldn't be removed for deprecated behaviour, only once the functionality is totally removed
12613:47 <michaelfolkson> <nothingmuch> And that's what happens here too. No tests are removed but one is added
12713: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
12813:48 <jnewbery> in general we don't want a bunch of #ifdefs litering the code for build configurations
12913:48 <jnewbery> it makes it more difficult to maintain
13013:49 <setpill> gotcha
13113:50 <nothingmuch> michaelfolkson: yep, that was in response to jonatack's remark responding to zenogais (i think), not the PR itself
13213:50 <michaelfolkson> Ah ok
13313: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
13413:51 <jnewbery> yeah, I agree. That doesn't seem like a very friendly error message
13513:52 <zenogais> running build and tests again locally, if all goes well, ready to give this an ACK
13613:52 <jnewbery> There's also a bit of historic context around difficulty using floats in the JSON RPC
13713:52 <jnewbery> sdaftuar just reminded me of that
13813:53 <jnewbery> 8 minutes left. If you've been waiting to ask your question, now would be a really good time to ask!
13913:53 <wallet42> what's the current goal for floats/btc/satoshis per vbytes/tonal btc per kiloweight etc... unification?
14013:54 <nehan> question about the new test: spend_one_input() is duplicated code. Is there a policy for that in tests?
14113:55 <zenogais> jnewberry: IIRC ParseNonRFCJSONValue has some issues, I've briefly looked into it in the past
14213:55 <xis10tial1> First time observer. Just want to say it is interesting.
14313:55 <zenogais> Investigation is here: https://github.com/bitcoin/bitcoin/issues/14173#issuecomment-427605111
14413: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.
14513:56 <jonatack> nothingmuch: I saw it as updating the tests more than removing tests but could be wrong, point taken
14613: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
14713:57 <jnewbery> See the comment here: https://github.com/bitcoin/bitcoin/pull/15996#issuecomment-491888247
14813:58 <michaelfolkson> <xis10tial1> Cool. Attend again next week and come ready with a question :)
14913:59 <jnewbery> wallet42: no particular plan that I'm aware of
15013: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?
15114:00 <jnewbery> nothingmuch: I think generate(101) would have been enough
15214:00 <jnewbery> I don't think we need to nit the test too much since it's not going to be around for long
15314:00 <jnewbery> ok, let's wrap it up there. Thanks everyone!
15414:00 <zenogais> Thanks all!
15514:00 <peevsie> thanks!
15614:01 <nothingmuch> that was more of a curiosity question than a critique one, seems borrowed from other test =)
15714:01 <nothingmuch> thanks!
15814:01 <jonatack> nothingmuch: yes
15914:01 <jnewbery> See you next week for #15481 - Restrict timestamp when mining a diff-adjustment block to prev-600
16014:01 <jonatack> thanks!
16114:01 <michaelfolkson> Thanks
16214:01 <setpill> thanks
16314:06 <moneyball> thanks jnewbery and all participants!