[RPC] Simplified bumpfee command.

[mrbandrews]

This purports to be a simplified "bumpfee" RPC command.

This pull was motivated by the discussion in #7865, where at the end, the idea was floated of submitting a simplified version of the code. I put a little thought into how a simple "bumpfee" command should work and here is what I came up with:

1. The user should specify which output to decrement in order to pay increased fee. This saves us from trying to figure out which address(es) are "change" addresses. This is preferable not only because it simplifies the code, but because as far as I can tell, the code that identifies change outputs is not particularly robust and may be modified in the future. If it's desirable to have user-friendly code that figures out which output to decrement, perhaps that logic could be placed in the GUI?

2. The command should not add new inputs. In other words, if the user can't identify an output that can be decremented in an amount sufficient to bump the fee then the command should fail. It seems likely that the vast majority of the time, identifying such an output would not be a problem. Adding new inputs complicates the code because the size of the transaction increases, perhaps significantly, so then the user would have to pay more fee for that reason as well, as opposed to just bumping the fee on the tx as it currently exists.

3. If the tx has descendant transactions, the bumped tx has to pay the fees for all of the descendants since they will get evicted from the mempool, and the rule as I understand it is that replacing a tx cannot cause the total fees in the mempool to go down.

So, the required inputs are the txid and the output index.

The optional inputs are:

confirmTarget - perhaps the reason the user feels the need to bump the fee is because the original confirmTarget was too slow
totalFee - for full control, the user can simply specify how many satoshis to pay as a fee on the bumped transaction (this may be useful when the parent tx has children tx'es)

This pull includes a python test.

[jonasschnelli]

Thanks! Looks good.
Concept ACK will review and test soon.

[NicolasDorier]

nit: Double negation makes it hards to read. I suggest to replace by

 CAmount nOldFee = wtx.IsFromMe(ISMINE_SPENDABLE) ? nDebit - wtx.GetValueOut() : 0;

[mrbandrews]

Fixed.

[NicolasDorier]

I'm not sure about this. The problem is that such case will rarely happen so applications risk to break during sudden peak usage, amplifying bad news.

I suggest to silently bump fees to the minMempoolFeeRate.

[mrbandrews]

I edited the comment to explain why I'm reporting an error. Especially if the user set totalFee (or had in the recent past used paytxfee to set an explicit fee rate) I think silently bumping could surprise the user.

[NicolasDorier]

nit:

 nFeesWithDescendants + ::minRelayTxFee.GetFee(txSize)

Is repeated. Suggest using variable.

[mrbandrews]

Fixed.

[NicolasDorier]

additional check should be done:

nDelta >= poutput->GetDustThreshold(::minRelayTxFee);

Or it won't get propagated.

[mrbandrews]

I checked earlier that the bumped fee pays for the new relay fee; I'm not following as to the relationship between nDelta and dust.

[luke-jr]

@NicolasDorier I also don't see a need for that...

[NicolasDorier]

can you use C++11 loop style ?

[mrbandrews]

Good idea, fixed.

[NicolasDorier]

You should use the same SIGHASH than the previous one. (might be done separate PR ?)

[mrbandrews]

Let's punt to separate PR.

[NicolasDorier]

(nIn < (int)(*mi).second.vout.size())

I think this is a bug, why does the index of the input to sign should be below the number of vout of the parent ? Does not make sense.

[mrbandrews]

Good catch, edited to bounds-check the prevout.

[jonasschnelli]

Needs rebase and reviewers... setting 0.14 milestone.

[mrbandrews]

Rebased and addressed feedback.

[jonasschnelli]

Needs rebase again...

[mrbandrews]

Rebased and edited to use JSONRPCRequest, consistent with #8788.

[luke-jr]

This line is outdated

[RHavar]

I think it's very important for this command to actual figure out the change output on its own. I understand that it's a bit messier and more fragile doing it here -- but it actually has enough information to do this, and by avoiding it, it just pushes that mess into the caller which comes at a very significant usability issue.

Compare:
Transaction stuck? Use bumpfee $txid

vs

Transaction stuck? First use gettransaction $txid, and now get the data from the "hex" field to feed into decoderawtransaction $hex now go through the "vout" fields until you find "addresses" nested in the "scriptPubKey". Now try figure out which is a change address. (I can't even see this exposed over rpc at all?). So maybe go validateaddress $address and check the ismine field. If one of them is yours, but the other one isn't -- then the one that is yours is a change address, so now go back through the results of decoderawtransaction to figure out which index that is. And finally bumpfee $txid $index. Oh yeah, becareful to handle all the edge cases, like the transaction only having 1 output, some of the outputs not having address, both having the same address etc.

I think you'll find the usability problem of needing to know the index will prevent the majority of use by users and services for this =)

[RHavar]

Also ideally, I think the txid argument should be an array of transaction ids to bump. And then it creates a single transaction that bumps the fees on all of those transactions (stripping out extraneous change outputs as it goes). However, I imagine this can be done separately and later as the txid argument could be overloaded to either accept a string or array of strings?

[JeremyRubin]

Overall seems like the right behavior for bumpfee, just a few minor suggestions.

It seems that the descendants fee adds additional complexity that probably can't be addressed in the scope of this PR but at least could merit some documentation so that a stuck user can find out what's wrong.

[JeremyRubin]

Perhaps make this fail if there are any options other than confTarget or totalFee passed in to guard against the case where a user has a typo or something.

[JeremyRubin]

Maybe add a tighter limit to your bumpfee code, bumpfeeMaxTxFee. Set this to be an optional default parameter in options. maxTxFee is huge, so might be safer to have something smaller and not much added code. This parameter can be overridden if need be, but not to more the maxTxFee.

[JeremyRubin]

Using a nNewFee = 0 is fine here, it just slightly bothers me to use this as a null value when indeed 0 is a valid fee amount.

-1 would be an invalid fee amount (creates coins) so I have a slight preference to use this.

[JeremyRubin]

There is kind of a weird deal here where nDelta can be <= 0 if totalFee is set, and will be accepted on a race condition with mempool, I would explicitly guard against this case by restricting nDelta > 0.

[mrbandrews]

Done.

[morcos]

@JeremyRubin how could nDelta be negative here?

[JeremyRubin]

I would suggest that this should also return in the error message the txid of the furthest child, and suggest bumping the fee on that one if possible to take advantage of ancestor fee based mining and keep txs valid?

[mrbandrews]

As far as I can tell it's a little tricky to track down the furthest child (and there could be numerous furthest children at the same level) but I edited the error message to give the user more info (number of children and size of those transactions) and explaining the situation a bit better.

[JeremyRubin]

There is something kinda funky when a user is running in say blocksonlymode and doesn't know about any child transactions that may exist and therefore has trouble setting the fee correctly for those that they will invalidate.

This is probably a hard problem to solve; so I'm just pointing it out.

[mrbandrews]

Addressed JeremyRubin feedback, edited the python test, and made a few other small changes I noticed with further testing.

RHavar: I understand your point but I still think it's better for this command to be low-level and not fragile. A more user-friendly RPC (e.g., "bumpfeeauto" or something) could be layered on top, identifying the change output and then using this code. Then if the change-output-identifying code breaks, it might break the user-friendly version but it wouldn't dismantle RBF entirely.

[Victorsueca]

ACK f3833f4
Tested on Windows x64

bumpfee 8c56b13830405a55ec4bc58b26b531f1b187d2349ee19bd0dd01aa835972929a 1

{
  "txid": "67d6af1a3e29e246eaef0b7ce272f745e2ae6178050ccb78fca515b13c0f9e92",
  "oldfee": 0.00000260,
  "fee": 0.00000520
}

[jonasschnelli]

nit: Maybe use txid to correspond with the call header at L2587 or use transactionid there?

[jonasschnelli]

add third parameter ("options")?

[jonasschnelli]

I'm not sure if we want absolute fee values here.
The user can't be sure how many inputs are getting added when setting this value, probably resulting in an uncontrollable feerate.

What about switching this to a feerate?

[Victorsueca]

Another possibility is to leave the RPC with an absolute value and use Fee/KB on the GUI.
Some software may want to use it's own relative fee rate.

[jonasschnelli]

This check looks really expensive for large wallets. Why not calling mapWallet.find(hash) and check nDepth (and maybe call GetConflicts() to ensure its not conflicted with a already mined tx)?

[jonasschnelli]

Oh. I just realized that this PR does not add new inputs (it requires an output index to identify the change-output which then can be reduced).

IMO we should...

1.) not let the user identify which output is change
2.) allow bumping fees including adding new inputs (some transactions do not have a change output, some will not allow a reasonable bumping without adding a new input).

But 1.) & 2.) can also be solved later.

[mrbandrews]

Feedback addressed. Good catch on checking whether the tx had already been mined - that code was able to be shortened to a single line.
Yes, the approach of this PR is to solve 1) and 2) later.
If this looks good I'll squash the commits.

[morcos]

I don't think this section makes sense. Transactions which are in newer in the wallet are not a good indication of the most "up to date" spend of the outputs. If anything, conflicts in the mempool would be a good proxy, because at least for those you'd be able to calculate any descendants fees. But it may make the most sense to make this more of a utility function and just assume the user is trying to bump the right txid. I would just eliminate this set of checks entirely.

[mrbandrews]

Fixed (removed)

[morcos]

