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: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
218@@ -219,7 +219,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
219 CAmount fee_ret;
220 int change_pos_in_out = -1; // No requested location for change
221 bilingual_str fail_reason;
222- if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
223+ std::string feeReason;
snake_case rather than camelCase.
410 }
411 pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */);
412+
413+ if(verbose){
414+ entry.pushKV("hex", tx->GetHash().GetHex());
415+ entry.pushKV("Fee Reason", feeReason);
438@@ -430,6 +439,8 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
439 " \"" + FeeModes("\"\n\"") + "\""},
440 {"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"
441 " dirty if they have previously been used in a transaction."},
442+ {"verbose", RPCArg::Type::BOOL, /* default */ "false",
443+ "Whether to display the fee reason or not."},
src/rpc/client.cpp.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
400@@ -401,11 +401,20 @@ UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std
401 int nChangePosRet = -1;
402 bilingual_str error;
403 CTransactionRef tx;
404- bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
405+ UniValue entry(UniValue::VOBJ);
if (verbose) scope
src/rpc/client.cpp , changed the case to snake_case, generalized the verbose description and added entry object inside verbose if.
652@@ -653,6 +653,16 @@ def run_test(self):
653 assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
654 assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))
655
656+ self.log.info("Testing Fee Reason")
438@@ -430,6 +439,8 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
439 " \"" + FeeModes("\"\n\"") + "\""},
440 {"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"
441 " dirty if they have previously been used in a transaction."},
442+ {"verbose", RPCArg::Type::BOOL, /* default */ "false",
443+ "Optional, the fee reason (only present when `verbose` is passed)"},
utACK
Travis is failing on the linter though, most likely whitespace. You can run it yourself with test/lint/lint-all.sh (you might need to install a few packages). I left a bunch of nits that you can include with your changes, and then please rebase and squash.
Concept ACK
Please squash the fixup commits.
Update: Fixed verbose information and also the log info in wallet_basic.py, also squashed all the nit commits.
Thank you! But I meant to squash the fix-ups into the original commits. Otherwise, it’s harder to review because reviewers can not just go through the commits in order but need to jump back and forth to check if what they see in the first commit is not already changed in a later commit. So, for example fee_reason should be already used in the first commit.
658+ address = self.nodes[0].getnewaddress("test")
659+ txid_feeReason_one = self.nodes[2].sendtoaddress(address = address, amount = 10, verbose = True)
660+ assert_equal(str(txid_feeReason_one["Fee Reason"]), "Fallback fee")
661+
662+ #testing send_rpc_verbose sendtoaddress
663+ txid_feeReason_two = self.nodes[2].sendmany(dummy = '', amounts = {address: 10}, verbose = True)
652@@ -653,6 +653,16 @@ def run_test(self):
653 assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
654 assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))
655
656+ self.log.info("Testing Fee Reason")
657+ #testing send_rpc_verbose sendtoaddress
658+ address = self.nodes[0].getnewaddress("test")
408 if (!fCreated) {
409 throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
410 }
411 pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */);
412+
413+ if(verbose){
if (verbose) {
Small comment on the commit messages
0* dbaf… [test] send rpcs returns fee reason (Sishir Giri) (pull/19501/head)
1* 7298… [send] send rpcs returns fee reason (Sishir Giri)
I think, instead of using the same message with a different prefix, it would be clearer to make it:
Also we don’t have a category [send] so make it [rpc] :smile:
410 }
411 pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */);
412+
413+ if(verbose){
414+ UniValue entry(UniValue::VOBJ);
415+ entry.pushKV("hex", tx->GetHash().GetHex());
hex usually implies the full raw transaction. This is just a txid, so this should instead be named txid.
438@@ -430,6 +439,8 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
439 " \"" + FeeModes("\"\n\"") + "\""},
440 {"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"
441 " dirty if they have previously been used in a transaction."},
442+ {"verbose", RPCArg::Type::BOOL, /* default */ "false",
443+ "Optional, If true, return extra information about the transaction."},
444 },
445 RPCResult{
RPCResult will need to be updated to include the verbose result too.
This isn’t quite what I was looking for.
There are 2 possible results, so we need to have different RPC results for them. getrawtransaction has a similar verbose result. I would suggest looking at that to see how this should be done.
652@@ -653,5 +653,12 @@ def run_test(self):
653 assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
654 assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))
655
656+ self.log.info("Test send* RPCs with verbose=True")
657+ address = self.nodes[0].getnewaddress("test")
658+ txid_feeReason_one = self.nodes[2].sendtoaddress(address = address, amount = 10, verbose = True)
659+ assert_equal(str(txid_feeReason_one["fee_reason"]), "Fallback fee")
str so there’s no need for str(...) here.
412- if(verbose){
413+ if (verbose) {
414 UniValue entry(UniValue::VOBJ);
415- entry.pushKV("hex", tx->GetHash().GetHex());
416- entry.pushKV("fee_reason", fee_reason);
417- return entry;
sendmany param 1/dummy into an options object? (And not including this functionality in sendtoaddress)
2701@@ -2701,7 +2702,7 @@ OutputType CWallet::TransactionChangeType(const Optional<OutputType>& change_typ
2702 return m_default_address_type;
2703 }
2704
2705-bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign)
2706+bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, std::string& fee_reason, bool sign)
FeeReason* here (or perhaps FeeCalculation*)
FeeReason or the struct FeeCalculation to be used here instead of a string.
createTransaction and also the UniValue entry string type? I’m just a bit confused because when I change its type to FeeReason* , I get bombarded with errors
FeeReason* type as well (as you have already noticed). You also won’t convert the reason to a string in this function but where you actually need it as a string. But most importantly: take your time to and try to understand these errors. Once you understand them they can guide you to the right solution.
str fee reason to struct FeeCalculation
0@@ -0,0 +1,29 @@
1+pick c321fe670 [send] Make send RPCs return fee reason
844@@ -831,10 +845,16 @@ static UniValue sendmany(const JSONRPCRequest& request)
845 {"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)"},
846 {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
847 " \"" + FeeModes("\"\n\"") + "\""},
848+ {"verbose", RPCArg::Type::BOOL, /* default */ "false",
849+ "Optional, If true, return extra information about the transaction."},
s/Optional, // no need to say optional.
436@@ -430,9 +437,15 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
437 " \"" + FeeModes("\"\n\"") + "\""},
438 {"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"
439 " dirty if they have previously been used in a transaction."},
440+ {"verbose", RPCArg::Type::BOOL, /* default */ "false",
441+ "Optional, If true, return extra information about the transaction."},
442 },
443 RPCResult{
444- RPCResult::Type::STR_HEX, "txid", "The transaction id."
445+ RPCResult::Type::OBJ, "", "",
436@@ -430,9 +437,15 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
437 " \"" + FeeModes("\"\n\"") + "\""},
438 {"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"
439 " dirty if they have previously been used in a transaction."},
440+ {"verbose", RPCArg::Type::BOOL, /* default */ "false",
441+ "Optional, If true, return extra information about the transaction."},
s/Optional, // no need to say optional.
Travis:
0* Checking consistency between dispatch tables and vRPCConvertParams
1
2The subject line of commit hash c8992e89594a54edf283e4916f794475070b5114 is followed by a non-empty line. Subject lines should always be followed by a blank line.
(you need an empty line between the commit title and the body)
Travis:
0* Checking consistency between dispatch tables and vRPCConvertParams 1 2The subject line of commit hash c8992e89594a54edf283e4916f794475070b5114 is followed by a non-empty line. Subject lines should always be followed by a blank line.(you need an empty line between the commit title and the body)
It’s not his fault, the new linter was buggy. The offending commit https://github.com/bitcoin/bitcoin/commit/c8992e89594a54edf283e4916f794475070b5114 is not part of this PR. Should be fixed with #19654. I restarted the job and it should pass that linter.
str feereason to FeeCalculation*
973@@ -974,7 +974,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
974 * selected by SelectCoins(); Also create the change output, when needed
975 * @note passing nChangePosInOut as -1 will result in setting a random position
976 */
977- bool CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, bool sign = true);
978+ bool CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& feecalc_feereason, bool sign = true);
0 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);
Could rename to fee_calc_out to clarify that this is an out param?
feecalc_feereason to `fee_calc_out'
sendtoaddress and sendmany
881+ RPCResult{"if verbose is set to true",
882+ RPCResult::Type::OBJ, "", "",
883+ {
884+ {RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
885+ "the number of addresses."},
886+ {RPCResult::Type::BOOL, "fee reason", "The transaction fee reason."}
0 {RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
str from [test] and changed rpc result return to str
660@@ -661,5 +661,13 @@ def run_test(self):
661 assert_array_result(tx["details"], {"category": "receive"}, expected_receive_vout)
662 assert_equal(tx[verbose_field], self.nodes[0].decoderawtransaction(tx["hex"]))
663
664+ self.log.info("Test send* RPCs with verbose=True")
665+ address = self.nodes[0].getnewaddress("test")
666+ txid_feeReason_one = self.nodes[2].sendtoaddress(address = address, amount = 10, verbose = True)
667+ assert_equal(txid_feeReason_one["fee_reason"], "Fallback fee")
668+ txid_feeReason_two = self.nodes[2].sendmany(dummy = '', amounts = {address: 10}, verbose = True)
669+ assert_equal(txid_feeReason_two["fee_reason"], "Fallback fee")
verbose = False to cover that case too :+1:
verbose = False case in test file
513@@ -497,8 +514,9 @@ static RPCHelpMan sendtoaddress()
514
515 std::vector<CRecipient> recipients;
516 ParseRecipients(address_amounts, subtractFeeFromAmount, recipients);
517+ bool verbose = request.params[9].isNull() ? false: request.params[9].get_bool();
false:
870@@ -853,11 +871,22 @@ static RPCHelpMan sendmany()
871 {"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)"},
872 {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
873 " \"" + FeeModes("\"\n\"") + "\""},
874+ {"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra infomration about the transaction."},
875+ },
876+ {
877+ RPCResult{"if verbose is not set or set to false",
878+ RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n"
Only 1
@stackman27 if there are a couple of ACKs then it’s better not to fix small nits and leave them in case there’s a bigger change that needs to be done. Small spacing nits, etc. won’t stop a merge 😄
Okay for sure! Thank you
Mind force pushing the previous reviewed version?
0git push your_remote 69cf5d4:send_rpc_feereason -f
?
Mind force pushing the previous reviewed version?
0git push your_remote 69cf5d4:send_rpc_feereason -f?
Yes just pushed!