Skip to content

Commit 6a164ea

Browse files
MarcoFalkeknst
MarcoFalke
authored and
knst
committed
Merge bitcoin#19501: send* RPCs in the wallet returns the "fee reason"
69cf5d4 [test] Make sure send rpc returns fee reason (Sishir Giri) d5863c0 [send] Make send RPCs return fee reason (Sishir Giri) Pull request description: Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py. link to the issue: maflcko#22 (comment) ACKs for top commit: instagibbs: ACK bitcoin@69cf5d4 meshcollider: utACK 69cf5d4 Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
1 parent b6c8d85 commit 6a164ea

File tree

10 files changed

+83
-27
lines changed

10 files changed

+83
-27
lines changed

‎src/coinjoin/util.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,8 @@ bool CTransactionBuilder::Commit(bilingual_str& strResult)
274274
CTransactionRef tx;
275275
{
276276
LOCK2(pwallet->cs_wallet, cs_main);
277-
if (!pwallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl)) {
277+
FeeCalculation fee_calc_out;
278+
if (!pwallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl, fee_calc_out)) {
278279
return false;
279280
}
280281
}

‎src/rpc/client.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
4444
{ "sendtoaddress", 6, "use_cj" },
4545
{ "sendtoaddress", 7, "conf_target" },
4646
{ "sendtoaddress", 9, "avoid_reuse" },
47+
{ "sendtoaddress", 10, "verbose"},
4748
{ "settxfee", 0, "amount" },
4849
{ "sethdseed", 0, "newkeypool" },
4950
{ "getreceivedbyaddress", 1, "minconf" },
@@ -89,6 +90,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
8990
{ "sendmany", 6, "use_is" },
9091
{ "sendmany", 7, "use_cj" },
9192
{ "sendmany", 8, "conf_target" },
93+
{ "sendmany", 10, "verbose" },
9294
{ "deriveaddresses", 1, "range" },
9395
{ "scantxoutset", 1, "scanobjects" },
9496
{ "addmultisigaddress", 0, "nrequired" },

‎src/rpc/evo.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,8 @@ static void FundSpecialTx(CWallet* pwallet, CMutableTransaction& tx, const Speci
270270
int nChangePos = -1;
271271
bilingual_str strFailReason;
272272

273-
if (!pwallet->CreateTransaction(vecSend, newTx, nFee, nChangePos, strFailReason, coinControl, false, tx.vExtraPayload.size())) {
273+
FeeCalculation fee_calc_out;
274+
if (!pwallet->CreateTransaction(vecSend, newTx, nFee, nChangePos, strFailReason, coinControl, fee_calc_out, false, tx.vExtraPayload.size())) {
274275
throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason.original);
275276
}
276277

‎src/wallet/interfaces.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,8 +271,9 @@ class WalletImpl : public Wallet
271271
LOCK(m_wallet->cs_wallet);
272272
ReserveDestination m_dest(m_wallet.get());
273273
CTransactionRef tx;
274+
FeeCalculation fee_calc_out;
274275
if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos,
275-
fail_reason, coin_control, sign)) {
276+
fail_reason, coin_control, fee_calc_out, sign)) {
276277
return {};
277278
}
278279
return tx;

‎src/wallet/rpcwallet.cpp‎

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,7 @@ void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_f
382382
}
383383
}
384384

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

@@ -395,11 +395,18 @@ UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std
395395
int nChangePosRet = -1;
396396
bilingual_str error;
397397
CTransactionRef tx;
398-
bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
398+
FeeCalculation fee_calc_out;
399+
bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
399400
if (!fCreated) {
400401
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
401402
}
402403
pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */);
404+
if (verbose) {
405+
UniValue entry(UniValue::VOBJ);
406+
entry.pushKV("txid", tx->GetHash().GetHex());
407+
entry.pushKV("fee_reason", StringForFeeReason(fee_calc_out.reason));
408+
return entry;
409+
}
403410
return tx->GetHash().GetHex();
404411
}
405412

