Make feebumper class stateless #10600

ryanofsky · 2017-06-15T14:55:50Z

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 #10244

dcousens · 2017-06-16T03:58:29Z

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

ryanofsky · 2017-06-16T10:02:13Z

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.

sipa · 2017-08-27T20:06:19Z

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 · 2017-08-28T14:21:28Z

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 1ea3597 -> 47c5601 (pr/ipc-bump.6 -> pr/ipc-bump.7, compare)

promag · 2017-08-28T14:38:12Z

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 · 2017-08-28T14:55:39Z

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.

promag · 2017-08-28T21:25:48Z

src/wallet/feebumper.cpp

 {
+    LOCK2(cs_main, pWallet->cs_wallet);


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

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

Good catch! Fixed in 6a01316. 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).

promag · 2017-08-28T21:32:47Z

src/wallet/feebumper.h

+BumpFeeResult CreateBumpFeeTransaction(const CWallet* pWallet,
+    const uint256& txid,
+    const CCoinControl& coin_control,
+    CAmount totalFee,


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

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

Done in db134b4

promag · 2017-08-28T21:33:40Z

src/wallet/feebumper.h

-    CAmount nOldFee;
-    CAmount nNewFee;
-};
+/* return whether transaction can be bumped */


Use doxygen compatible comments as per developer notes?

Use doxygen compatible comments as per developer notes?

Done in db134b4

promag · 2017-08-28T21:36:01Z

src/wallet/feebumper.h

+bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid);
+
+/* create bumpfee transaction */
+BumpFeeResult CreateBumpFeeTransaction(const CWallet* pWallet,


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.

Suggested by João Barbosa <joao.paulo.barbosa@gmail.com> in bitcoin#10600 (comment)

Switch to doxgygen comments and snake_case as suggested by João Barbosa <joao.paulo.barbosa@gmail.com> in bitcoin#10600 (comment) and bitcoin#10600 (comment). Other minor edits.

ryanofsky

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

ryanofsky · 2017-08-29T09:19:24Z

src/wallet/feebumper.cpp

 {
+    LOCK2(cs_main, pWallet->cs_wallet);


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

Good catch! Fixed in 6a01316. 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).

ryanofsky · 2017-08-29T09:44:31Z

src/wallet/feebumper.h

+bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid);
+
+/* create bumpfee transaction */
+BumpFeeResult CreateBumpFeeTransaction(const CWallet* pWallet,


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 · 2017-08-29T09:46:04Z

src/wallet/feebumper.h

-    CAmount nOldFee;
-    CAmount nNewFee;
-};
+/* return whether transaction can be bumped */


Use doxygen compatible comments as per developer notes?

Done in db134b4

ryanofsky · 2017-08-29T09:46:17Z

src/wallet/feebumper.h

