Refactor Bumpfee, move core functionality to CWallet

jonasschnelli commented at 8:07 pm on February 3, 2017: contributor

This is a required step for using the current bumpfee logic in the GUI. The first two commits are “moveonly”.

jonasschnelli added the label Wallet on Feb 3, 2017

in src/wallet/wallet.h: in 45ec058131 outdated

795+        BumpFeeResult_INVALID_ADDRESS_OR_KEY,
796+        BumpFeeResult_INVALID_REQUEST,
797+        BumpFeeResult_INVALID_PARAMETER,
798+        BumpFeeResult_WALLET_ERROR,
799+        BumpFeeResult_MISC_ERROR,
800+    };

luke-jr commented at 8:09 pm on February 3, 2017:

Is there some reason not to use exceptions?

jonasschnelli commented at 8:12 pm on February 3, 2017:

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.

sipa commented at 8:13 pm on February 3, 2017: member

I prefer an enum here (I’ll elaborate if wanted).

laanwj commented at 1:26 pm on February 6, 2017: member

Concept ACK, it is better to have this in wallet than in the RPC code, especially as the code is going to be used by the GUI.

No opinion on how to handle the errors. Though returning an enum is consistent with some other wallet functions that return a status.

Aside: As said earlier on IRC I’m not entirely happy with the current trend of moving everything wallet related to CWallet - we’re creating a “god class”. It would be nice to see if we can parcel up the functionality and separate concerns, to avoid a huge file and exponentially growing complexity.

But in any case, this is a step forward…

in src/wallet/wallet.h: in 8993411d7b outdated

785@@ -786,6 +786,21 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
786     bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, bool keepReserveKey = true, const CTxDestination& destChange = CNoDestination());
787 
788     /**
789+     * Bumps the fee of an opt-in-RBF transaction <txid>,
790+     * replacing it with a new transaction <wtxNew>
791+     */
792+    enum BumpFeeResult
793+    {
794+        BumpFeeResult_OK,

ryanofsky commented at 8:56 pm on February 6, 2017:

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

in src/wallet/wallet.cpp: in 8993411d7b outdated

2296+// TODO: re-use this in CWallet::CreateTransaction (right now
2297+// CreateTransaction uses the constructed dummy-signed tx to do a priority
2298+// calculation, but we should be able to refactor after priority is removed).
2299+// NOTE: this requires that all inputs must be in mapWallet (eg the tx should
2300+// be IsAllFromMe).
2301+int64_t CalculateMaximumSignedTxSize(const CTransaction &tx)

ryanofsky commented at 9:01 pm on February 6, 2017:

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.

in src/wallet/wallet.h: in 8993411d7b outdated

785@@ -786,6 +786,21 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
786     bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, bool keepReserveKey = true, const CTxDestination& destChange = CNoDestination());
787 
788     /**
789+     * Bumps the fee of an opt-in-RBF transaction <txid>,
790+     * replacing it with a new transaction <wtxNew>
791+     */
792+    enum BumpFeeResult

ryanofsky commented at 9:11 pm on February 6, 2017:

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.

in src/wallet/wallet.cpp: in 45ec058131 outdated

2317+        return -1;
2318     }
2319     return GetVirtualTransactionSize(txNew);
2320 }
2321 
2322 enum CWallet::BumpFeeResult CWallet::BumpFee(uint256 txid, int newConfirmTarget, bool specifiedConfirmTarget, CAmount totalFee, bool newTxReplaceable, CAmount& nOldFee, CAmount& nNewFee, std::shared_ptr<CWalletTx>& wtxNew, std::vector<std::string>& vErrors)

ryanofsky commented at 9:17 pm on February 6, 2017:

Can drop “enum” in c++.

in src/wallet/wallet.cpp: in 45ec058131 outdated

2535         // wtxBumped to the caller even if marking the original transaction
2536         // replaced does not succeed for some reason.
2537         vErrors.push_back("Error: Created new bumpfee transaction but could not mark the original transaction as replaced.");
2538     }
2539+
2540+    wtxNew = std::make_shared<CWalletTx>(wtxBumped);

ryanofsky commented at 9:32 pm on February 6, 2017:

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.

ryanofsky commented at 9:39 pm on February 6, 2017: member

