rpc/wallet: add optional transaction(s) to getbalances #22776

kallewoof · 2021-08-23T04:13:17Z

Optional transactions provided to getbalances iterates over the inputs and outputs, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.

This is an alternative to #22751.

ghost · 2021-08-23T05:49:17Z

Concept ACK. Will try today.

meshcollider · 2021-08-23T06:14:58Z

Approach ACK, definitely prefer this over #22751 👍

maflcko · 2021-08-23T06:33:31Z

How is the user going to use this? Call the RPC thrice and compare the results?

Fist time to get the balances before
Second time to get the balances with the tx included
Third time to verify no other thread added a wallet tx in-between, which would invalidate the result

kallewoof · 2021-08-23T07:23:06Z

@MarcoFalke The transaction is supposedly not yet broadcasted. They'd only need to call getbalances once. The existing output would give the wallet's balance as it is (excluding the transaction), and the added "changes" would show how the balance would change, if the transaction was broadcasted.

I.e. I get a coin-join with 100 other inputs and a bunch of outputs. I call getbalances [hex], and the "changes" column tells me how much I gain/lose from broadcasting the transaction.

maflcko · 2021-08-23T07:31:20Z

Oh I didn't see the changed result, because the documentation wasn't updated.

kallewoof · 2021-08-23T07:38:50Z

Good point, documentation updated.

DrahtBot · 2021-08-23T13:32:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23813 (Add test and docs for getblockfrompeer by fjahr)
#23706 (rpc: getblockfrompeer followups by Sjors)
#23532 (test: add functional test for -startupnotify by brunoerg)
#22751 (rpc/wallet: add simulaterawtransaction RPC by kallewoof)

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.

ghost

tACK 67ff5ae

Tested with different transactions (input from wallet, input not belonging to wallet, outputs from wallet, outputs outside wallet)

Example:

Request:

POST /wallet/W2 HTTP/1.1
Host: 127.0.0.1:18333
Authorization: Basic dXNlcjMzOnBhc3N3b3JkMzM=
Content-Type: text/plain
Content-Length: 521

{"jsonrpc": "1.0", "id": "curltest", "method": "getbalances", "params": ["02000000000101fff8d9d39105eeec162fb45bd2a3e796b255866469a2cc7de93e438f3722c1490000000000fdffffff02801a0600000000001600145b497cfd64324fb0c4f7317472ca591cd7eb3bbfe093040000000000160014b1348ec3acd422b7c84f97f8757a0a5c3db9e32d02473044022020c3a9d7d06429008de82f13de672aac7886c5d6557a7c330bcb8cbc20b8cf4f02203890042ed8bf331e92f07df5d2a7bf5d8ade12cbb216f14493a98d0a188c4918012103c65a6e4a66a4a6b4af981c5e81632bb2d7336bc75d7ccf83e055006a2693050f00000000"]}

Response:

{
    "result": {
        "mine": {
            "trusted": 5051.31588006,
            "untrusted_pending": 0.00000000,
            "immature": 0.00000000
        },
        "tx": {
            "changes": 0.00000000
        }
    },
    "error": null,
    "id": "curltest"
}

Everything looks good to me. Also tested by fuzzing this argument for getbalances with random strings. No issues.

achow101 · 2021-08-25T23:54:13Z

-0 on this.

As I commented in #22751 (comment), I would prefer a broader "see how transactions modify the wallet without actually modifying it" RPC rather than trying to jam that functionality into an existing one.

But if this is the approach that others prefer, then I would also prefer this to accept multiple transactions (as an array) rather than a single transaction. It is conceivable that a user would want to see how multiple transactions would modify their wallet at the same time, e.g. when broadcasting a package.

kallewoof · 2021-08-26T05:32:16Z

Switched to array of raw transactions.

I am neutral on whether to add this here or create a new simulaterawtransaction as in #22751.

luke-jr · 2021-09-20T01:45:27Z

#22751 seems to make more sense IMO

If users need both pieces of information, they can just batch them.

luke-jr · 2021-09-20T01:46:21Z

src/wallet/rpcwallet.cpp

It seems weird to have an array of raw transactions as a positional argument here.

kallewoof · 2021-09-21T10:54:13Z

I think #22751 is emerging as the superior option here. Ping if you disagree and I'll reopen this.

kallewoof · 2021-09-21T23:53:02Z

Some prefer this over #22751, so reopening this for now.

test/functional/wallet_balance_getbalances.py

One or several optional transactions provided to getbalances iterates over the inputs and outputs, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.

laanwj · 2021-12-17T15:02:06Z

I would prefer a broader "see how transactions modify the wallet without actually modifying it" RPC rather than trying to jam that functionality into an existing one.

I tend to agree here. It is useful functionality but I think it's cleaner to have it separated as a seperate RPC.

DrahtBot · 2022-01-03T08:40:33Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2022-07-25T07:03:23Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