Would be helpful to refer to this as the output index throughout... (comments, help, and error messages)

[mrbandrews]

Done.

[morcos]

We need to be sure we are calculating the correct fee here. Either using new IsAllFromMe or getting the fee from the mempool.

[mrbandrews]

This should be correct since now we're checking that the user owns all the inputs.

[morcos]

you don't need this, you can just leave it out as an argument

[mrbandrews]

Fixed.

[morcos]

I think this reduces bumpfee to a two step process in the common case (whenever there are children) and makes it so the second step is going to require setting totalfee. It seems more user friendly to just pay what it takes?

On a separate note, I think it might be nice to check all of the children transactions and make sure their ancestor fee rate isn't higher than the new feerate you are bumping to. If one of them has a higher ancestor fee rate you are actually make your situation worse.

[morcos]

I spoke with @sdaftuar and @mrbandrews about this and withdraw both of the above points. If the transaction has children, it becomes quite complicated to decide whether bumping fee makes sense and by how much and its probably better to just report to the user the minimum fee that would be required to bump and let the user decide whether that makes sense.

[mrbandrews]

morcos feedback addressed, including adding the first commit from #9167.

[morcos]

You should probably check that the depth is exactly 0, because otherwise the tx is already mined or conflicted with a mined tx and no point in bumping.

[luke-jr]

IMO the main logic should probably be split up into non-RPC files so it can be reused in the GUI...

[luke-jr]

Maybe move output into options? Even if it's required in this version, it should ideally become optional in the future...

[luke-jr]

"BIP 125"

[luke-jr]

Just use int64_t? int is only guaranteed to be 15-bit, so it's too small anyway.

[luke-jr]

nNewFeeRate = payTxFee;?

[luke-jr]

+ wtx.vin.size() seems appropriate here too?

[luke-jr]

s/would/may probably/

In the future, it may make sense to combine some children transactions (ie, our own) into this one while bumping the fee.

[luke-jr]

@NicolasDorier I also don't see a need for that...

[luke-jr]

Why not just discard the change?

[luke-jr]

This needs to be rebased...

[luke-jr]

No need for strprintf here

[luke-jr]

Also no strprintf here

[jonasschnelli]

I think, because every else use fee-ratres per KB, we should make clear at this point that totalFee and maxFee are absolute fee-values (and not per KB).

[luke-jr]

del strprintf

[mrbandrews]

OK, rebased and squashed, with edits addressing the most recent feedback in a separate commit.

I didn't separate code into non-rpc files because I'm not 100% sure which logic should be moved, and thought this decision could be made when using it from the GUI, as moving the code then should be easy enough.

[luke-jr]

You should calculate txSize + wtx.vin.size() once and keep it in a const variable for these...

const int64_t maxNewTxSize = txSize + wtx.vin.size();

[gmaxwell]

Asking the user to identify change is unreasonable and dangerous. Within our own wallet we should know which outputs are our own. This also has a problem of creating a mess when the original version of the transaction it spent, but later sends may have spent the replacements output.

I would suggest that a minimal bump-fee would do this:

(1) Only be available on transactions where none of their outputs have been spent (even in mempool).
(2) Preserve all the user provided outputs, potentially changing the change amount, potentially adding more inputs if needed to get past dust or just sufficient fees. Obey the opt-in RBF minimum increment
(3) Mark its local output(s) as unspendable until confirmed.

1/3 are required so that we don't end up with a mess when the 'wrong' version of the transaction confirms and invalidates the others.

A somewhat more advanced -- and perhaps better to do instead-- would be a "bumpunconfirmed" which would not act on a single transaction but on all valid unconfirmed sends of the local wallet at once-- generating a single replacement transaction which conflicts each of the originals, pays a higher fee, and marks its own output as unspendable until confirmed.

I suggest this might be better instead because it would not need to have the requirement that its local outputs not be unspent, since it would replace all those spends at once. The reduced number of change outputs plus the ability to drop some of the inputs (so long as it still conflicts, and still marks them as unspendable until this transaction confirms) means that it can reduce the total transaction sizes.

[mrbandrews]

Addressed the gmaxwell feedback as follows: now it identifies the change output and guards against spending outputs until the bumped transaction (or perhaps the original transaction) is mined.

I reorganized this into two commits, the latter being the python test, which is updated.

[luke-jr]

Any reason not to automatically just switch to the new txid here?

[mrbandrews]

For now, I'd rather leave it as is, and let the user resubmit with the correct txid.

[luke-jr]

Just use a size_t here, not cast to int... And use ++i, not i++

But more importantly, better to use C++11 iterators in this case:

for (const CTxOut& txout : wtx.tv->vout) {
    if (pwalletMain->IsChange(txout)) {
        if (nOutput != -1) {
            throw JSONRPCError(RPC_MISC_ERROR, "Transaction has multiple change outputs");
        }
        nOutput = i;
    }
}

[sdaftuar]

@luke-jr That won't compile sincei is undefined... Is there a better way to loop here where the index is needed? Also I'd suggest that this kind of style nit can be cleaned up in a later pull anyway; this one has been open for long enough.

[mrbandrews]

Changed to a size_t (and ++i)

[luke-jr]

I think we're trying to keep CWalletDB only used by CWallet. @pstratem ?

[luke-jr]

Probably this needs to go through CWallet::AddToWallet

[luke-jr]

May need to be atomic with adding the new one as well...

[mrbandrews]

I moved this code into a new method on the wallet so the CWalletDB is only used from CWallet. I looked at AddToWallet but I would need to have edited it a bit, plus it does some stuff I don't need so I thought a new but concise method was a better approach.

[luke-jr]

I wonder if this ought to interact with abandontransaction in some way?

[sdaftuar]

Looks pretty good to me overall, though there's one bug here (after #8850) that needs to be fixed, plus some doc fixes to reflect the changes that have been made in this PR from when it was first opened.

Also, I think bumpfee.py should be added to rpc-tests.py.

[sdaftuar]

I think this line should be removed now that the output is no longer specified?

[sdaftuar]

I found this sentence, and the ones below ("This command will fail if fee is not high enough...") to be confusing. Perhaps some explanation of what automatic fee calculation will occur, unless overriden/modified by the options, and that if the resulting fee isn't high enough then the command will fail?

[sdaftuar]

This line in the help text needs to be updated for the latest semantics (no descendants in mempool or wallet).

[sdaftuar]

<output> should not be here anymore.

[sdaftuar]

I think this cast isn't correct after #8580 was merged; looks like the correct way to do this is GetVirtualTransactionSize(*wtx->tx).

[sdaftuar]

I don't think we should rely on the CMerkleTx function

operator const CTransaction&() const { return *tx; }

that makes this work, as that function has a comment saying it should be removed. *wtx->tx here again I think?

[mrbandrews]

Pushed a new commit addressing the recent feedback and rebased due to a conflict in rpc-tests.py.

[ryanofsky]

Looks good. Lots of nitpicky comments, nothing major.

[ryanofsky]

self isn't being used here, would suggest making this a standalone function like submit_block_with_tx below.

[ryanofsky]

This test is long and it's not clear right away whether the different parts of it depend on each other. If it could be broken up in some way, I think it would be more readable and easier to work with in the future. E.g.

self.test_simple_bumpfee_succeeds(a1)
self.test_nonrbf_bumpfee_fails(a1)
self.test_notmine_bumpfee_fails(a0)
...

[ryanofsky]

Maybe say this will decrease and possibly remove the change output, but leave other outputs alone. Also could say explicitly that this will not change existing inputs.

[ryanofsky]

Unclear what good having this variable does. Seems like it's always just set in one line then thrown in the next line, making the code more verbose and causing unnecessary string copies.

[ryanofsky]

Why use 0.01 * COIN instead of the global maxTxFee? I think it would be worth explaining in a comment. Also, if 0.01 * COIN is a significant value, maybe it would be good to declare it in validation.h alongside similar sounding constants like HIGH_MAX_TX_FEE.

[MarcoFalke]

Regardless, we don't want to use flops for CAmount.

(I know that in this case the compiler can do static analysis and figure out the exact result, but we should not promote bad practice; It is proved to break on 32 bit platforms when static code analysis does not catch the flop.)

[ryanofsky]