Looks good, just left a few minor comments. Hopefully this can be merged soon to avoid conflicts.

Agree with laanwj about the “god class” problem, but it seems straightforward to solve. BumpFee could easily be a standalone function taking a CWallet* pointer, instead of a CWallet method.

jonasschnelli force-pushed on Feb 7, 2017

jonasschnelli commented at 8:43 pm on February 7, 2017: contributor

Completely rewrote the refactor.

Moved/refactored relevant code into feeBumper.cpp (CFeeBump) which breaks the pattern of CWallet as god class.
Split BumpFee into three steps, Bump, Sign (new CWallet function, can later be used by CreateTransaction) and Commit.

jonasschnelli force-pushed on Feb 7, 2017

in src/wallet/wallet.cpp: in 65e2e0085a outdated

2317@@ -2318,50 +2318,70 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx)
2318     return GetVirtualTransactionSize(txNew);
2319 }
2320 
2321-enum CWallet::BumpFeeResult CWallet::BumpFee(uint256 txid, int newConfirmTarget, bool specifiedConfirmTarget, CAmount totalFee, bool newTxReplaceable, CAmount& nOldFee, CAmount& nNewFee, std::shared_ptr<CWalletTx>& wtxNew, std::vector<std::string>& vErrors)
2322+bool CWallet::SignTransaction(std::shared_ptr<CMutableTransaction> tx)

ryanofsky commented at 9:11 pm on February 7, 2017:

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.

in src/primitives/transaction.h: in cc585d5c18 outdated

450@@ -451,7 +451,9 @@ struct CMutableTransaction
451 };
452 
453 typedef std::shared_ptr<const CTransaction> CTransactionRef;
454+typedef std::shared_ptr<CMutableTransaction> CMutableTransactionRef;

ryanofsky commented at 11:41 pm on February 23, 2017:

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.

in src/wallet/feebumper.h: in cc585d5c18 outdated

