Skip to content

Commit

Permalink
[send] Make send RPCs return fee reason
Browse files Browse the repository at this point in the history
  • Loading branch information
stackman27 committed Sep 27, 2020
1 parent 1b313ca commit d5863c0
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 21 deletions.
3 changes: 2 additions & 1 deletion src/interfaces/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,9 @@ class WalletImpl : public Wallet
{
LOCK(m_wallet->cs_wallet);
CTransactionRef tx;
FeeCalculation fee_calc_out;
if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos,
fail_reason, coin_control, sign)) {
fail_reason, coin_control, fee_calc_out, sign)) {
return {};
}
return tx;
Expand Down
2 changes: 2 additions & 0 deletions src/rpc/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "sendtoaddress", 5 , "replaceable" },
{ "sendtoaddress", 6 , "conf_target" },
{ "sendtoaddress", 8, "avoid_reuse" },
{ "sendtoaddress", 9, "verbose"},
{ "settxfee", 0, "amount" },
{ "sethdseed", 0, "newkeypool" },
{ "getreceivedbyaddress", 1, "minconf" },
Expand Down Expand Up @@ -72,6 +73,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "sendmany", 4, "subtractfeefrom" },
{ "sendmany", 5 , "replaceable" },
{ "sendmany", 6 , "conf_target" },
{ "sendmany", 8, "verbose" },
{ "deriveaddresses", 1, "range" },
{ "scantxoutset", 1, "scanobjects" },
{ "addmultisigaddress", 0, "nrequired" },
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/feebumper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
CAmount fee_ret;
int change_pos_in_out = -1; // No requested location for change
bilingual_str fail_reason;
if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
FeeCalculation fee_calc_out;
if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, fee_calc_out, false)) {
errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + fail_reason);
return Result::WALLET_ERROR;
}
Expand Down
55 changes: 43 additions & 12 deletions src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_f
}
}

UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std::vector<CRecipient> &recipients, mapValue_t map_value)
UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std::vector<CRecipient> &recipients, mapValue_t map_value, bool verbose)
{
EnsureWalletIsUnlocked(pwallet);

Expand All @@ -409,11 +409,18 @@ UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std
int nChangePosRet = -1;
bilingual_str error;
CTransactionRef tx;
bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
FeeCalculation fee_calc_out;
bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
if (!fCreated) {
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
}
pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */);
if (verbose) {
UniValue entry(UniValue::VOBJ);
entry.pushKV("txid", tx->GetHash().GetHex());
entry.pushKV("fee_reason", StringForFeeReason(fee_calc_out.reason));
return entry;
}
return tx->GetHash().GetHex();
}

Expand All @@ -438,9 +445,19 @@ static RPCHelpMan sendtoaddress()
" \"" + FeeModes("\"\n\"") + "\""},
{"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n"
" dirty if they have previously been used in a transaction."},
{"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra information about the transaction."},
},
RPCResult{
RPCResult::Type::STR_HEX, "txid", "The transaction id."
{
RPCResult{"if verbose is not set or set to false",
RPCResult::Type::STR_HEX, "txid", "The transaction id."
},
RPCResult{"if verbose is set to true",
RPCResult::Type::OBJ, "", "",
{
{RPCResult::Type::STR_HEX, "txid", "The transaction id."},
{RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
},
},
},
RPCExamples{
HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1")
Expand Down Expand Up @@ -497,8 +514,9 @@ static RPCHelpMan sendtoaddress()

std::vector<CRecipient> recipients;
ParseRecipients(address_amounts, subtractFeeFromAmount, recipients);
bool verbose = request.params[9].isNull() ? false: request.params[9].get_bool();

return SendMoney(pwallet, coin_control, recipients, mapValue);
return SendMoney(pwallet, coin_control, recipients, mapValue, verbose);
},
};
}
Expand Down Expand Up @@ -853,11 +871,22 @@ static RPCHelpMan sendmany()
{"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
" \"" + FeeModes("\"\n\"") + "\""},
{"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra infomration about the transaction."},
},
{
RPCResult{"if verbose is not set or set to false",
RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
"the number of addresses."
},
RPCResult{"if verbose is set to true",
RPCResult::Type::OBJ, "", "",
{
{RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
"the number of addresses."},
{RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
},
},
},
RPCResult{
RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
"the number of addresses."
},
RPCExamples{
"\nSend two amounts to two different addresses:\n"
+ HelpExampleCli("sendmany", "\"\" \"{\\\"" + EXAMPLE_ADDRESS[0] + "\\\":0.01,\\\"" + EXAMPLE_ADDRESS[1] + "\\\":0.02}\"") +
Expand Down Expand Up @@ -902,12 +931,14 @@ static RPCHelpMan sendmany()

std::vector<CRecipient> recipients;
ParseRecipients(sendTo, subtractFeeFromAmount, recipients);
bool verbose = request.params[8].isNull() ? false : request.params[8].get_bool();

return SendMoney(pwallet, coin_control, recipients, std::move(mapValue));
return SendMoney(pwallet, coin_control, recipients, std::move(mapValue), verbose);
},
};
}


