#15996 Deprecate totalfee argument in `bumpfee` (rpc)
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!