Make feebumper class stateless #10600

ryanofsky commented at 2:55 PM on June 15, 2017: member

Make feebumper methods static and remove stored state in the class.

Having the results of feebumper calls persist in an object makes process separation between Qt and wallet awkward, because it means the feebumper object either has to be serialized back and forth between Qt and wallet processes between fee bump calls, or that the feebumper object needs to stay alive in the wallet process with an object reference passed back to Qt. It's simpler just to have fee bumper calls return their results immediately instead of storing them in an object with an extended lifetime.

In addition to making feebumper methods static, also:

Move LOCK calls from Qt code to feebumper
Move TransactionCanBeBumped implementation from Qt code to feebumper
Rename CFeeBumper class to FeeBumper (every CFeeBumper reference had to be updated in this PR anyway so this doesn't increase the size of the diff)

This change was originally part of https://github.com/bitcoin/bitcoin/pull/10244

fanquake added the label Wallet on Jun 15, 2017

ryanofsky force-pushed on Jun 15, 2017

dcousens commented at 3:58 AM on June 16, 2017: contributor

Why bother with the class? Practically a namespace at this point?

ryanofsky commented at 10:02 AM on June 16, 2017: member

Why bother with the class? Practically a namespace at this point?

I don't see a problem with using a class as a namespace, but happy to change if you think it is in bad taste and have a different suggestion. Note that the files are called feebumper.h/cpp so if you want to replace the class with something else, it might involve file renames. This PR is really more concerned with simplifying Qt code and moving code that doesn't belong there out.

ryanofsky force-pushed on Jul 11, 2017

ryanofsky force-pushed on Jul 18, 2017

ryanofsky force-pushed on Aug 25, 2017

sipa commented at 8:06 PM on August 27, 2017: member

I agree that if all of a class's methods have become static it's better to turn it into a namespace of functions, but don't care strongly. If so, I don't think a file rename is necessary.

ryanofsky commented at 2:21 PM on August 28, 2017: member

I agree that if all of a class's methods have become static it's better to turn it into a namespace of functions, but don't care strongly. If so, I don't think a file rename is necessary.

Thanks @sipa, removed class without renaming file.

Added commit 1ea35974529622eb16fe5710bee6719dfc205c31 -> 47c5601d5f6a992d9c46d8300de248374d6eb066 (pr/ipc-bump.6 -> pr/ipc-bump.7, compare)

promag commented at 2:38 PM on August 28, 2017: member

because it means the feebumper object either has to be serialized back and forth between Qt and wallet processes between fee bump calls

That is a problem or inconvenient?

ryanofsky commented at 2:55 PM on August 28, 2017: member

because it means the feebumper object either has to be serialized back and forth between Qt and wallet processes between fee bump calls

That is a problem or inconvenient?

These should just be plain function calls because there are no advantages to having a stateful FeeBumper object. Making the feebumper class serializable is dumb because it will add new code that send extra data that isn't always needed.

in src/wallet/feebumper.cpp:73 in 47c5601d5f outdated

  81 | -    txid(std::move(txidIn)),
  82 | -    nOldFee(0),
  83 | -    nNewFee(0)
  84 | +bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid)
  85 |  {
  86 | +    LOCK2(cs_main, pWallet->cs_wallet);

promag commented at 9:25 PM on August 28, 2017:

It's in the original code, but is it necessary to lock cs_main? The same applies for other functions.

ryanofsky commented at 9:19 AM on August 29, 2017:

It's in the original code, but is it necessary to lock cs_main? The same applies for other functions.

Good catch! Fixed in 6a0131608e738f7fcf8ae9efb8ea54bf749ffe85. In the other places cs_main is required for preconditionChecks. Note that in #10973, I replace accesses to cs_main global throughout wallet code with ipc_locked locals, so it is immediately obvious where locking is and isn't required and mistakes like this will not happen in the future (see ipc_locked in https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep/src/wallet/feebumper.cpp).

in src/wallet/feebumper.h:33 in 47c5601d5f outdated

  62 | +
  63 | +/* create bumpfee transaction */
  64 | +BumpFeeResult CreateBumpFeeTransaction(const CWallet* pWallet,
  65 | +    const uint256& txid,
  66 | +    const CCoinControl& coin_control,
  67 | +    CAmount totalFee,

promag commented at 9:32 PM on August 28, 2017:

At least in the headers we could fix to snake_case, it doesn't make the patch bigger. WDYT?

ryanofsky commented at 9:46 AM on August 29, 2017:

At least in the headers we could fix to snake_case, it doesn't make the patch bigger. WDYT?

Done in db134b451b7c5871e12bf7ef947d7745446e96f4

in src/wallet/feebumper.h:26 in 47c5601d5f outdated

  55 | -    std::vector<std::string> vErrors;
  56 | -    BumpFeeResult currentResult;
  57 | -    CAmount nOldFee;
  58 | -    CAmount nNewFee;
  59 | -};
  60 | +/* return whether transaction can be bumped */

promag commented at 9:33 PM on August 28, 2017:

Use doxygen compatible comments as per developer notes?

ryanofsky commented at 9:46 AM on August 29, 2017:

Use doxygen compatible comments as per developer notes?

Done in db134b451b7c5871e12bf7ef947d7745446e96f4

in src/wallet/feebumper.h:30 in 47c5601d5f outdated

  59 | -};
  60 | +/* return whether transaction can be bumped */
  61 | +bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid);
  62 | +
  63 | +/* create bumpfee transaction */
  64 | +BumpFeeResult CreateBumpFeeTransaction(const CWallet* pWallet,

promag commented at 9:36 PM on August 28, 2017:

One line?

ryanofsky commented at 9:44 AM on August 29, 2017:

One line?

I use clang-format for everything, and avoid manual formatting, so that's where this comes from. Would prefer not to change unless someone thinks this is a problem.

ryanofsky referenced this in commit 6a0131608e on Aug 29, 2017

ryanofsky referenced this in commit db134b451b on Aug 29, 2017

ryanofsky commented at 10:00 AM on August 29, 2017: member

Added 2 commits 47c5601d5f6a992d9c46d8300de248374d6eb066 -> db134b451b7c5871e12bf7ef947d7745446e96f4 (pr/ipc-bump.7 -> pr/ipc-bump.8, compare)

dcousens approved

dcousens commented at 4:00 AM on September 5, 2017: contributor

light utACK

in src/wallet/feebumper.cpp:73 in 6a0131608e outdated

  69 | @@ -70,7 +70,7 @@ BumpFeeResult PreconditionChecks(const CWallet* pWallet, const CWalletTx& wtx, s
  70 |  
  71 |  bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid)
  72 |  {
  73 | -    LOCK2(cs_main, pWallet->cs_wallet);
  74 | +    LOCK(pWallet->cs_wallet);

TheBlueMatt commented at 11:00 PM on September 6, 2017:

Can you document the exact locking requirements in GetWalletTx in wallet.h or wallet.cpp just to make sure someone doesnt do some crazy-ass change in the future and blow up the lockorder?

ryanofsky commented at 11:43 PM on September 6, 2017:

Can you document the exact locking requirements in GetWalletTx in wallet.h or wallet.cpp just to make sure someone doesnt do some crazy-ass change in the future and blow up the lockorder?

Done in 3a7c0dfafbd3b8fc1e32f40d98a7614bc190635d. I could go further and assert cs_wallet lock is held in GetWalletTx, though that is getting pretty far afield. Could open a separate PR.

in src/wallet/feebumper.h:31 in db134b451b outdated

  34 |      const CCoinControl& coin_control,
  35 | -    CAmount totalFee,
  36 | -    std::vector<std::string>& vErrors,
  37 | -    CAmount& nOldFee,
  38 | -    CAmount& nNewFee,
  39 | +    CAmount total_fee,

TheBlueMatt commented at 11:03 PM on September 6, 2017:

Yes, please do this, but also please keep names consistent between the cpp and h.

ryanofsky commented at 11:43 PM on September 6, 2017:

Done in 52301dbe0ee05c6bb8b5e187d7d5384a7f7bef5c

TheBlueMatt commented at 11:04 PM on September 6, 2017: member

Hmm, yea, I think I'd prefer leaving the BumpFee:: functions in a namespace. I find BumpFee::* to be cleaner than *BumpFee.

ryanofsky commented at 12:36 AM on September 7, 2017: member

Added 3 commits db134b451b7c5871e12bf7ef947d7745446e96f4 -> 2b2158ae5e21927f332c06ed72239bd2f4392dd9 (pr/ipc-bump.8 -> pr/ipc-bump.9, compare)

Hmm, yea, I think I'd prefer leaving the BumpFee:: functions in a namespace. I find BumpFee::* to be cleaner than *BumpFee.

Added in 2b2158ae5e21927f332c06ed72239bd2f4392dd9

ryanofsky referenced this in commit 52301dbe0e on Sep 7, 2017

ryanofsky referenced this in commit 3a7c0dfafb on Sep 7, 2017

ryanofsky referenced this in commit 2b2158ae5e on Sep 7, 2017

TheBlueMatt commented at 8:30 PM on September 7, 2017: member

utACK 2b2158ae5e21927f332c06ed72239bd2f4392dd9

jnewbery commented at 8:04 PM on September 19, 2017: member

I like this, but the commits are structured in a way that makes it more difficult to review than necessary. I think you can remove the first commit that changes all the functions to static (since you're removing the class entirely in a later commit). I also think some of the intermediate commits could perhaps be squashed, although I haven't looked to closely at them.

If you're not interested in maintaining this branch, I can take it.

ryanofsky force-pushed on Sep 20, 2017

ryanofsky commented at 9:09 PM on September 20, 2017: member

Reset 2b2158ae5e21927f332c06ed72239bd2f4392dd9 -> 510e19c8c9d3c3b33a37dc601835f264df0c6685 (pr/ipc-bump.9 -> pr/ipc-bump.10)

New branch was written by @jnewbery. Has several changes:

Simplifies history so related changes are better grouped together.
Rebased on master
Reverts some unrelated changes suggested by previous reviewers which could go in separate PRs.
Other minor changes like declaring functions static instead of using an anonymous namespace.

ryanofsky force-pushed on Sep 20, 2017

ryanofsky commented at 9:30 PM on September 20, 2017: member

Updated 510e19c8c9d3c3b33a37dc601835f264df0c6685 -> 2db9e0a3d123dd7c7f5eb74fdefdb5dba189055b (pr/ipc-bump.10 -> pr/ipc-bump.11)

Another update from @jnewbery, adding to TransactionCanBeBumped to the namespace.

jnewbery commented at 9:37 PM on September 20, 2017: member

Tested ACK 2db9e0a3d123dd7c7f5eb74fdefdb5dba189055b. This should be trivially reviewable now. The first two commits in particular are just mechanical changes. The third commit changes the feebumper from a stateful class to stateless function calls, but the code change is minimal (the only real change is the way that errors are propagated back to the caller). @promag - I've left out your suggested simplification to the locking, since I think that can be done in a follow-up PR.

MarcoFalke commented at 7:25 PM on November 10, 2017: member

Needs rebase

[trivial] Rename feebumper variables according to project code style

Future PRs will completely refactor this translation unit and touch all
this code so we rename the variables to follow project stlye guidelines
in this preparation commit.

Don't use m_ prefixes for member variables since we're going to remove
the class entirely in the next commits.

7c4f009195

[refactor] Make feebumper namespace

Future commit will remove the FeeBumper class. This commit simply places
everything into a feebumper namespace, and changes the enum class name
from BumpeFeeResult to feebumper::Result.

37bdcca3c3

[wallet] Change feebumper from class to functions

Change feebumper from a stateful class into a namespace of stateless
functions.

Having the results of feebumper calls persist in an object makes process
separation between Qt and wallet awkward, because it means the feebumper object
either has to be serialized back and forth between Qt and wallet processes
between fee bump calls, or that the feebumper object needs to stay alive in the
wallet process with an object reference passed back to Qt. It's simpler just to
have fee bumper calls return their results immediately instead of storing them
in an object with an extended lifetime.

In addition to making feebumper stateless, also:

- Move LOCK calls from Qt code to feebumper
- Move TransactionCanBeBumped implementation from Qt code to feebumper

aed1d90aca

jnewbery force-pushed on Nov 10, 2017

jnewbery commented at 10:20 PM on November 10, 2017: member

rebased

ryanofsky commented at 3:58 PM on November 15, 2017: member

utACK aed1d90aca81c20c6e982ad567291f3812d47c8f. Only change since last review was the rebase.

in src/qt/walletmodel.cpp:717 in aed1d90aca

 723 | -    {
 724 | -        LOCK2(cs_main, wallet->cs_wallet);
 725 | -        res = feeBump->commit(wallet);
 726 | -    }
 727 | -    if(!res) {
 728 | +    uint256 txid;

MarcoFalke commented at 5:20 PM on November 15, 2017:

nit: txid_dummy?

in src/qt/walletmodel.cpp:673 in aed1d90aca

 680 | +    coin_control.signalRbf = true;
 681 | +    std::vector<std::string> errors;
 682 | +    CAmount old_fee;
 683 | +    CAmount new_fee;
 684 | +    CMutableTransaction mtx;
 685 | +    if (feebumper::CreateTransaction(wallet, hash, coin_control, 0 /* totalFee */, errors, old_fee, new_fee, mtx) != feebumper::Result::OK) {

MarcoFalke commented at 5:33 PM on November 15, 2017:

nit: total_fee

MarcoFalke commented at 5:49 PM on November 15, 2017: member

utACK aed1d90aca81c20c6e982ad567291f3812d47c8f (Also checked that 7c4f0091957d305679546fde592ffa2de2d186fe produces the same binaries on my arch, since it is not a scripted diff.)

MarcoFalke merged this on Nov 15, 2017

MarcoFalke closed this on Nov 15, 2017

MarcoFalke referenced this in commit 4ed818060e on Nov 15, 2017

MarcoFalke locked this on Sep 8, 2021