11+class uint256;
12+
13+class CFeeBumper
14+{
15+public:
16+    enum class BumpFeeResult

ryanofsky commented at 11:42 pm on February 23, 2017:

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.

in src/wallet/feebumper.h: in cc585d5c18 outdated

26+    CFeeBumper::BumpFeeResult getResult() const { return currentResult; }
27+    const std::vector<std::string>& getErrors() const { return vErrors; }
28+    CAmount getOldFee() const { return nOldFee; }
29+    CAmount getNewFee() const { return nNewFee; }
30+    CMutableTransactionRef getBumpedTxRef() const { return mtx; }
31+    uint256 getBumpedTxId() const { return bumpedTxid; }

ryanofsky commented at 11:43 pm on February 23, 2017:

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:

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

without the stateful class.

ryanofsky commented at 11:57 pm on February 23, 2017: member

utACK cc585d5c182405997fe79e1f87bbee0747606e5d

I looked at all the moves in meld and verified that no behavior should be changing. I left a few comments on how I think the feebumper API might be improved, but do not think any work in that area should block this PR, because this is already doing a great job of moving things where they need to be, and this doesn’t seem like an easy PR to keep maintaining and updating.

One suggestion I might make, though, is to maybe squash some of the commits. For example the commit 641c99cb6f96afa166a293917f36f3cf8f28a08a doesn’t compile and might be annoying for git-bisect. And the multiple moves might be confusing for reviewers looking at the PR commit-by-commit.

Will take a look at the followup PR 9697 as soon as I get a chance.

jonasschnelli commented at 10:18 am on March 2, 2017: contributor

Added a commit c81a8a7ddef6f3c1e4cb0816aa12ae16b91cbf02 that addresses the shared pointer for CMutableTransaction.

The second commit a9c25b611504225d61291365ab6025dbd426e2d0 moves the enum class BumpFeeResult into the global scope.

I haven’t implemented the proposed API change (https://github.com/bitcoin/bitcoin/pull/9681#discussion_r102848152) which maybe could be a follow up. I’n not sure if this would go into a direction to purely static in/out functions.

in src/wallet/feebumper.cpp: in a9c25b6115 outdated

27+    // Look up the inputs.  We should have already checked that this transaction
28+    // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
29+    // wallet, with a valid index into the vout array.
30+    for (auto& input : tx.vin) {
31+        const auto mi = pwalletMain->mapWallet.find(input.prevout.hash);
32+        assert(mi != pwalletMain->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());

ryanofsky commented at 2:44 pm on March 2, 2017:

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

ryanofsky commented at 2:51 pm on March 2, 2017: member

utACK a9c25b611504225d61291365ab6025dbd426e2d0

Should squash the commits down though to reduce churn, one or two commits would be ideal.

jonasschnelli force-pushed on Mar 3, 2017

jonasschnelli commented at 3:26 pm on March 3, 2017: contributor

Needed rebase (#8775). Squashed down to three relevant commits.

ryanofsky commented at 3:48 pm on March 3, 2017: member

utACK 4ce1d6fb995bc30551571fe3621f185ea66eae7f

Comparing against previous a9c25b611504225d61291365ab6025dbd426e2d0, there were no changes except for some whitespace and a renamed local iterator variable.

jonasschnelli commented at 3:50 pm on March 3, 2017: contributor

[…] renamed local iterator variable.

Had to change this to respect the -Wshadow (#9828) change

ryanofsky commented at 2:32 pm on March 9, 2017: member

It would be really nice to have this merged soon. This PR is blocking the bumpfee GUI PR #9697, which is blocking the RBF checkbox PR #9592, which is blocking the RBF RPC PR #9672, and possibly the -walletrbf default PR #9527. It would be great to start clearing this backlog.

I guess maybe this needs another reviewer? It also needs a rebase to avoid trivial conflicts with #9853 and #9643.

in src/wallet/feebumper.cpp: in 4ce1d6fb99 outdated

118+        return;
119+    }
120+
121+    // Calculate the expected size of the new transaction.
122+    int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
123+    const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(*wtx.tx);

ryanofsky commented at 11:03 pm on March 9, 2017:

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: https://github.com/ryanofsky/bitcoin/commit/731ca5c652d42806e82892695f4e43082c9d6a20

jonasschnelli force-pushed on Mar 10, 2017

jonasschnelli commented at 7:51 am on March 10, 2017: contributor

Rebased (carefully). Added @ryanofsky’s https://github.com/ryanofsky/bitcoin/commit/731ca5c652d42806e82892695f4e43082c9d6a20

in src/wallet/feebumper.cpp: in e9c88c855e outdated

73+        }
74+    }
75+
76+    if (wtx.GetDepthInMainChain() != 0) {
77+        vErrors.push_back("Transaction has been mined, or is conflicted with a mined transaction");
78+        currentResult = BumpFeeResult::MISC_ERROR;

ryanofsky commented at 7:52 pm on March 10, 2017:

This appears to be WALLET_ERROR not MISC_ERROR in master:

https://github.com/bitcoin/bitcoin/blob/8910b4717e5bb946ee6988f7fe9fd461f53a5935/src/wallet/rpcwallet.cpp#L2875

jonasschnelli commented at 1:37 pm on March 16, 2017:

Thanks. Great catch… fixed now.

in src/wallet/rpcwallet.cpp: in a6183598ea outdated

2960-        const CScript& scriptPubKey = mi->second.tx->vout[input.prevout.n].scriptPubKey;
2961-        const CAmount& amount = mi->second.tx->vout[input.prevout.n].nValue;
2962-        SignatureData sigdata;
2963-        if (!ProduceSignature(TransactionSignatureCreator(pwallet, &txNewConst, nIn, amount, SIGHASH_ALL), scriptPubKey, sigdata)) {
2964-            throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction.");
2965+    CFeeBumper feeBump(pwalletMain, hash, newConfirmTarget, specifiedConfirmTarget, totalFee, replaceable);

ryanofsky commented at 7:57 pm on March 10, 2017:

pwalletMain should be pwallet, also a few places below

ryanofsky commented at 7:58 pm on March 10, 2017: member

utACK a6183598ea15d0b9acd8e22b3f8af6ac444f9db6, but any chance you could squash the following three commits into one?

26df8ae0fb9403acc8c64fe60d891f74a197a231 [Move] move bumpfee core logic to CWallet
e9c88c855e558b7372445414a57ef635d18d22eb Refactor Bumpfee core functionality
a6183598ea15d0b9acd8e22b3f8af6ac444f9db6 fixup! Refactor BumpFee, move relevant code into CFeeBumper

The first commit there doesn’t compile, and the third commit was meant to be a fixup to another commit which no longer exists. Also, having the code move twice in one PR (rpcwallet -> wallet -> feebumper) only makes the PR harder to rebase and review, AFAICT.

jonasschnelli force-pushed on Mar 16, 2017

jonasschnelli commented at 1:37 pm on March 16, 2017: contributor

Fixed @ryanofsky findings and squashed into a single commit.

in src/wallet/rpcwallet.cpp:2860 in 1d8529bf3c outdated

2849@@ -2933,157 +2850,55 @@ UniValue bumpfee(const JSONRPCRequest& request)
2850             }
2851         } else if (options.exists("totalFee")) {
2852             totalFee = options["totalFee"].get_int64();

ryanofsky commented at 4:30 pm on March 17, 2017:

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:

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

jonasschnelli commented at 11:57 am on March 27, 2017:

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

ryanofsky commented at 4:36 pm on March 17, 2017: member

Slightly tested ACK 1d8529bf3c279e8e2128425cec12c2b8263a5e54.

Ran tests, confirmed the RPC error code and pwalletMain usages were fixed and that there were no other changes since last my previous ACK.

I did notice one case where the PR seemed to be unintentionally changing behavior (no longer raising error on 0 or negative totalFee option values, see comment in rpcwallet.cpp), but other than that I confirmed this should only be moving code, not changing behavior.

in src/wallet/wallet.cpp:2251 in 1d8529bf3c outdated

2246+    // sign the new tx
2247+    CTransaction txNewConst(tx);
2248+    int nIn = 0;
2249+    for (auto& input : tx.vin) {
2250+        std::map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(input.prevout.hash);
2251+        assert(mi != mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());

jnewbery commented at 8:55 pm on March 21, 2017:

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.

jonasschnelli commented at 11:57 am on March 27, 2017:

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

in src/wallet/feebumper.cpp:243 in 1d8529bf3c outdated

231+
232+bool CFeeBumper::commit(CWallet *pWallet)
233+{
234+    AssertLockHeld(pWallet->cs_wallet);
235+    vErrors.clear();
236+    if (txid.IsNull() || !pWallet->mapWallet.count(txid)) {

jnewbery commented at 9:06 pm on March 21, 2017:

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.

jonasschnelli commented at 11:59 am on March 27, 2017:

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

jnewbery commented at 9:32 pm on March 21, 2017: member

I’ve quickly reviewed the parts which I think have changed during the move. I haven’t verified that the code move hasn’t changed any of the functionality in the code that has been moved.

A couple of comments from me about error handling. Otherwise, lightly tested ACK https://github.com/bitcoin/bitcoin/pull/9681/commits/1d8529bf3c279e8e2128425cec12c2b8263a5e54

jonasschnelli force-pushed on Mar 27, 2017

jonasschnelli commented at 1:43 pm on March 27, 2017: contributor

Addressed @ryanofsky and @jnewbery points. Hopefully soon ready for merge.

ryanofsky commented at 8:02 pm on March 27, 2017: member

utACK ccbdb2ceadade6059d7c375592f01db7feda05f6. The three new commits, as well as the main commit 1d8529bf3c279e8e2128425cec12c2b8263a5e54 (previously ACKed) all look good.

in src/wallet/feebumper.cpp:23 in 1d8529bf3c outdated

18+// TODO: re-use this in CWallet::CreateTransaction (right now
19+// CreateTransaction uses the constructed dummy-signed tx to do a priority
20+// calculation, but we should be able to refactor after priority is removed).
21+// NOTE: this requires that all inputs must be in mapWallet (eg the tx should
22+// be IsAllFromMe).
23+int64_t CalculateMaximumSignedTxSize(const CWallet *pWallet, const CTransaction &tx)

JeremyRubin commented at 8:12 pm on March 27, 2017:

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

jonasschnelli commented at 6:55 am on March 28, 2017:

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

in src/wallet/feebumper.cpp:33 in 1d8529bf3c outdated

28+    // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
29+    // wallet, with a valid index into the vout array.
30+    for (auto& input : tx.vin) {
31+        const auto mi = pWallet->mapWallet.find(input.prevout.hash);
32+        assert(mi != pWallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());
33+        vCoins.emplace_back(std::make_pair(&(mi->second), input.prevout.n));

JeremyRubin commented at 8:16 pm on March 27, 2017:

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

in src/wallet/feebumper.cpp:123 in 1d8529bf3c outdated

118+        return;
119+    }
120+
121+    // Calculate the expected size of the new transaction.
122+    int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
123+    const int64_t maxNewTxSize = CalculateMaximumSignedTxSize(pWallet, *wtx.tx);

JeremyRubin commented at 8:36 pm on March 27, 2017:

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.

jonasschnelli commented at 6:59 am on March 28, 2017:

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

in src/wallet/feebumper.cpp:150 in 1d8529bf3c outdated

145+            vErrors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
146+                                                                FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(::incrementalRelayFee.GetFee(maxNewTxSize))));
147+            currentResult = BumpFeeResult::INVALID_PARAMETER;
148+            return;
149+        }
150+        CAmount requiredFee = pWallet->GetRequiredFee(maxNewTxSize);

JeremyRubin commented at 8:43 pm on March 27, 2017:

Reason to change this line from CWallet::GetRequiredFee?

in src/wallet/feebumper.cpp:162 in 1d8529bf3c outdated

157+        nNewFee = totalFee;
158+        nNewFeeRate = CFeeRate(totalFee, maxNewTxSize);
159+    } else {
160+        // if user specified a confirm target then don't consider any global payTxFee
161+        if (specifiedConfirmTarget) {
162+            nNewFee = pWallet->GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool, CAmount(0));

JeremyRubin commented at 8:46 pm on March 27, 2017:

This is also static, please keep CWallet::

in src/wallet/feebumper.cpp:166 in 1d8529bf3c outdated

161+        if (specifiedConfirmTarget) {
162+            nNewFee = pWallet->GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool, CAmount(0));
163+        }
164+        // otherwise use the regular wallet logic to select payTxFee or default confirm target
165+        else {
166+            nNewFee = pWallet->GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool);

JeremyRubin commented at 8:46 pm on March 27, 2017:

This is also static, please keep CWallet::

jonasschnelli commented at 7:02 am on March 28, 2017:

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.

in src/wallet/feebumper.cpp:205 in 1d8529bf3c outdated

200+    }
201+
202+    // Now modify the output to increase the fee.
203+    // If the output is not large enough to pay the fee, fail.
204+    CAmount nDelta = nNewFee - nOldFee;
205+    assert(nDelta > 0);

JeremyRubin commented at 9:12 pm on March 27, 2017:

Maybe you should refactor this to:

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

jonasschnelli commented at 7:02 am on March 28, 2017:

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

JeremyRubin commented at 2:19 pm on March 28, 2017:

I think it makes sense to just change it now… #8456 (review) 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.

jonasschnelli commented at 3:39 pm on March 28, 2017:

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

in src/wallet/rpcwallet.cpp:2876 in 1d8529bf3c outdated

2869-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction has descendants in the wallet");
2870-    }
2871 
2872+    CFeeBumper feeBump(pwallet, hash, newConfirmTarget, specifiedConfirmTarget, totalFee, replaceable);
2873+    BumpFeeResult res = feeBump.getResult();
2874+    if (res != BumpFeeResult::OK)

