<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
<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
<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?
<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
<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
<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)
<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
<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
<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
<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?
<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.
<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
<setpill> they could always opt not to upgrade, but deprecating before removal allows them to upgrade (and reap any security/performance/other benefits)
<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
<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.
<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
<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
<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)
<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)
<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
<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)?
<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
<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
<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.
<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
<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?