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
Concept ACK!
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;
nit: Our naming convention is to use 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);
I don't think we put spaces as the key in a json object as spaces tend to break things. Typically we do lowercase snake_case.
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."},
It would be preferable to have this text be generic instead of specific for the fee reason. Same for the other RPCs you've changed.
In order for cli to work, you will need to also add the new arguments to src/rpc/client.cpp.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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);
nit: I think this can be put in the (smaller) if (verbose) scope
Concept ACK
Added a new commit: added new arguments to 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")
nit: log message can be more informative, like "Test send* RPCs with verbose=True"
Also then comments are not that necessary since it's pretty self-explanatory what you're doing
Style-wise, if you keep comments, it's best for them to be complete sentences
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)"},
This is just a tad bit confusing, I'd recommend something like "If true, return the fee reason"
Er, something more generic would be "If true, return extra information about the transaction."
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.
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)
Linter fails here on the trailing whitespace.
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")
Also here trailing whitespace.
Squashed nit commits to [test] send rpcs returns fee reason!
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){
please follow the style: if (verbose) {
Small comment on the commit messages
* dbaf… [test] send rpcs returns fee reason (Sishir Giri) (pull/19501/head)
* 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{
The 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")
The result should already be a str so there's no need for str(...) here.
Resolved all the comments! Fixed travis and squash the nit commits!
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;
seems like these changes should be in the previous commit
Thoughts on turning 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)
Seems like it would work better to use FeeReason* here (or perhaps FeeCalculation*)
Are you recommending changing the pointer name from fee_reason to FeeReason?
He is referring to the enum FeeReason or the struct FeeCalculation to be used here instead of a string.
How would i go about that because i'd have to change everything that calls 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
Yes, it's not just a change in this line. The other places where you pass in a string now you will need to use the 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.
I changed the str fee reason to struct FeeCalculation
0 | @@ -0,0 +1,29 @@ 1 | +pick c321fe670 [send] Make send RPCs return fee reason
uhhh, this looks like an accidental file addition? :)
Fixed it
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, "", "",
yay for object instead of string result, can add more things going forward.
This will need to be two RPCResults (one for verbose=False and one for verbose=True). Refer to other places in the code base on how to do that.
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.
concept ACK @luke-jr Previously I was told due to named args we don't have to stick everything in an object argument.
Travis:
* Checking consistency between dispatch tables and vRPCConvertParams
The 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:
* Checking consistency between dispatch tables and vRPCConvertParams The 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.
Rebased and Changed str feereason to FeeCalculation*
Rebased and fixed all the changes
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);
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?
Changed the variable name from feecalc_feereason to `fee_calc_out'
Also added two RPCResult in sendtoaddress and sendmany
Looks good, but the feedback about the rpc documentation should be fixed
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."}
{RPCResult::Type::STR, "fee reason", "The transaction fee reason."}
code adjusted based on the feedback. Removed str from [test] and changed rpc result return to str
ACK. Locks good beside feedback and https://github.com/bitcoin/bitcoin/pull/19501/files#r459066407
ACK a3fe42c140008b7dbbef55b3d75c56d2c6343c43
@luke-jr @instagibbs @laanwj @fjahr could you please verify whether I've resolved all the problems that you've highlighted? Thank you
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")
maybe add a case where you explicitly pass in verbose = False to cover that case too :+1:
Added verbose = False case in test file
ACK a3fe42c140008b7dbbef55b3d75c56d2c6343c43
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();
micro-nit: spacing between false:
for some reason git is ignoring my whitespace changes. Any suggestions on how to fix that?
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"
micro-nit: double space between Only 1
utACK 69cf5d4eeb73f7d685e915fc17af64634d88a4a2
@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 :smile:
@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?
git push your_remote 69cf5d4:send_rpc_feereason -f
?
Mind force pushing the previous reviewed version?
git push your_remote 69cf5d4:send_rpc_feereason -f?
Yes just pushed!