JeremyRubin commented at 9:15 pm on March 27, 2017:

You should just make this a case of the switch.

in src/wallet/feebumper.cpp:235 in 1d8529bf3c outdated

230+}
231+
232+bool CFeeBumper::commit(CWallet *pWallet)
233+{
234+    AssertLockHeld(pWallet->cs_wallet);
235+    vErrors.clear();

JeremyRubin commented at 9:21 pm on March 27, 2017:

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

jonasschnelli commented at 7:05 am on March 28, 2017:

Good point. Will change.

in src/wallet/feebumper.cpp:237 in 1d8529bf3c outdated

227+    }
228+
229+    currentResult = BumpFeeResult::OK;
230+}
231+
232+bool CFeeBumper::commit(CWallet *pWallet)

JeremyRubin commented at 9:23 pm on March 27, 2017:

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

jonasschnelli commented at 7:03 am on March 28, 2017:

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.

JeremyRubin commented at 2:30 pm on March 28, 2017:

What if you do something like

0commit() {
1    assert(!comitted)
2    comitted = true;
3     ...
4}
5~CFeeBumper() {
6    if (vErrors.empty())
7    assert(comitted);
8}

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

jonasschnelli commented at 3:38 pm on March 28, 2017:

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

JeremyRubin changes_requested