@@ -425,9 +432,19 @@ static RPCHelpMan sendtoaddress()
425432
" \"" + FeeModes("\"\n\"") + "\""},
426433
{"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"
427434
" dirty if they have previously been used in a transaction."},
435+
{"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra information about the transaction."},
428436
},
429-
RPCResult{
430-
RPCResult::Type::STR_HEX, "txid", "The transaction id."
437+
{
438+
RPCResult{"if verbose is not set or set to false",
439+
RPCResult::Type::STR_HEX, "txid", "The transaction id."
440+
},
441+
RPCResult{"if verbose is set to true",
442+
RPCResult::Type::OBJ, "", "",
443+
{
444+
{RPCResult::Type::STR_HEX, "txid", "The transaction id."},
445+
{RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
446+
},
447+
},
431448
},
432449
RPCExamples{
433450
HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1")
@@ -486,8 +503,9 @@ static RPCHelpMan sendtoaddress()
486503

487504
std::vector<CRecipient> recipients;
488505
ParseRecipients(address_amounts, subtractFeeFromAmount, recipients);
506+
bool verbose = request.params[10].isNull() ? false: request.params[10].get_bool();
489507

490-
return SendMoney(pwallet, coin_control, recipients, mapValue);
508+
return SendMoney(pwallet, coin_control, recipients, mapValue, verbose);
491509
},
492510
};
493511
}
@@ -917,11 +935,22 @@ static RPCHelpMan sendmany()
917935
{"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)"},
918936
{"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
919937
" \"" + FeeModes("\"\n\"") + "\""},
938+
{"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra infomration about the transaction."},
939+
},
940+
{
941+
RPCResult{"if verbose is not set or set to false",
942+
RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
943+
"the number of addresses."
944+
},
945+
RPCResult{"if verbose is set to true",
946+
RPCResult::Type::OBJ, "", "",
947+
{
948+
{RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
949+
"the number of addresses."},
950+
{RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
951+
},
952+
},
920953
},
921-
RPCResult{
922-
RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
923-
"the number of addresses."
924-
},
925954
RPCExamples{
926955
"\nSend two amounts to two different addresses:\n"
927956
+ HelpExampleCli("sendmany", "\"\" \"{\\\"" + EXAMPLE_ADDRESS[0] + "\\\":0.01,\\\"" + EXAMPLE_ADDRESS[1] + "\\\":0.02}\"") +
@@ -966,8 +995,9 @@ static RPCHelpMan sendmany()
966995

967996
std::vector<CRecipient> recipients;
968997
ParseRecipients(sendTo, subtractFeeFromAmount, recipients);
998+
bool verbose = request.params[10].isNull() ? false : request.params[10].get_bool();
969999

970-
return SendMoney(pwallet, coin_control, recipients, std::move(mapValue));
1000+
return SendMoney(pwallet, coin_control, recipients, std::move(mapValue), verbose);
9711001
},
9721002
};
9731003
}
@@ -4651,8 +4681,8 @@ static const CRPCCommand commands[] =
46514681
{ "wallet", "removeprunedfunds", &removeprunedfunds, {"txid"} },
46524682
{ "wallet", "rescanblockchain", &rescanblockchain, {"start_height", "stop_height"} },
46534683
{ "wallet", "send", &send, {"outputs","conf_target","estimate_mode","options"} },
4654-
{ "wallet", "sendmany", &sendmany, {"dummy","amounts","minconf","addlocked","comment","subtractfeefrom","use_is","use_cj","conf_target","estimate_mode"} },
4655-
{ "wallet", "sendtoaddress", &sendtoaddress, {"address","amount","comment","comment_to","subtractfeefromamount","use_is","use_cj","conf_target","estimate_mode", "avoid_reuse"} },
4684+
{ "wallet", "sendmany", &sendmany, {"dummy","amounts","minconf","addlocked","comment","subtractfeefrom","use_is","use_cj","conf_target","estimate_mode","verbose"} },
4685+
{ "wallet", "sendtoaddress", &sendtoaddress, {"address","amount","comment","comment_to","subtractfeefromamount","use_is","use_cj","conf_target","estimate_mode", "avoid_reuse","verbose"} },
46564686
{ "wallet", "sethdseed", &sethdseed, {"newkeypool","seed"} },
46574687
{ "wallet", "setcoinjoinrounds", &setcoinjoinrounds, {"rounds"} },
46584688
{ "wallet", "setcoinjoinamount", &setcoinjoinamount, {"amount"} },

