send* RPCs in the wallet returns the "fee reason" #19501

stackman27 · 2020-07-12T20:04:49Z

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)

glozow · 2020-07-12T20:38:26Z

Concept ACK!

achow101

In order for cli to work, you will need to also add the new arguments to src/rpc/client.cpp.

src/wallet/feebumper.cpp

src/wallet/rpcwallet.cpp

DrahtBot · 2020-07-12T23:18:55Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: Remove duplicate name and argNames from CRPCCommand #20012 (rpc: Remove duplicate name and argNames from CRPCCommand by MarcoFalke)
wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418 (wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 by fjahr)

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.

maflcko

Concept ACK

src/wallet/rpcwallet.cpp

stackman27 · 2020-07-15T04:03:01Z

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.

glozow

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.

test/functional/wallet_basic.py

src/wallet/rpcwallet.cpp

fjahr · 2020-07-17T23:10:48Z

Concept ACK

Please squash the fixup commits.

stackman27 · 2020-07-18T04:07:10Z

Update: Fixed verbose information and also the log info in wallet_basic.py, also squashed all the nit commits.

fjahr · 2020-07-18T09:57:46Z

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.

test/functional/wallet_basic.py

stackman27 · 2020-07-18T20:02:34Z

Squashed nit commits to [test] send rpcs returns fee reason!

maflcko

ACK. Locks good beside feedback and https://github.com/bitcoin/bitcoin/pull/19501/files#r459066407

maflcko · 2020-09-26T10:23:41Z

src/wallet/rpcwallet.cpp

