Is there some reason not to use exceptions?

I though about using exceptions but came to the conclusion that a simple result state enum will (probably) allow more flexibility for further optimisations. But no strong opinion.

could use "enum class" here to avoid the "BumpFeeResult_" prefixes here while still requiring them where the values are referenced.

It appears that you are only copying, not moving the CalculateMaximumSignedTxSize function in this commit (8993411d7b3e6fe2c4baa917874941ed95c39188 [Move] move bumpfee core logic to CWallet). Would make more sense to remove it from rpcwallet.cpp in this commit instead of the next one.

Enum is not being used in this commit (8993411d7b3e6fe2c4baa917874941ed95c39188 [Move] move bumpfee core logic to CWallet), would probably make more sense to define in the next commit.

Can drop "enum" in c++.

I don't think it's a good idea to return a mutable pointer to a copy of the CWalletTx that isn't actually in mapwallet, because changes a caller makes to this pointer might have unexpected or no effects. Since the shared pointer doesn't appear to be needed here or in #9697, I think it'd be better to return just wtxBumped.tx or wtxBumped.GetHash() instead.

Don't see a reason to require shared_ptr here. CMutableTransaction& would allow callers to use it or not use it. Similarly for other shared_ptr arguments.

I don't think there's a reason to have these typedefs or even to use shared pointers in feebumper at all. Shared pointers are a great tool in places where they are actually needed, but plain c++ references that don't need to be deferenced, don't require reference counting, and can't be null are a better option in the places where they work. Another disadvantage of shared_ptr interfaces is that the requirement to wrap everything in shared_ptr can start spreading virally from callers, to callers callers, etc.

I think the less verbose BumpFeeResult::OK would be preferable to CFeeBumper::BumpFeeResult::OK

Could move enum above class to accomplish this. Or could change "enum class" to "enum" for CFeeBumper::OK.

Seeing all these getters makes me think this API grouping is not a good idea. We have this class with two functions that do real work (constructor and commit), and then a bunch of members, and a bunch of getters, and somehow the caller is supposed to keep track of all the getters and know which ones are valid to call at different stages in the object lifetime. Also, member lifetimes are unnecessarily prolonged (constructor's return values like nOldFee/nNewFee/currentResult/vErrors do not need to be kept alive in order for the commit method to do it's job).

I think a better API here would look something like:

void CreateBumpTx(<input const ref args>, <output ref args>);
void CommitBumpTx(<input const ref args>, <output ref args>);

without the stateful class.

Maybe split into two asserts, to be a little easier to read, and so you can tell which condition failed from the error.

There is a minor bug here. This needs to pass pWallet into CalculateMaximumSignedTxSize so it can stop using pwalletMain. When making this change, I also had to make the CWallet::DummySignTx method const. Feel free to use my complete fix: ryanofsky@731ca5c

This appears to be WALLET_ERROR not MISC_ERROR in master:

Thanks. Great catch... fixed now.

pwalletMain should be pwallet, also a few places below

In commit "Bumpfee move request parameter interaction to the top":

This commit seems to be changing behavior in a way that I think would be good to avoid. Previously, if you passed a negative or 0 totalFee option, you would get an "Insufficient totalFee" error. But now, an automatic fee will be calculated, ignoring the totalFee value that was passed.

I think you could fix this by writing:

totalFee = options["totalFee"].get_int64();
if (totalFee <= 0) {
    throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee %s (must be greater than 0)", FormatMoney(totalFee)));
}

Yes. That's correct. I'm adding a commit that restores that behaviour.

I'd prefer this not to be an assert. Is it possible to return an error rather than assert?

I know that this was here before the code move, but my worry is that moving it to wallet.cpp might encourage people to call this function without knowing that they must lock the wallet and verify that all the transaction inputs are in the wallet map first.

Right. This is a valid point. Adding a commit that returns false instead of the assert().

I'm not sure if you need to check this here, when you already checked it in CFeeBumper::CFeeBumper()?

If this check fails, I think you need to return false from this function immediately rather than fall through, since the function goes on to deference the transaction in the map.

I think it's sane to keep this check... but your right, it should immediately return. Will fix.

I don't think you need to change the signature of CalculateMaximumSignedTxSize, and the prior version seems better

Indeed. This seems to be a rebase issue. Will change.

make_pair with emplace_back is not necessary, may as well fix it.

might make more sense for CalculateMaximumSignexTxSize to take a vErrors argument, as this is non-obvious to me (from local code) why maxNewTxSize < 0 means there were unsignable inputs.

Maybe, though the scope of this PR is – ideally – pure refactoring to make it usable over the UI. Let's optimise such things later.

Reason to change this line from CWallet::GetRequiredFee?

This is also static, please keep CWallet::

This is also static, please keep CWallet::

Yes. This seems to be again a rebase issue. Though, changing it to pWallet would allow to be more flexible when multi wallet takes off... but changing it back for now.

Maybe you should refactor this to:

 if (nOldFee >= nNewFee) {
    vErrors.push_back("Attempt to increase fee decreased fee.");
    currentResult = BumpFeeResult::WALLET_ERROR;     
    return;
}

Let's do this later and focus now on refactoring out the core functionality.

I think it makes sense to just change it now... #8456 (comment) is where it got introduced, I think it still may be possible to trigger & throwing seems better for this than crashing your node.

I'll leave it up to you though -- just let me know if you decide not to & I'll prepare a patch.

Let's try to keep the scope ("refactoring"), behaviours changes have already sneaked into this PR.

You should just make this a case of the switch.

Maybe you should fail to commit if (!vErrors.empty())?

Good point. Will change.

I think I would prefer to see commit be done via RAII (but maybe that's bad if you make a container of CFeeBumpers...)

Hmm.. not sure if RAII for a bump commit is appropriate. IMO the commit (it actually does sign and broadcast) should be a clearly visible call.

What if you do something like

commit() {
    assert(!comitted)
    comitted = true;
     ...
}
~CFeeBumper() {
    if (vErrors.empty())
    assert(comitted);
}

To ensure that if a CFeeBump could be comitted, it is & comitting isn't tried multiple times?

Not sure but this looks after too much hand holding. But if someone wants to implement this, yeah, maybe after this PR?

In commit "Restore CalculateMaximumSignedTxSize function signature":

Previous signature took a CWallet reference instead of a pointer. (Just an observation, feel free to leave it.)

In commit "Use static calls for GetRequiredFee and GetMinimumFee, remove make_pair from emplace_back"

Could drop parens around mi->second. (Just an observation, feel free to leave.)

vErrors.clear() is now redundant.

I think it makes sense to just change it now... #8456 (comment) is where it got introduced, I think it still may be possible to trigger & throwing seems better for this than crashing your node.

I'll leave it up to you though -- just let me know if you decide not to & I'll prepare a patch.

What if you do something like

commit() {
    assert(!comitted)
    comitted = true;
     ...
}
~CFeeBumper() {
    if (vErrors.empty())
    assert(comitted);
}

To ensure that if a CFeeBump could be comitted, it is & comitting isn't tried multiple times?