JeremyRubin commented at 9:27 pm on March 27, 2017: contributor

utack ccbdb2ceadade6059d7c375592f01db7feda05f6, couple of nits/things to fix.

jonasschnelli commented at 7:09 am on March 28, 2017: contributor

Thanks @JeremyRubin for the review. Fixed the relevant points.

in src/wallet/feebumper.cpp:23 in 2b573c4ebe outdated

19@@ -20,7 +20,7 @@
20 // calculation, but we should be able to refactor after priority is removed).
21 // NOTE: this requires that all inputs must be in mapWallet (eg the tx should
22 // be IsAllFromMe).
23-int64_t CalculateMaximumSignedTxSize(const CWallet *pWallet, const CTransaction &tx)
24+int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWallet)

ryanofsky commented at 12:48 pm on March 28, 2017:

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 src/wallet/feebumper.cpp:33 in aa55df28bc outdated

29@@ -30,7 +30,7 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWal
30     for (auto& input : tx.vin) {
31         const auto mi = pWallet->mapWallet.find(input.prevout.hash);
32         assert(mi != pWallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());
33-        vCoins.emplace_back(std::make_pair(&(mi->second), input.prevout.n));
34+        vCoins.emplace_back(&(mi->second), input.prevout.n);

ryanofsky commented at 12:49 pm on March 28, 2017:

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.)