db10cf8 rpc/wallet: add simulaterawtransaction RPC (Karl-Johan Alm) 701a64f test: add support for Decimal to assert_approx (Karl-Johan Alm) Pull request description: (note: this was originally titled "add analyzerawtransaction RPC") This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally. I originally proposed this to Elements (ElementsProject/elements#1016) and it was suggested that I propose this upstream. There is an alternative #22776 to instead add this info to `getbalances` when providing an optional transaction as argument. ACKs for top commit: jonatack: ACK db10cf8 achow101: re-ACK db10cf8 Tree-SHA512: adf222ec7dcdc068d007ae6f465dbc35b692dc7bb2db337be25340ad0c2f9c64cfab4124df23400995c700f41c83c29a2c34812121782c26063b100c7969b89d

db10cf8 rpc/wallet: add simulaterawtransaction RPC (Karl-Johan Alm) 701a64f test: add support for Decimal to assert_approx (Karl-Johan Alm) Pull request description: (note: this was originally titled "add analyzerawtransaction RPC") This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally. I originally proposed this to Elements (ElementsProject/elements#1016) and it was suggested that I propose this upstream. There is an alternative bitcoin#22776 to instead add this info to `getbalances` when providing an optional transaction as argument. ACKs for top commit: jonatack: ACK db10cf8 achow101: re-ACK db10cf8 Tree-SHA512: adf222ec7dcdc068d007ae6f465dbc35b692dc7bb2db337be25340ad0c2f9c64cfab4124df23400995c700f41c83c29a2c34812121782c26063b100c7969b89d

fanquake added the RPC/REST/ZMQ label Aug 23, 2021

kallewoof mentioned this pull request Aug 23, 2021

rpc/wallet: add simulaterawtransaction RPC #22751

Merged

kallewoof force-pushed the 202108-getbalances-tx branch from 127e3bd to 92fe092 Compare August 23, 2021 04:26

kallewoof force-pushed the 202108-getbalances-tx branch from 92fe092 to 1a1fef9 Compare August 23, 2021 06:01

kallewoof force-pushed the 202108-getbalances-tx branch from 1a1fef9 to 67ff5ae Compare August 23, 2021 07:38

ghost approved these changes Aug 23, 2021

View reviewed changes

meshcollider added the Wallet label Aug 24, 2021

kallewoof force-pushed the 202108-getbalances-tx branch from 67ff5ae to d14266d Compare August 26, 2021 05:31

kallewoof force-pushed the 202108-getbalances-tx branch from d14266d to 2c4e77e Compare August 26, 2021 06:18

kallewoof changed the title ~~rpc/wallet: add optional transaction to getbalances~~ rpc/wallet: add optional transaction(s) to getbalances Aug 26, 2021

DrahtBot mentioned this pull request Aug 27, 2021

rpc: getblockfrompeer #20295

Merged

luke-jr suggested changes Sep 20, 2021

View reviewed changes

src/wallet/rpcwallet.cpp Outdated

Copy link

Member

luke-jr Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems weird to have an array of raw transactions as a positional argument here.

kallewoof mentioned this pull request Sep 20, 2021

rpc/wallet: add simulaterawtransaction RPC ElementsProject/elements#1016

Closed

kallewoof closed this Sep 21, 2021

kallewoof reopened this Sep 21, 2021

maflcko reviewed Nov 9, 2021

View reviewed changes

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

kallewoof force-pushed the 202108-getbalances-tx branch from 2c4e77e to f7c4f88 Compare November 14, 2021 12:05

DrahtBot mentioned this pull request Nov 17, 2021

test: add functional test for -startupnotify #23532

Merged

This was referenced Dec 2, 2021

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

Split up rpcwallet #23667

Merged

DrahtBot added the Needs rebase label Dec 8, 2021

kallewoof force-pushed the 202108-getbalances-tx branch from f7c4f88 to e63ec4c Compare December 9, 2021 04:48

DrahtBot removed the Needs rebase label Dec 9, 2021

DrahtBot mentioned this pull request Dec 11, 2021

rpc: getblockfrompeer followups #23706

Merged

DrahtBot mentioned this pull request Dec 21, 2021

Add test and docs for getblockfrompeer with pruning #23813

Merged

DrahtBot added the Needs rebase label Jan 3, 2022

kallewoof marked this pull request as draft February 1, 2022 07:08

kallewoof closed this Jul 25, 2022

bitcoin locked and limited conversation to collaborators Jul 25, 2023

rpc/wallet: add optional transaction(s) to getbalances #22776

rpc/wallet: add optional transaction(s) to getbalances #22776

Uh oh!

Conversation

kallewoof commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 23, 2021

Uh oh!

meshcollider commented Aug 23, 2021

Uh oh!

maflcko commented Aug 23, 2021

Uh oh!

kallewoof commented Aug 23, 2021

Uh oh!

maflcko commented Aug 23, 2021

Uh oh!

kallewoof commented Aug 23, 2021

Uh oh!

DrahtBot commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 25, 2021

Uh oh!

kallewoof commented Aug 26, 2021

Uh oh!

luke-jr commented Sep 20, 2021

Uh oh!

luke-jr Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Sep 21, 2021

Uh oh!

kallewoof commented Sep 21, 2021

Uh oh!

Uh oh!

laanwj commented Dec 17, 2021

Uh oh!

DrahtBot commented Jan 3, 2022

Uh oh!

DrahtBot commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kallewoof commented Aug 23, 2021 •

edited

Loading

DrahtBot commented Aug 23, 2021 •

edited

Loading