‎src/wallet/test/coinjoin_tests.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,8 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
185185
}
186186
for (CAmount nAmount : vecAmounts) {
187187
CTransactionRef tx;
188-
BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, nFeeRet, nChangePosRet, strError, coinControl));
188+
FeeCalculation fee_calc_out;
189+
BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, nFeeRet, nChangePosRet, strError, coinControl, fee_calc_out));
189190
{
190191
LOCK2(wallet->cs_wallet, cs_main);
191192
wallet->CommitTransaction(tx, {}, {});

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,8 @@ class ListCoinsTestingSetup : public TestChain100Setup
602602
int changePos = -1;
603603
bilingual_str error;
604604
CCoinControl dummy;
605-
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy));
605+
FeeCalculation fee_calc_out;
606+
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy, fee_calc_out));
606607
wallet->CommitTransaction(tx, {}, {});
607608
CMutableTransaction blocktx;
608609
{
@@ -756,7 +757,8 @@ class CreateTransactionTestSetup : public TestChain100Setup
756757
int nChangePos = nChangePosRequest;
757758
bilingual_str strError;
758759

759-
bool fCreationSucceeded = wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePos, strError, coinControl);
760+
FeeCalculation fee_calc_out;
761+
bool fCreationSucceeded = wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePos, strError, coinControl, fee_calc_out);
760762
bool fHitMaxTries = strError.original == strExceededMaxTries;
761763
// This should never happen.
762764
if (fHitMaxTries) {
@@ -808,7 +810,8 @@ class CreateTransactionTestSetup : public TestChain100Setup
808810
int nChangePosRet = -1;
809811
bilingual_str strError;
810812
CCoinControl coinControl;
811-
BOOST_CHECK(wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePosRet, strError, coinControl));
813+
FeeCalculation fee_calc_out;
814+
BOOST_CHECK(wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePosRet, strError, coinControl, fee_calc_out));
812815
wallet->CommitTransaction(tx, {}, {});
813816
CMutableTransaction blocktx;
814817
{
@@ -1147,10 +1150,11 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup
11471150
int changePos = -1;
11481151
bilingual_str error;
11491152
CCoinControl dummy;
1153+
FeeCalculation fee_calc_out;
11501154
BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 2 * COIN, true /* subtract fee */}},
1151-
tx1, fee, changePos, error, dummy));
1155+
tx1, fee, changePos, error, dummy, fee_calc_out));
11521156
BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 1 * COIN, true /* subtract fee */}},
1153-
tx2, fee, changePos, error, dummy));
1157+
tx2, fee, changePos, error, dummy, fee_calc_out));
11541158
wallet->CommitTransaction(tx1, {}, {});
11551159
BOOST_CHECK_EQUAL(wallet->GetAvailableBalance(), 0);
11561160
CreateAndProcessBlock({CMutableTransaction(*tx2)}, GetScriptForRawPubKey({}));

‎src/wallet/wallet.cpp‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3080,7 +3080,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
30803080
LOCK(cs_wallet);
30813081

30823082
CTransactionRef tx_new;
3083-
if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, false, tx.vExtraPayload.size())) {
3083+
FeeCalculation fee_calc_out;
3084+
if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, fee_calc_out, false, tx.vExtraPayload.size())) {
30843085
return false;
30853086
}
30863087