static RPCHelpMan addmultisigaddress()
{
return RPCHelpMan{"addmultisigaddress",
Expand Down Expand Up @@ -4501,8 +4532,8 @@ static const CRPCCommand commands[] =
{ "wallet", "removeprunedfunds", &removeprunedfunds, {"txid"} },
{ "wallet", "rescanblockchain", &rescanblockchain, {"start_height", "stop_height"} },
{ "wallet", "send", &send, {"outputs","conf_target","estimate_mode","options"} },
{ "wallet", "sendmany", &sendmany, {"dummy","amounts","minconf","comment","subtractfeefrom","replaceable","conf_target","estimate_mode"} },
{ "wallet", "sendtoaddress", &sendtoaddress, {"address","amount","comment","comment_to","subtractfeefromamount","replaceable","conf_target","estimate_mode","avoid_reuse"} },
{ "wallet", "sendmany", &sendmany, {"dummy","amounts","minconf","comment","subtractfeefrom","replaceable","conf_target","estimate_mode","verbose"} },
{ "wallet", "sendtoaddress", &sendtoaddress, {"address","amount","comment","comment_to","subtractfeefromamount","replaceable","conf_target","estimate_mode","avoid_reuse","verbose"} },
{ "wallet", "sethdseed", &sethdseed, {"newkeypool","seed"} },
{ "wallet", "setlabel", &setlabel, {"address","label"} },
{ "wallet", "settxfee", &settxfee, {"amount"} },
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/test/wallet_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -524,8 +524,9 @@ class ListCoinsTestingSetup : public TestChain100Setup
int changePos = -1;
bilingual_str error;
CCoinControl dummy;
FeeCalculation fee_calc_out;
{
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy));
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy, fee_calc_out));
}
wallet->CommitTransaction(tx, {}, {});
CMutableTransaction blocktx;
Expand Down
10 changes: 7 additions & 3 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2631,7 +2631,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
LOCK(cs_wallet);

CTransactionRef tx_new;
if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, false)) {
FeeCalculation fee_calc_out;
if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, fee_calc_out, false)) {
return false;
}

Expand Down Expand Up @@ -2755,6 +2756,7 @@ bool CWallet::CreateTransactionInternal(
int& nChangePosInOut,
bilingual_str& error,
const CCoinControl& coin_control,
FeeCalculation& fee_calc_out,
bool sign)
{
CAmount nValue = 0;
Expand Down Expand Up @@ -3098,6 +3100,7 @@ bool CWallet::CreateTransactionInternal(
// Before we return success, we assume any change key will be used to prevent
// accidental re-use.
reservedest.KeepDestination();
fee_calc_out = feeCalc;

WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
Expand All @@ -3117,19 +3120,20 @@ bool CWallet::CreateTransaction(
int& nChangePosInOut,
bilingual_str& error,
const CCoinControl& coin_control,
FeeCalculation& fee_calc_out,
bool sign)
{
int nChangePosIn = nChangePosInOut;
CTransactionRef tx2 = tx;
bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, sign);
bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign);
// try with avoidpartialspends unless it's enabled already
if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
CCoinControl tmp_cc = coin_control;
tmp_cc.m_avoid_partial_spends = true;
CAmount nFeeRet2;
int nChangePosInOut2 = nChangePosIn;
bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results
if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, sign)) {
if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, fee_calc_out, sign)) {
// if fee of this alternative one is within the range of the max fee, we use this one
const bool use_aps = nFeeRet2 <= nFeeRet + m_max_aps_fee;
WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, use_aps ? "grouped" : "non-grouped");
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
// ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure
std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;

bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign);
bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign);

public:
/*
Expand Down Expand Up @@ -974,7 +974,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
* selected by SelectCoins(); Also create the change output, when needed
* @note passing nChangePosInOut as -1 will result in setting a random position
*/
bool CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign = true);
bool CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true);
/**
* Submit the transaction to the node's mempool and then relay to peers.
* Should be called after CreateTransaction unless you want to abort
Expand Down
1 change: 0 additions & 1 deletion test/functional/wallet_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,5 @@ def run_test(self):
assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))


if __name__ == '__main__':
WalletTest().main()

0 comments on commit d5863c0

Please sign in to comment.