I would move this declaration down closer to where the variable is first used (line 2719 nNewFee = nNewFeeRate.GetFee(maxNewTxSize);

[ryanofsky]

This block of code seems very similar to existing code in CWallet::GetMinimumFee. If you tweaked that function to return the fee rate in addition to the fee, could that function be reused here?

[ryanofsky]

Seems like if this condition is true, the work done above gets thrown away. Maybe that code could be moved into an else block so the sematics of the totalFee > 0 case are more straightforward.

[ryanofsky]

Would suggest changing error code below to RPC_INTERNAL_ERROR if it's really not possible for this condition to happen at this point.

[ryanofsky]

What should the user do when this case happens? Should they report a bug because it should not be possible for this condition to happen? Or should they just try passing in a high(er) totalFee value? I think it'd be good to clarify in the error message.

[ryanofsky]

I think it would make sense to move this check up towards the top of this function near the pwalletMain / wtx checks, instead of leaving it down here after fee estimation & option parsing. As a user I wouldn't want to first resolve a bunch of errors about fees and options before finding out that the transaction I am trying to bump can't be bumped in the first place.

[ryanofsky]

Maybe use poutput->nValue -= nDelta syntax here (and nNewFee += poutput->nValue below). += and -= operators I think would make the code easier to read and less prone to typo errors.

[ryanofsky]

Think <= would be better than < here. Thinking in a theoretical situation where nValue was 0 and dust threshould was somehow also 0, it would be more consistent to remove the output than keep it.

[ryanofsky]

it isn't really a good name for this variable, since it's a direct reference to the input, and not an iterator. Maybe in or input would be a better name.

[ryanofsky]

Maybe use auto here since you're already using it one line up.

[ryanofsky]

Can write mi->second instead of (*mi).second.

[ryanofsky]

Would suggest replacing tx.vin[nIn] here with it or replacing the opposite way above to be consistent about how the input is referred to inside the loop.

[ryanofsky]

Maybe add || !state.IsValid() to the condition here or assert(state.IsValid()) as a sanity check below.

[mrbandrews]

Pushed a new commit addressing ryanofsky's feedback.

I removed the maxFee option. The rationale for this option was to have a more conservative tx fee limit for a bumpfee user, as the default maxTxFee (0.1 btc) is quite high and in particular, if bumping fee on a transaction with lots of descendants a user might end up paying a pretty high fee for the parent transaction alone (since the descendants would be removed). Since the new approach is to not allow bumpfee to be used on a tx with descendants at all, this rationale mostly goes away and I think the maxFee option is not worth it.

I moved the python test above 'nulldummy.py' in the pull-tester list for the purpose of avoiding a conflict.

[mrbandrews]

I got a travis failure I think due to the default transaction version=2, which caused a failure in the python test. I corrected the test, rebased, and force-pushed.

[luke-jr]

I don't think we should guarantee this. (sorry for reposting)

[ryanofsky]

Thanks, I changed the wording to describe the current behavior but suggest it could be improved in the future.

[morcos]

@sdaftuar points out that this won't correctly update scriptWitness for witness transactions
Probably better to use the UpdateTransaction function

[ryanofsky]

Added UpdateTransaction in e4a7662, and after a lot of struggle, eventually got a test to work which creates a segwit transaction and makes sure scriptWitness is set.

The test also uncovered another bug in the TransactionSignatureCreator call above where the SIGHASH_ALL value was incorrectly being interpretted as a CAmount. This is also fixed in e4a7662.

[morcos]

nit: mentioning fallbackfee is too much implementation detail here

[ryanofsky]

Removed in e4a7662.

[morcos]

Should there be a check against providing both options

[ryanofsky]

Added in e4a7662.

[morcos]

i'd abandon bumpid here so it doesn't accidentally rereplace rbfid if its reaccepted from the wallet

if later abandoning conflicts/replacements frees up the original to spend again then thats a change in behavior that can result in changing the test

[ryanofsky]

Added abandontransaction call in b0d4f93.

[morcos]

ACK modulo comments above.

[jtimon]

Concept ACK, needs rebase.

[ryanofsky]

Rebased to handle named arguments.

[morcos]

This should take place after submit_block_with_tx and before invalidateblock
That is the only time it is guaranteed that bumpid is not in the mempool.

[ryanofsky]

Oops, yes, I got confused about what the test was doing with the two transactions. Fixed and updated the comment in a1046e0.

[gmaxwell]

I believe this should make the new transaction non-RBF replaceable. (1) we currently won't replace it, (2) if someone made an original payment and the receiver is squaking because it's replaceable, a mechanism is needed to issue a non-replacable version. Since we won't replace it, we can do this in one step. Does this make sense?

[gmaxwell]

Hm. I expected this to change the walletrbf default.

[gmaxwell]

I can't see where it checks if the wallet is unlocked. Am I missing it?

[morcos]

yikes that seems like a pretty big oversight

[ryanofsky]

Added EnsureWalletIsUnlocked call in 59e425a

[laanwj]

Hm. I expected this to change the walletrbf default.

I don't think we should change that last-minute for 0.14 (only mentioning that because this is tagged 0.14).

Edit: Concept ACK otherwise.

[ryanofsky]

Created #9527 for changing the walletrbf default.

[ryanofsky]

Added replaceable option in 8d3cd28, to allow users to create replacement transactions that are not replaceable.

I ran into a lot of spurious errors related to fundrawtransaction in the bumpfee tests while trying to add a simple test for this. fundrawtransaction would sometimes chose different sized inputs than the small_output_fails and dust_to_fee tests were expecting, so I rewrote these tests in 13960f8 to manually create RBF transactions with the right inputs.

I also ran into problems with fundrawtransaction using up all the right-sized inputs before these tests could use them, so I changed the order of the tests so they would run before any fundrawtransaction calls.

I also ran into problems with fundrawtransaction sometimes chosing a higher fee for the RBF transaction than the test_rebumping test expected, causing the bumpfee command in the test to fail with an "Invalid totalFee, must be at least ..." error. I did not figure out what was causing this, but it seemed to somehow be related to the number of peer_node.sendtoaddress(rbf_node_address, 0.001) calls made during the test setup.

[sdaftuar]

re-ACK 8d3cd28

[morcos]

tested ACK 8d3cd28

[ryanofsky]

Squashed 8d3cd28 -> 1d71fd0

[instagibbs]

tACK with random nits. If you were to make one change I think printing out the necessary flat fee on rejection(for whatever reason) would be a solid improvement.

[instagibbs]

nit: maybe name the relay fee constant in case user wants to look it up.

[ryanofsky]

Added in 7c9ad69

[instagibbs]

nit: add n here as well as below

[ryanofsky]

Options style here is the same as the style used in fundrawtransaction (and it seems clearer without all the n's and extra punctuation).

[instagibbs]

can you explain why user-chosen feerate isn't an option, but absolute fee?

[ryanofsky]

User chosen feerate is possible with settxfee, and could be added as another option here in the future. Absolute fee is a reasonable thing a user might want to set, probably determining it from the fee of the previous transaction.

[instagibbs]

nit: or full RBF nodes relay it

[ryanofsky]

Referring to nodes that don't follow BIP-125? Maybe a more general note about these nodes would be helpful, but it seems like an odd thing to mention only in this context.

[luke-jr]

"(though it may still be replacable in practice)" seems sufficient.

[instagibbs]

nit: you already check that the wallet is available above, I believe unneeded.

[ryanofsky]

Removed in 7c9ad69

[instagibbs]

if this is the intended user flow, we might want to inform them via the following error message by including a totalfee guess alongside the minimum required feerate with a suggestion to use that option.

[ryanofsky]

Added in 7c9ad69

[instagibbs]

just set it

[morcos]

I guess I don't see how it makes a difference, but it seems to me that the whole point of this PR is to change the transaction to the least degree possible. I don't know why someone would have chosen to set their sequence to 0xffffffff, but if they did, why should we change it for them here?

[luke-jr]

Agreed with @morcos, leave this as-is.

[morcos]

why does it need to be in the mempool?

[instagibbs]

(weird, comments are in wrong order) it doesn't you're right.

[luke-jr]

It says in the wallet, not the mempool...?

[TheBlueMatt]

Only reviewed parts of the wallet.cpp stuff just yet, but figured I'd post it now.

[TheBlueMatt]

I'm super not convinced that this is sufficient. See all the places where isAbandoned is checked - we probably need to check for replacement in most, if not all, of the same places.

eg probably want to check in RelayWalletTransaction and possibly isSpent?

[ryanofsky]

This is being set specifically to change the behavior of AvailableCoins to avoid creating new transactions which spend outputs of rbf & bumpfee transactions, for reasons described in comments in AvailableCoins.

I'm actually not sure why we don't mark the transaction that is being replaced to be abandoned. But I don't think it should make any practical difference right now.

RelayWalletTransaction in the normal case should already skip this transaction because it conflicts with the bumpfee transaction and won't be in mempool.

IsSpent could perhaps be written to not consider inputs of a transaction that's been replaced to be spent, though in practice it doesn't make a difference because the bumpfee replacing transaction always spends the same inputs as the replaced transaction.

[sdaftuar]

Fee bumped should not imply abandoned -- it's entirely possible the original version of the transaction will confirm.

Changing IsSpent as you suggest ought to have no visible effect (since bumpfee spends all the same inputs, currently), yet I think it would be incorrect to make your change -- if we add smarter fee bumping behavior in the future, and it becomes possible to drop an input from the original tx in a replacement transaction, we would still want to treat the original input as spent until the transaction that spends it becomes conflicted (or the user explicitly abandons it).

In RelayWalletTransaction, I think it's more debatable whether to attempt relay of the original version, but since it would only succeed if somehow the feebumped version dropped out of your mempool, I think it's reasonable for the wallet to attempt to relay unless the user explicitly calls abandontransaction on it. We don't care which version gets mined, and if something is subtly wrong with the bumped transaction then we might as well attempt the original. The effect on tx relay bandwidth should be negligible.

Also -- I believe this is no different behavior than if we have conflicting transactions in our wallet that originate via some means other than bumpfee. For instance if someone sends funds to us and then fee bumps it themselves, we'll wind up with both transactions in our wallet, and we'll attempt to relay both, which I think is correct behavior.

[morcos]

100% agree with sdaftuar. it would be incorrect to auto abandon the bumpee.

[TheBlueMatt]

Abandoned doesn't imply the original version of the transaction won't confirm, it just means "pretend this isnt here, unless it somehow gets confirmed"?

But, ok, fair point regarding IsSpent - I'm not convinced thats is definitely The Right Behavior to not allow a user to spend something they removed from their transaction, but this is the more conservative option, so have no problem with it for now.

re: RelayWalletTransaction: I think this absolutely should have a replacement check...if a transaction has been replaced it shouldn't make it back into our mempool without the other transaction also replacing it, sure, but belt-and-suspenders. Plus a user could, theoretically, restart with a higher min relay fee (or upgrade to a new version), and this could break.

[TheBlueMatt]

I'll withdraw my request after IRC discussion. I've been convinced that in some use-cases a user might prefer us to accept the old version of a transaction which has been bumped, so no need to change that.

[ryanofsky]

Long discussion in IRC about this beginning here: https://botbot.me/freenode/bitcoin-core-dev/msg/79332875/.

I think the conclusion is that it is preferable to keep the current code which attempts to add all replaced and replacing transactions to the mempool , because there are cases when there is a problem with the replacing tx (e.g. output too close to dust limit) that would cause it to get rejected from mempool, and where it would be preferable to relay the original replaced transaction.

Also, the risk of the wallet relaying both replaced and replaced transactions to peers is very low. It will not happen unless a CWallet::ResendWalletTransactions call somehow is made before the initial CWallet::ReacceptWalletTransactions call in CWallet::postInitProcess finishes.

[TheBlueMatt]

Hum, should we be ignoring the return value here? (We check it in some places, ignore it in others in wallet.cpp, but it seems wrong to ignore it).

[ryanofsky]

Will add a check.

[sdaftuar]

I think we can just clean this up in a subsequent PR? I agree that the differing behaviors are not good, but perhaps this is a small enough issue to not hold this up...

[ryanofsky]

Either way seems fine. Added a check and error print for now in f9c4007.

[TheBlueMatt]

I only brought it up because I'd prefer new code to check it...it seems bad to not do so.

[ryanofsky]

Agreed, check is added in f9c4007.

[TheBlueMatt]

Do we need/want a MarkDirty here, too? I think we might not need it, but it seems strange to not have it here when we have it in all the other similar things (conflicted, abandon, etc).

[ryanofsky]

Will look more, but I would want to find a specific case it is needed before adding MarkDirty here. I have another PR #9381 which eliminates MarkDirty from AddToWallet() and will get rid of this entire MarkReplaced() method, replacing it with an AddToWallet call.

[sdaftuar]

I don't think we should treat this differently from any other unconfirmed, conflicting transaction entering the wallet, so I don't believe this is necessary (and it seems confusing to me to add calls like that in places where we think that balances shouldn't be changing).

[TheBlueMatt]

Obviously prefer to leave this in because belt-and-suspenders, but shouldn't this check be redundant with the !InMempool() check, two checks above?

[ryanofsky]

It's not completely redundant (see https://github.com/mrbandrews/bitcoin/blob/1d71fd0dab96430643b818ea993acea2aaa09256/qa/rpc-tests/bumpfee.py#L251 for the test which triggers this condition), but this is checking for a really esoteric case. See description above for all the gory details, but in this can happen when the replaced transaction A temporarily becomes part of a block instead of the replacing transaction B, and then gets added back to the mempool when there's a reorg.

[luke-jr]

Almost there... (if you could use braces more, that'd be nice, but not critical)

[luke-jr]

>3 now... but I don't know that we need an explicit check here. Seems like asking for bugs.

[ryanofsky]

Makes sense, removed in 23c389a.

[luke-jr]

Should be 1 now (as of #9239). Maybe make a const int in some header for this number...

[morcos]

nothing bad happens if you call it with 1, i think it makes most sense to allow users to select 1 meaning as fast as possible, and then we just return the fastest estimate we're comfortable with

[luke-jr]

Okay

[luke-jr]

get_int64

[ryanofsky]

Thanks, fixed in 23c389a.

[luke-jr]

Prefer to just do:

if (options.exists("replaceable")) {
    replaceable = options["replaceable"].get_bool();
}

So that if the default changes, this still works right.

[ryanofsky]

Good point, fixed in 23c389a.

[luke-jr]

So long as the user is explicitly setting a total fee, we should either fail when discarding dust, or at least document this behaviour in the RPC help.

[ryanofsky]

Added note to totalFee documentation in 4e1a040.

[luke-jr]

Agreed with @morcos, leave this as-is.

[luke-jr]

Maybe just add "errors" to the response below?

[ryanofsky]

Clarified the error message in 23c389a. If there are other examples of returning an "errors" key in a RPC response, I could change this to conform. But my preference would be to keep the exception (since this would be a sign of a serious problem that should not be ignored) and to just return more complete status information with the exception.

[luke-jr]

I would worry that a caller might ignore the error and try again, but I guess the harm risk of that is minimal with bumpfee.

[ryanofsky]

Implemented suggestions from @luke-jr in 23c389a, adding braces to all if statements, and making most of the other requested changes.

[ryanofsky]

morcos was responding to a review comment "painfully obvious, but needs to be in mempool as well :)" that is gone now.

[ryanofsky]

Reworded, adding "for example" in 23c389a. Just saying non-replaceable transactions are replaceable sounds mysterious. Maybe I'm less knowledgeable than the target user, but at least I would find it confusing.

[ryanofsky]

Makes sense, removed in 23c389a.

[ryanofsky]

Thanks, fixed in 23c389a.

[ryanofsky]

Good point, fixed in 23c389a.

[ryanofsky]

Added note to totalFee documentation in 4e1a040.

[ryanofsky]

Ok, keeping as is to change the transaction as little as possible.

[ryanofsky]

Clarified the error message in 23c389a. If there are other examples of returning an "errors" key in a RPC response, I could change this to conform. But my preference would be to keep the exception (since this would be a sign of a serious problem that should not be ignored) and to just return more complete status information with the exception.

[TheBlueMatt]

I know other ways of calling getbalance are pretty broken (#8183), but I think we need to fix a bit of how bumpfee interacts with getbalance "*". In testing I got getbalance "*" to give me a negative number, which seems super shitty.

Additionally, we need documentation on how bumpfee interacts with listunspent. While I dont mind the change outputs from bumpfee not appearing in the results, this is likely to be surprising to users (I believe this only otherwise occurs if the transaction was abandoned), so needs documented.

[morcos]

I'd vote against making this statement.
I think it should be a more general principle of our wallet that you might always lose unexpectedly something on the order of the dust threshold, rather than explicitly mention it every time we think it could happen. But if I'm outvoted, so be it..

[morcos]

@TheBlueMatt I don't see how this PR has any affect on any of the getbalance calls. I think what you are seeing is pre-existing misbehavior for multiple spends of the same outputs being present in the wallet.

I went through all the calls to AvailableCoins (which is the only thing this PR changes) and I think the only open decision is deciding what we want the interaction with listunspent to be and documenting it.

[TheBlueMatt]

I think if you have specified a manual confTarget you probably want to always take this branch.

[ryanofsky]

Done in a29b3f5.

[TheBlueMatt]

I believe this and the check further down should have a fOnlyConfirmed && at the beginning to avoid not showing the tx currently in mempool's change output in listunspent, which I think would be the correct behavior.

[TheBlueMatt]

Or maybe only the one below, but in either case should be well-documented.

[ryanofsky]

Added in 0f83aa3.

[instagibbs]

note: in the future we should rename and invert the AvailableCoins argument to this.

[ryanofsky]

Squashed 28fd457 -> ce3a363.

[TheBlueMatt]

utACK ce3a363

[morcos]

tested previous iteration ACK ce3a363

[luke-jr]

This seems redundant. Type-checking is the purpose of the .get_<type> methods on UniValue.

[TheBlueMatt]

We absolutely do not use .get_'s built-in type check anywhere - it throws a different type of exception which will not be correctly reported as an RPC_TYPE_ERROR.

[sdaftuar]

utACK ce3a363 (reviewed diff from last iteration I tested and ACKed above).

[luke-jr]

Looks okay. A few suggested changes, but not a big deal if they don't go in.

[luke-jr]

Prefer to add an options Object for this.

[ryanofsky]

Interesting. I wasn't following the named argument discussion in detail, but I thought named arguments would be preferable to options because they are easier to specify on the command line. But maybe there are advantages to options objects that I'm not aware of.

[luke-jr]

We can't be sure 1-block confirmed transactions won't have a replacement mined either...

[ryanofsky]

Changed wording in 4beb7c9.

[ryanofsky]

Squashed 4beb7c9 -> cc0243a

[morcos]

Still ACK cc0243a

[instagibbs]

tACK cc0243a

no need to change nit pre-merge, very minor

[instagibbs]

ultra-nit: "bumped" is a bit confusing since it's already used in another sense(bumped fee), "replace" is likely better.

[jameshilliard]

Should probably have an option to override this.

[ryanofsky]

An option to signal RBF in the original transaction (which already exists, see -walletrbf option, and #9592, #9527), or an option to create a bumpfee transaction even if the original doesn't signal RBF?

[jameshilliard]

An option to create a bumpfee transaction regardless of if the original signaled RBF since some nodes may accept it.

[ryanofsky]

There was some discussion about this idea in IRC https://botbot.me/freenode/bitcoin-core-dev/msg/79836432/ (unclear what the outcome was)