+                        {
+                            {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::BOOL, "fee reason", "The transaction fee reason."}


Suggested change

{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

maflcko · 2020-09-27T09:26:26Z

ACK a3fe42c

stackman27 · 2020-09-28T20:24:49Z

@luke-jr @instagibbs @laanwj @fjahr could you please verify whether I've resolved all the problems that you've highlighted? Thank you

instagibbs

ACK a3fe42c

instagibbs · 2020-09-28T20:46:38Z

test/functional/wallet_basic.py

+        txid_feeReason_one = self.nodes[2].sendtoaddress(address = address, amount = 10, verbose = True)
+        assert_equal(txid_feeReason_one["fee_reason"], "Fallback fee")
+        txid_feeReason_two = self.nodes[2].sendmany(dummy = '', amounts = {address: 10}, verbose = True)
+        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 👍

Added verbose = False case in test file

meshcollider

utACK 69cf5d4

meshcollider · 2020-09-29T11:43:16Z

src/wallet/rpcwallet.cpp

@@ -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();


micro-nit: spacing between false:

for some reason git is ignoring my whitespace changes. Any suggestions on how to fix that?

meshcollider · 2020-09-29T11:45:08Z

src/wallet/rpcwallet.cpp

+                },
+                {
+                    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"


micro-nit: double space between Only 1

instagibbs · 2020-09-29T15:25:05Z

ACK 69cf5d4

meshcollider · 2020-09-29T20:15:39Z

@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 😄

stackman27 · 2020-09-29T20:17:47Z

@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

maflcko · 2020-09-30T05:59:33Z

Mind force pushing the previous reviewed version?

git push your_remote 69cf5d4:send_rpc_feereason -f

?

stackman27 · 2020-09-30T06:04:45Z

Mind force pushing the previous reviewed version?
git push your_remote 69cf5d4:send_rpc_feereason -f
?

Yes just pushed!

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

stackman27 changed the title ~~"Send rpc feereason"~~ send* RPCs in the wallet returns the "fee reason" Jul 12, 2020

DrahtBot added RPC/REST/ZMQ Tests Wallet labels Jul 12, 2020

achow101 reviewed Jul 12, 2020

View reviewed changes

src/wallet/feebumper.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

This was referenced Jul 13, 2020

wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418

Merged

Use shared pointers only in validation interface #18354

Closed

The ultimate send RPC #16378

Merged

wallet: always do avoid partial spends if fees are within a specified range #14582

Merged

maflcko removed the Tests label Jul 13, 2020

maflcko approved these changes Jul 13, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

This was referenced Jul 13, 2020

Log RPC parameters (arguments) if -debug=rpcparams #16365

Closed

External signer support - Wallet Box edition #16546

Merged

glozow reviewed Jul 16, 2020

View reviewed changes

test/functional/wallet_basic.py Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 16, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

fjahr mentioned this pull request Jul 17, 2020

refactor: Add ParseBool to rpc/util #19544

Closed

stackman27 force-pushed the send_rpc_feereason branch from 4866d9b to 26c306c Compare July 18, 2020 04:05

stackman27 requested a review from achow101 July 18, 2020 04:10

fjahr reviewed Jul 18, 2020

View reviewed changes

test/functional/wallet_basic.py Outdated Show resolved Hide resolved

fjahr reviewed Jul 18, 2020

View reviewed changes

test/functional/wallet_basic.py Outdated Show resolved Hide resolved

stackman27 force-pushed the send_rpc_feereason branch from 26c306c to 05a8182 Compare July 18, 2020 20:01

stackman27 requested a review from fjahr July 18, 2020 20:02

stackman27 force-pushed the send_rpc_feereason branch from 15f7c1d to 729845d Compare July 20, 2020 03:37

stackman27 force-pushed the send_rpc_feereason branch from 82c8784 to c5d262f Compare September 24, 2020 20:28

DrahtBot removed the Needs rebase label Sep 24, 2020

stackman27 force-pushed the send_rpc_feereason branch 2 times, most recently from 099ac12 to 1d64009 Compare September 25, 2020 09:29

DrahtBot mentioned this pull request Sep 25, 2020

rpc: Remove duplicate name and argNames from CRPCCommand #20012

Merged

stackman27 requested a review from maflcko September 25, 2020 19:30

maflcko approved these changes Sep 26, 2020

View reviewed changes

[send] Make send RPCs return fee reason

d5863c0

stackman27 force-pushed the send_rpc_feereason branch 2 times, most recently from 19d86d2 to a3fe42c Compare September 27, 2020 02:03

instagibbs reviewed Sep 28, 2020

View reviewed changes

stackman27 force-pushed the send_rpc_feereason branch 4 times, most recently from 6d5e3ad to 6118e9d Compare September 28, 2020 21:55

[test] Make sure send rpc returns fee reason

69cf5d4

stackman27 force-pushed the send_rpc_feereason branch from 6118e9d to 69cf5d4 Compare September 28, 2020 22:05

meshcollider reviewed Sep 29, 2020

View reviewed changes

stackman27 force-pushed the send_rpc_feereason branch from 69cf5d4 to 09eb4d7 Compare September 29, 2020 19:23

stackman27 force-pushed the send_rpc_feereason branch from f84dc3a to 69cf5d4 Compare September 30, 2020 06:03

maflcko merged commit 1769828 into bitcoin:master Sep 30, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	{RPCResult::Type::BOOL, "fee reason", "The transaction fee reason."}
	{RPCResult::Type::STR, "fee reason", "The transaction fee reason."}

send* RPCs in the wallet returns the "fee reason" #19501

send* RPCs in the wallet returns the "fee reason" #19501

Uh oh!

Conversation

stackman27 commented Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Jul 12, 2020

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stackman27 commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjahr commented Jul 17, 2020

Uh oh!

stackman27 commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Jul 18, 2020

Uh oh!

Uh oh!

Uh oh!

stackman27 commented Jul 18, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

stackman27 Sep 27, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 27, 2020

Uh oh!

stackman27 commented Sep 28, 2020

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

stackman27 Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

stackman27 Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

meshcollider Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Sep 29, 2020

Uh oh!

meshcollider commented Sep 29, 2020

Uh oh!

stackman27 commented Jul 12, 2020 •

edited

Loading

DrahtBot commented Jul 12, 2020 •

edited

Loading

stackman27 commented Jul 15, 2020 •

edited

Loading

stackman27 commented Jul 18, 2020 •

edited

Loading