@@ -3387,7 +3388,8 @@ bool CWallet::GetBudgetSystemCollateralTX(CTransactionRef& tx, uint256 hash, CAm
33873388
if (!outpoint.IsNull()) {
33883389
coinControl.Select(outpoint);
33893390
}
3390-
bool success = CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, error, coinControl);
3391+
FeeCalculation fee_calc_out;
3392+
bool success = CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, error, coinControl, fee_calc_out);
33913393
if(!success){
33923394
WalletLogPrintf("CWallet::GetBudgetSystemCollateralTX -- Error: %s\n", error.original);
33933395
return false;
@@ -3403,6 +3405,7 @@ bool CWallet::CreateTransactionInternal(
34033405
int& nChangePosInOut,
34043406
bilingual_str& error,
34053407
const CCoinControl& coin_control,
3408+
FeeCalculation& fee_calc_out,
34063409
bool sign,
34073410
int nExtraPayloadSize)
34083411
{
@@ -3761,6 +3764,7 @@ bool CWallet::CreateTransactionInternal(
37613764
// Before we return success, we assume any change key will be used to prevent
37623765
// accidental re-use.
37633766
reservedest.KeepDestination();
3767+
fee_calc_out = feeCalc;
37643768

37653769
WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u 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",
37663770
nFeeRet, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
@@ -3780,12 +3784,13 @@ bool CWallet::CreateTransaction(
37803784
int& nChangePosInOut,
37813785
bilingual_str& error,
37823786
const CCoinControl& coin_control,
3787+
FeeCalculation& fee_calc_out,
37833788
bool sign,
37843789
int nExtraPayloadSize)
37853790
{
37863791
int nChangePosIn = nChangePosInOut;
37873792
Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr)
3788-
bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, sign, nExtraPayloadSize);
3793+
bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign, nExtraPayloadSize);
37893794
// try with avoidpartialspends unless it's enabled already
37903795
if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
37913796
CCoinControl tmp_cc = coin_control;
@@ -3801,7 +3806,7 @@ bool CWallet::CreateTransaction(
38013806
CTransactionRef tx2;
38023807
int nChangePosInOut2 = nChangePosIn;
38033808
bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results
3804-
if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, sign, nExtraPayloadSize)) {
3809+
if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, fee_calc_out, sign, nExtraPayloadSize)) {
38053810
// if fee of this alternative one is within the range of the max fee, we use this one
38063811
const bool use_aps = nFeeRet2 <= nFeeRet + m_max_aps_fee;
38073812
WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, use_aps ? "grouped" : "non-grouped");

‎src/wallet/wallet.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -842,7 +842,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
842842
// ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure
843843
std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;
844844

845-
bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign, int nExtraPayloadSize);
845+
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, int nExtraPayloadSize);
846846

847847
public:
848848
/**
@@ -1133,7 +1133,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
11331133
* selected by SelectCoins(); Also create the change output, when needed
11341134
* @note passing nChangePosInOut as -1 will result in setting a random position
11351135
*/
1136-
bool CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign = true, int nExtraPayloadSize = 0);
1136+
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, int nExtraPayloadSize = 0);
11371137
/**
11381138
* Submit the transaction to the node's mempool and then relay to peers.
11391139
* Should be called after CreateTransaction unless you want to abort

‎test/functional/wallet_basic.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,17 @@ def run_test(self):
673673
assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
674674
assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))
675675

676+
self.log.info("Test send* RPCs with verbose=True")
677+
address = self.nodes[0].getnewaddress("test")
678+
txid_feeReason_one = self.nodes[2].sendtoaddress(address = address, amount = 5, verbose = True)
679+
assert_equal(txid_feeReason_one["fee_reason"], "Fallback fee")
680+
txid_feeReason_two = self.nodes[2].sendmany(dummy = '', amounts = {address: 5}, verbose = True)
681+
assert_equal(txid_feeReason_two["fee_reason"], "Fallback fee")
682+
self.log.info("Test send* RPCs with verbose=False")
683+
txid_feeReason_three = self.nodes[2].sendtoaddress(address = address, amount = 5, verbose = False)
684+
assert_equal(self.nodes[2].gettransaction(txid_feeReason_three)['txid'], txid_feeReason_three)
685+
txid_feeReason_four = self.nodes[2].sendmany(dummy = '', amounts = {address: 5}, verbose = False)
686+
assert_equal(self.nodes[2].gettransaction(txid_feeReason_four)['txid'], txid_feeReason_four)
676687

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

0 commit comments

Comments
 (0)