+BumpFeeResult CreateBumpFeeTransaction(const CWallet* pWallet,
+    const uint256& txid,
+    const CCoinControl& coin_control,
+    CAmount totalFee,


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

Done in db134b4

dcousens

light utACK

TheBlueMatt

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

TheBlueMatt · 2017-09-06T23:00:17Z

src/wallet/feebumper.cpp

@@ -70,7 +70,7 @@ BumpFeeResult PreconditionChecks(const CWallet* pWallet, const CWalletTx& wtx, s

 bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid)
 {
-    LOCK2(cs_main, pWallet->cs_wallet);
+    LOCK(pWallet->cs_wallet);


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?

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 3a7c0df. 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.

TheBlueMatt · 2017-09-06T23:03:29Z

src/wallet/feebumper.h

-    std::vector<std::string>& vErrors,
-    CAmount& nOldFee,
-    CAmount& nNewFee,
+    CAmount total_fee,


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

Done in 52301db

ryanofsky

Added 3 commits db134b4 -> 2b2158a (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 2b2158a

ryanofsky · 2017-09-06T23:43:20Z

src/wallet/feebumper.h

-    std::vector<std::string>& vErrors,
-    CAmount& nOldFee,
-    CAmount& nNewFee,
+    CAmount total_fee,


Done in 52301db

ryanofsky · 2017-09-06T23:43:49Z

src/wallet/feebumper.cpp

@@ -70,7 +70,7 @@ BumpFeeResult PreconditionChecks(const CWallet* pWallet, const CWalletTx& wtx, s

 bool TransactionCanBeBumped(CWallet* pWallet, const uint256& txid)
 {
-    LOCK2(cs_main, pWallet->cs_wallet);
+    LOCK(pWallet->cs_wallet);


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 3a7c0df. 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.

Suggested by Matt Corallo bitcoin#10600 (comment)

Suggested by Matt Corallo bitcoin#10600 (review)

TheBlueMatt · 2017-09-07T20:30:24Z

utACK 2b2158a

jnewbery · 2017-09-19T20:04:47Z

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 · 2017-09-20T21:09:13Z

Reset 2b2158a -> 510e19c (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 · 2017-09-20T21:30:53Z

Updated 510e19c -> 2db9e0a (pr/ipc-bump.10 -> pr/ipc-bump.11)

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

jnewbery · 2017-09-20T21:37:51Z

Tested ACK 2db9e0a. 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.

maflcko · 2017-11-10T19:25:36Z

Needs rebase

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.

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.

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

jnewbery · 2017-11-10T22:20:58Z

rebased

ryanofsky

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

maflcko

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

maflcko · 2017-11-15T17:20:09Z

src/qt/walletmodel.cpp

-        res = feeBump->commit(wallet);
-    }
-    if(!res) {
+    uint256 txid;


nit: txid_dummy?

maflcko · 2017-11-15T17:33:15Z

src/qt/walletmodel.cpp

+    CAmount old_fee;
+    CAmount new_fee;
+    CMutableTransaction mtx;
+    if (feebumper::CreateTransaction(wallet, hash, coin_control, 0 /* totalFee */, errors, old_fee, new_fee, mtx) != feebumper::Result::OK) {


nit: total_fee

aed1d90 [wallet] Change feebumper from class to functions (Russell Yanofsky) 37bdcca [refactor] Make feebumper namespace (Russell Yanofsky) 7c4f009 [trivial] Rename feebumper variables according to project code style (Russell Yanofsky) Pull request description: 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 #10244 Tree-SHA512: bf75e0c741b4e9c8912e66cc1dedf0ff715f77ea65fc33f7020d97d9099b0f6448f5852236dac63eea649de7d6fc03b0b21492e2c5140fb7560a39cf085506fd

fanquake added the Wallet label Jun 15, 2017

ryanofsky force-pushed the pr/ipc-bump branch from e7280bf to 3a43bb9 Compare June 15, 2017 19:52

ryanofsky mentioned this pull request Jun 15, 2017

Refactor: separate gui from wallet and node #10244

Merged

ryanofsky mentioned this pull request Jun 19, 2017

Multiprocess bitcoin #10102

Draft

ryanofsky force-pushed the pr/ipc-bump branch 2 times, most recently from 30d2100 to b937494 Compare July 18, 2017 19:49

ryanofsky force-pushed the pr/ipc-bump branch 2 times, most recently from 893b4c4 to 1ea3597 Compare August 25, 2017 22:42

promag reviewed Aug 28, 2017

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 29, 2017

Remove unnecessary LOCK(cs_main) in bumpfee

6a01316

Suggested by João Barbosa <joao.paulo.barbosa@gmail.com> in bitcoin#10600 (comment)

ryanofsky commented Aug 29, 2017

View reviewed changes

dcousens approved these changes Sep 5, 2017

View reviewed changes

TheBlueMatt reviewed Sep 6, 2017

View reviewed changes

ryanofsky commented Sep 7, 2017

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 7, 2017

Rename fee bumper arguments

52301db

Suggested by Matt Corallo bitcoin#10600 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 7, 2017

Document GetWalletTx locking requirement

3a7c0df

Suggested by Matt Corallo bitcoin#10600 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 7, 2017

Move bumpfee functions into namespace

2b2158a

Suggested by Matt Corallo bitcoin#10600 (review)

ryanofsky force-pushed the pr/ipc-bump branch from 2b2158a to 510e19c Compare September 20, 2017 21:06

ryanofsky force-pushed the pr/ipc-bump branch from 510e19c to 2db9e0a Compare September 20, 2017 21:29

ryanofsky added 3 commits November 10, 2017 17:01

[refactor] Make feebumper namespace

37bdcca

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.

jnewbery force-pushed the pr/ipc-bump branch from 2db9e0a to aed1d90 Compare November 10, 2017 22:20

ryanofsky commented Nov 15, 2017

View reviewed changes

maflcko reviewed Nov 15, 2017

View reviewed changes

maflcko merged commit aed1d90 into bitcoin:master Nov 15, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Make feebumper class stateless #10600

Make feebumper class stateless #10600

Uh oh!

Conversation

ryanofsky commented Jun 15, 2017

Uh oh!

dcousens commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jun 16, 2017

Uh oh!

sipa commented Aug 27, 2017

Uh oh!

ryanofsky commented Aug 28, 2017

Uh oh!

promag commented Aug 28, 2017

Uh oh!

ryanofsky commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 7, 2017

Uh oh!

jnewbery commented Sep 19, 2017

Uh oh!

ryanofsky commented Sep 20, 2017

Uh oh!

ryanofsky commented Sep 20, 2017

Uh oh!

jnewbery commented Sep 20, 2017

Uh oh!

maflcko commented Nov 10, 2017

Uh oh!

jnewbery commented Nov 10, 2017

dcousens commented Jun 16, 2017 •

edited

Loading

maflcko left a comment •

edited

Loading