ryanofsky commented at 12:55 pm on March 28, 2017: member

utACK 3 new commits in 99070569662dde84a4f4b1d38cd53762063f53d2 since previously ACKed ccbdb2ceadade6059d7c375592f01db7feda05f6. This seems like it is ready to be merged.

in src/wallet/feebumper.cpp:238 in 9907056966 outdated

231@@ -232,6 +232,9 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConf
232 bool CFeeBumper::commit(CWallet *pWallet)
233 {
234     AssertLockHeld(pWallet->cs_wallet);
235+    if (!vErrors.empty()) {
236+        return false;
237+    }
238     vErrors.clear();

JeremyRubin commented at 1:37 pm on March 28, 2017:

vErrors.clear() is now redundant.

in src/wallet/feebumper.h:50 in 9907056966 outdated

35+
36+private:
37+    const uint256 txid;
38+    uint256 bumpedTxid;
39+    CMutableTransaction mtx;
40+    std::vector<std::string> vErrors;

JeremyRubin commented at 2:38 pm on March 28, 2017:

You do not use vErrors to store more than one error, so may as well just use a std::string (this clarifies it is set-once).

Alternatively, you could use a smart pointer to a string and just check that it is not nullptr.

jonasschnelli commented at 3:35 pm on March 28, 2017:

L265 and L274 can both add an error that then get’s spit out over the JSON-RPC “errors” array. I haven’t changed this in this PR.

in src/wallet/feebumper.cpp:235 in 9907056966 outdated

230+}
231+
232+bool CFeeBumper::commit(CWallet *pWallet)
233+{
234+    AssertLockHeld(pWallet->cs_wallet);
235+    if (!vErrors.empty()) {

JeremyRubin commented at 2:40 pm on March 28, 2017:

maybe change to/add && BumpFeeResult::OK == currentResult

JeremyRubin changes_requested

jonasschnelli force-pushed on Mar 28, 2017

jonasschnelli commented at 3:43 pm on March 28, 2017: contributor

Amend-changed the last commit to remove the redundant vErrors.clear(). Added a check for currentResult == BumpFeeResult::OK during commit.

in src/wallet/feebumper.cpp:152 in fb7bb67a41 outdated

147+            currentResult = BumpFeeResult::INVALID_PARAMETER;
148+            return;
149+        }
150+        CAmount requiredFee = CWallet::GetRequiredFee(maxNewTxSize);
151+        if (totalFee < requiredFee) {
152+            vErrors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",

TheBlueMatt commented at 1:47 pm on March 29, 2017:

Having this and the totalFee error be separate is somewhat strange, maybe combine them now that the checks are in the same place (in an additional commit) into one generic error so we dont have users chasing error messages for a few rounds?

in src/wallet/feebumper.cpp:45 in fb7bb67a41 outdated

40+    return GetVirtualTransactionSize(txNew);
41+}
42+
43+CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConfirmTarget, bool specifiedConfirmTarget, CAmount totalFee, bool newTxReplaceable)
44+    :
45+    txid(txidIn),

