diff options
author | MarcoFalke <falke.marco@gmail.com> | 2019-10-15 14:31:15 -0400 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2019-10-15 14:32:03 -0400 |
commit | e180be49d7d802d05b1850924be97fe27d353ffe (patch) | |
tree | a9c07662f679c8cc4216fb0ba77bf6103a72db49 /src | |
parent | 137b7a2af1aa3e19e543af68345c6e10dd27a6dc (diff) | |
parent | 66b29848c71c4b3b4dc36ca6d94de829bd533797 (diff) | |
download | bitcoin-e180be49d7d802d05b1850924be97fe27d353ffe.tar.xz |
Merge #17098: refactor: Feebumper EstimateFeeRate follow-up
66b29848c71c4b3b4dc36ca6d94de829bd533797 change wallet pointers to references in feebumper (Adam Jonas)
9be6666a4e648782b49a6fdc458372ea521b444d typo and unneccessary parentheses (Adam Jonas)
Pull request description:
Picking up some of the suggestions in the comments of #16727 including:
https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330547321
https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330549766
https://github.com/bitcoin/bitcoin/pull/16727#discussion_r333209674
ACKs for top commit:
promag:
Code review ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797.
MarcoFalke:
ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797 (looked at the diff on GitHub)
fjahr:
ACK 66b2984 reviewed code
Tree-SHA512: d118f7689970fe39d9f5318dc818f13283cce9194370b3ce4758f298172e4681ae119ddc809f5c0b7602677137ac0d38147b915422ff616531a76a570b766fa2
Diffstat (limited to 'src')
-rw-r--r-- | src/interfaces/wallet.cpp | 8 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 78 | ||||
-rw-r--r-- | src/wallet/feebumper.h | 8 | ||||
-rw-r--r-- | src/wallet/rpcwallet.cpp | 6 |
4 files changed, 50 insertions, 50 deletions
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 0c8d92eba5..aff08bfbea 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -241,7 +241,7 @@ public: } bool transactionCanBeBumped(const uint256& txid) override { - return feebumper::TransactionCanBeBumped(m_wallet.get(), txid); + return feebumper::TransactionCanBeBumped(*m_wallet.get(), txid); } bool createBumpTransaction(const uint256& txid, const CCoinControl& coin_control, @@ -255,17 +255,17 @@ public: return feebumper::CreateTotalBumpTransaction(m_wallet.get(), txid, coin_control, total_fee, errors, old_fee, new_fee, mtx) == feebumper::Result::OK; } else { - return feebumper::CreateRateBumpTransaction(m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx) == + return feebumper::CreateRateBumpTransaction(*m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx) == feebumper::Result::OK; } } - bool signBumpTransaction(CMutableTransaction& mtx) override { return feebumper::SignTransaction(m_wallet.get(), mtx); } + bool signBumpTransaction(CMutableTransaction& mtx) override { return feebumper::SignTransaction(*m_wallet.get(), mtx); } bool commitBumpTransaction(const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid) override { - return feebumper::CommitTransaction(m_wallet.get(), txid, std::move(mtx), errors, bumped_txid) == + return feebumper::CommitTransaction(*m_wallet.get(), txid, std::move(mtx), errors, bumped_txid) == feebumper::Result::OK; } CTransactionRef getTx(const uint256& txid) override diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index b87231293f..fbcf8d4de7 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -17,15 +17,15 @@ //! Check whether transaction has descendant in wallet or mempool, or has been //! mined, or conflicts with a mined transaction. Return a feebumper::Result. -static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chain, const CWallet* wallet, const CWalletTx& wtx, std::vector<std::string>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) +static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chain, const CWallet& wallet, const CWalletTx& wtx, std::vector<std::string>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { - if (wallet->HasWalletSpend(wtx.GetHash())) { + if (wallet.HasWalletSpend(wtx.GetHash())) { errors.push_back("Transaction has descendants in the wallet"); return feebumper::Result::INVALID_PARAMETER; } { - if (wallet->chain().hasDescendantsInMempool(wtx.GetHash())) { + if (wallet.chain().hasDescendantsInMempool(wtx.GetHash())) { errors.push_back("Transaction has descendants in the mempool"); return feebumper::Result::INVALID_PARAMETER; } @@ -48,7 +48,7 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai // check that original tx consists entirely of our inputs // if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee) - if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) { + if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) { errors.push_back("Transaction contains inputs that don't belong to this wallet"); return feebumper::Result::WALLET_ERROR; } @@ -58,13 +58,13 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai } //! Check if the user provided a valid feeRate -static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wtx, const CFeeRate& newFeerate, const int64_t maxTxSize, std::vector<std::string>& errors) { +static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wtx, const CFeeRate& newFeerate, const int64_t maxTxSize, std::vector<std::string>& errors) { // check that fee rate is higher than mempool's minimum fee // (no point in bumping fee if we know that the new tx won't be accepted to the mempool) // This may occur if the user set FeeRate, TotalFee or paytxfee too low, if fallbackfee is too low, or, perhaps, // in a rare situation where the mempool minimum fee increased significantly since the fee estimation just a // moment earlier. In this case, we report an error to the user, who may adjust the fee. - CFeeRate minMempoolFeeRate = wallet->chain().mempoolMinFee(); + CFeeRate minMempoolFeeRate = wallet.chain().mempoolMinFee(); if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) { errors.push_back(strprintf( @@ -76,7 +76,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt CAmount new_total_fee = newFeerate.GetFee(maxTxSize); - CFeeRate incrementalRelayFee = std::max(wallet->chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE)); + CFeeRate incrementalRelayFee = std::max(wallet.chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE)); // Given old total fee and transaction size, calculate the old feeRate CAmount old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut(); @@ -91,7 +91,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt return feebumper::Result::INVALID_PARAMETER; } - CAmount requiredFee = GetRequiredFee(*wallet, maxTxSize); + CAmount requiredFee = GetRequiredFee(wallet, maxTxSize); if (new_total_fee < requiredFee) { errors.push_back(strprintf("Insufficient total fee (cannot be less than required fee %s)", FormatMoney(requiredFee))); @@ -99,7 +99,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt } // Check that in all cases the new fee doesn't violate maxTxFee - const CAmount max_tx_fee = wallet->m_default_max_tx_fee; + const CAmount max_tx_fee = wallet.m_default_max_tx_fee; if (new_total_fee > max_tx_fee) { errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)", FormatMoney(new_total_fee), FormatMoney(max_tx_fee))); @@ -109,7 +109,7 @@ static feebumper::Result CheckFeeRate(const CWallet* wallet, const CWalletTx& wt return feebumper::Result::OK; } -static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount& old_fee) +static CFeeRate EstimateFeeRate(const CWallet& wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount& old_fee) { // Get the fee rate of the original transaction. This is calculated from // the tx fee/vsize, so it may have been rounded down. Add 1 satoshi to the @@ -123,15 +123,15 @@ static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinCont // the minimum of that and the wallet's conservative // WALLET_INCREMENTAL_RELAY_FEE value to future proof against changes to // network wide policy for incremental relay fee that our node may not be - // aware of. This ensures we're over the over the required relay fee rate + // aware of. This ensures we're over the required relay fee rate // (BIP 125 rule 4). The replacement tx will be at least as large as the // original tx, so the total fee will be greater (BIP 125 rule 3) - CFeeRate node_incremental_relay_fee = wallet->chain().relayIncrementalFee(); + CFeeRate node_incremental_relay_fee = wallet.chain().relayIncrementalFee(); CFeeRate wallet_incremental_relay_fee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE); feerate += std::max(node_incremental_relay_fee, wallet_incremental_relay_fee); // Fee rate must also be at least the wallet's GetMinimumFeeRate - CFeeRate min_feerate(GetMinimumFeeRate(*wallet, coin_control, /* feeCalc */ nullptr)); + CFeeRate min_feerate(GetMinimumFeeRate(wallet, coin_control, /* feeCalc */ nullptr)); // Set the required fee rate for the replacement transaction in coin control. return std::max(feerate, min_feerate); @@ -139,11 +139,11 @@ static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinCont namespace feebumper { -bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid) +bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid) { - auto locked_chain = wallet->chain().lock(); - LOCK(wallet->cs_wallet); - const CWalletTx* wtx = wallet->GetWalletTx(txid); + auto locked_chain = wallet.chain().lock(); + LOCK(wallet.cs_wallet); + const CWalletTx* wtx = wallet.GetWalletTx(txid); if (wtx == nullptr) return false; std::vector<std::string> errors_dummy; @@ -166,7 +166,7 @@ Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, co } const CWalletTx& wtx = it->second; - Result result = PreconditionChecks(*locked_chain, wallet, wtx, errors); + Result result = PreconditionChecks(*locked_chain, *wallet, wtx, errors); if (result != Result::OK) { return result; } @@ -276,17 +276,17 @@ Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, co } -Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors, +Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors, CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx) { // We are going to modify coin control later, copy to re-use CCoinControl new_coin_control(coin_control); - auto locked_chain = wallet->chain().lock(); - LOCK(wallet->cs_wallet); + auto locked_chain = wallet.chain().lock(); + LOCK(wallet.cs_wallet); errors.clear(); - auto it = wallet->mapWallet.find(txid); - if (it == wallet->mapWallet.end()) { + auto it = wallet.mapWallet.find(txid); + if (it == wallet.mapWallet.end()) { errors.push_back("Invalid or non-wallet transaction id"); return Result::INVALID_ADDRESS_OR_KEY; } @@ -300,7 +300,7 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo // Fill in recipients(and preserve a single change key if there is one) std::vector<CRecipient> recipients; for (const auto& output : wtx.tx->vout) { - if (!wallet->IsChange(output)) { + if (!wallet.IsChange(output)) { CRecipient recipient = {output.scriptPubKey, output.nValue, false}; recipients.push_back(recipient); } else { @@ -313,8 +313,8 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo if (coin_control.m_feerate) { // The user provided a feeRate argument. // We calculate this here to avoid compiler warning on the cs_wallet lock - const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet); - Result res = CheckFeeRate(wallet, wtx, *(new_coin_control.m_feerate), maxTxSize, errors); + const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, &wallet); + Result res = CheckFeeRate(wallet, wtx, *new_coin_control.m_feerate, maxTxSize, errors); if (res != Result::OK) { return res; } @@ -342,7 +342,7 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo CAmount fee_ret; int change_pos_in_out = -1; // No requested location for change std::string fail_reason; - if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { + if (!wallet.CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { errors.push_back("Unable to create transaction: " + fail_reason); return Result::WALLET_ERROR; } @@ -353,7 +353,7 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo // Write back transaction mtx = CMutableTransaction(*tx_new); // Mark new tx not replaceable, if requested. - if (!coin_control.m_signal_bip125_rbf.get_value_or(wallet->m_signal_rbf)) { + if (!coin_control.m_signal_bip125_rbf.get_value_or(wallet.m_signal_rbf)) { for (auto& input : mtx.vin) { if (input.nSequence < 0xfffffffe) input.nSequence = 0xfffffffe; } @@ -362,21 +362,21 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo return Result::OK; } -bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) { - auto locked_chain = wallet->chain().lock(); - LOCK(wallet->cs_wallet); - return wallet->SignTransaction(mtx); +bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) { + auto locked_chain = wallet.chain().lock(); + LOCK(wallet.cs_wallet); + return wallet.SignTransaction(mtx); } -Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid) +Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid) { - auto locked_chain = wallet->chain().lock(); - LOCK(wallet->cs_wallet); + auto locked_chain = wallet.chain().lock(); + LOCK(wallet.cs_wallet); if (!errors.empty()) { return Result::MISC_ERROR; } - auto it = txid.IsNull() ? wallet->mapWallet.end() : wallet->mapWallet.find(txid); - if (it == wallet->mapWallet.end()) { + auto it = txid.IsNull() ? wallet.mapWallet.end() : wallet.mapWallet.find(txid); + if (it == wallet.mapWallet.end()) { errors.push_back("Invalid or non-wallet transaction id"); return Result::MISC_ERROR; } @@ -394,7 +394,7 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti mapValue["replaces_txid"] = oldWtx.GetHash().ToString(); CValidationState state; - if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state)) { + if (!wallet.CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state)) { // NOTE: CommitTransaction never returns false, so this should never happen. errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state))); return Result::WALLET_ERROR; @@ -408,7 +408,7 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti } // mark the original tx as bumped - if (!wallet->MarkReplaced(oldWtx.GetHash(), bumped_txid)) { + if (!wallet.MarkReplaced(oldWtx.GetHash(), bumped_txid)) { // TODO: see if JSON-RPC has a standard way of returning a response // along with an exception. It would be good to return information about // wtxBumped to the caller even if marking the original transaction diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 0c4e1cb7dd..9357397606 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -26,7 +26,7 @@ enum class Result }; //! Return whether transaction can be bumped. -bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid); +bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid); //! Create bumpfee transaction based on total amount. Result CreateTotalBumpTransaction(const CWallet* wallet, @@ -39,7 +39,7 @@ Result CreateTotalBumpTransaction(const CWallet* wallet, CMutableTransaction& mtx); //! Create bumpfee transaction based on feerate estimates. -Result CreateRateBumpTransaction(CWallet* wallet, +Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors, @@ -50,13 +50,13 @@ Result CreateRateBumpTransaction(CWallet* wallet, //! Sign the new transaction, //! @return false if the tx couldn't be found or if it was //! impossible to create the signature(s) -bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx); +bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx); //! Commit the bumpfee transaction. //! @return success in case of CWallet::CommitTransaction was successful, //! but sets errors if the tx could not be added to the mempool (will try later) //! or if the old transaction could not be marked as replaced. -Result CommitTransaction(CWallet* wallet, +Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index dc437e5cd8..debc3fecda 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3417,7 +3417,7 @@ static UniValue bumpfee(const JSONRPCRequest& request) res = feebumper::CreateTotalBumpTransaction(pwallet, hash, coin_control, totalFee, errors, old_fee, new_fee, mtx); } else { // Targeting feerate bump. - res = feebumper::CreateRateBumpTransaction(pwallet, hash, coin_control, errors, old_fee, new_fee, mtx); + res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx); } if (res != feebumper::Result::OK) { switch(res) { @@ -3440,12 +3440,12 @@ static UniValue bumpfee(const JSONRPCRequest& request) } // sign bumped transaction - if (!feebumper::SignTransaction(pwallet, mtx)) { + if (!feebumper::SignTransaction(*pwallet, mtx)) { throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction."); } // commit the bumped transaction uint256 txid; - if (feebumper::CommitTransaction(pwallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) { + if (feebumper::CommitTransaction(*pwallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) { throw JSONRPCError(RPC_WALLET_ERROR, errors[0]); } UniValue result(UniValue::VOBJ); |