TheBlueMatt commented at 2:21 pm on March 29, 2017:

nit: pass txidIn by reference or std::move it here.

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

26+    CFeeBumper(const CWallet *pWalletIn, const uint256 txidIn, int newConfirmTarget, bool specifiedConfirmTarget, CAmount totalFee, bool newTxReplaceable);
27+    BumpFeeResult getResult() const { return currentResult; }
28+    const std::vector<std::string>& getErrors() const { return vErrors; }
29+    CAmount getOldFee() const { return nOldFee; }
30+    CAmount getNewFee() const { return nNewFee; }
31+    CMutableTransaction* getBumpedTxRef() { return &mtx; }

TheBlueMatt commented at 2:33 pm on March 29, 2017:

This feels like a super strange API…you’re getting a reference to memory owned by CFeeBumper, passing it to the wallet, then calling a method owned by CFeeBumper which assumes you mutated its (seemingly internal) state. Given you’re already passing a pwallet reference into CFeeBumper, maybe replace this with bool SignTransaction(CWallet *pwalletIn)?

in src/wallet/feebumper.h:44 in fb7bb67a41 outdated

29+    CAmount getOldFee() const { return nOldFee; }
30+    CAmount getNewFee() const { return nNewFee; }
31+    CMutableTransaction* getBumpedTxRef() { return &mtx; }
32+    uint256 getBumpedTxId() const { return bumpedTxid; }
33+
34+    bool commit(CWallet *pWalletNonConst);

TheBlueMatt commented at 2:37 pm on March 29, 2017:

Can you document the return value here? It initially confused me because I thought it would return false if it set vErrors, when, in fact, it might add vErrors and then return true.

TheBlueMatt commented at 2:40 pm on March 29, 2017: member

Generally looks good, only issue I have is the API where the RPC calls SignTransaction directly feels super strange.

jonasschnelli commented at 7:28 am on March 30, 2017: contributor

Added a commit that addresses @TheBlueMatt’s points.

Changed CFeeBumper’s interface. It has now a signTransaction method instead of the – indeed strange – getBumpedTxRef().
Made use of std::move for the txIn.
Added some comments

jonasschnelli added this to the "Blockers" column in a project

Bumpfee move request parameter interaction to the top d1a95e8d3d

Refactor Bumpfee core functionality 0337a39d31

Restore invalid fee check (must be > 0) 2718db0705

Directly abort execution in FeeBumper::commit if wallet or tx is not available bcc72cccc7

Use "return false" instead assert() in CWallet::SignTransaction 51ea44f01c

Restore CalculateMaximumSignedTxSize function signature bb78c1599e

Use static calls for GetRequiredFee and GetMinimumFee, remove make_pair from emplace_back 44cabe6380

Cancel feebump is vErrors is not empty 0df22ed6fd

Improve CFeeBumper interface, add comments, make use of std::move 5f59d3ecb7

jonasschnelli force-pushed on Apr 2, 2017

jonasschnelli commented at 8:13 am on April 2, 2017: contributor

Rebased (trivial) after #9424

ryanofsky commented at 1:37 pm on April 7, 2017: member

utACK 5f59d3ecb7f1b63579e7f07fc520459cdf119c81, confirmed first 8 commits (previously acked as 99070569662dde84a4f4b1d38cd53762063f53d2) did not change in the rebase, other than the log category fix. New 9th commit also looks fine.

I think it would be good to merge this because it is a burden to maintain separately and keep reviewing. It’s been rebased many times and is blocking other prs.

laanwj merged this on Apr 7, 2017

laanwj closed this on Apr 7, 2017

laanwj referenced this in commit a5fd746674 on Apr 7, 2017

jonasschnelli removed this from the "Blockers" column in a project

MarcoFalke locked this on Sep 8, 2021

Refactor Bumpfee, move core functionality to CWallet #9681