Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc/wallet: add simulaterawtransaction RPC #1016

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kallewoof
Copy link
Contributor

@kallewoof kallewoof commented Jul 6, 2021

This is an experimental feature for analyzing a given raw transaction, generating a mapping of what the wallet's balances will look like were the transaction to be signed and broadcast.

It uses GetDebit from wallet to determine incoming amounts and the unblinded transaction outputs to determine the outputs.

(Work here is on hold as upstream is progressing.)

Upstream: bitcoin/bitcoin#22751

@kallewoof kallewoof marked this pull request as draft July 6, 2021 07:38
@kallewoof kallewoof force-pushed the 202106-analyzerawtransaction branch 3 times, most recently from 31a1c2a to 6feb0f5 Compare July 7, 2021 06:09
@kallewoof kallewoof marked this pull request as ready for review July 7, 2021 07:00
@kallewoof kallewoof force-pushed the 202106-analyzerawtransaction branch 11 times, most recently from 3142409 to c745d75 Compare July 8, 2021 07:40
@apoelstra
Copy link
Member

concept ACK

Have you proposed this for Bitcoin? Curious if you've gotten any feedback there?

@kallewoof
Copy link
Contributor Author

This would be of limited use to bitcoin, I think, as it mostly becomes an issue when you are juggling multiple assets. (Maybe I'm too narrow-minded though. If there are actual use cases I would be happy to move upstream.)

@kallewoof
Copy link
Contributor Author

Also I don't see how the CI errors are related to this change (e.g. why would feature_taphash_pegins_issuances.py stop working because of a new RPC command that doesn't change anything anywhere?)

@apoelstra
Copy link
Member

Yeah I think they're caused by #1017

@apoelstra
Copy link
Member

@kallewoof how would you feel about rebasing this and changing it to work with PSETs rather than raw transactions?

@kallewoof
Copy link
Contributor Author

@apoelstra I'm actually making more progress upstream than I expected. There are two versions, one which is this renamed to simulaterawtransaction in bitcoin/bitcoin#22751 and one where an array of transactions is added to getbalances in bitcoin/bitcoin#22776.

Rewriting this to work with PS?Ts would be a downgrade I believe, as PS?Ts can be turned into raw transactions but raw transactions cannot be turned into PS?Ts.

@apoelstra
Copy link
Member

Rewriting this to work with PS?Ts would be a downgrade I believe, as PS?Ts can be turned into raw transactions but raw transactions cannot be turned into PS?Ts.

You can get a rawtx from a PSBT with finalizepsbt and get a PSBT from a rawtx with converttopsbt. PSBTs have strictly more information.

@kallewoof
Copy link
Contributor Author

Ah, I didn't realize that. If you can go both directions it doesn't seem hugely important which one is implemented, though. And since raw transactions are "closer" to what is actually put on chain (e.g. side-stepping bug(s) in PS?T code) raw seems sensible. Is there a specific reason why you prefer the other?

@kallewoof kallewoof changed the title rpc/wallet: add analyzerawtransaction RPC rpc/wallet: add simulaterawtransaction RPC Sep 21, 2021
@kallewoof kallewoof marked this pull request as draft September 21, 2021 23:05
@apoelstra
Copy link
Member

When you go from a PSBT to a raw tranaction then you lose a ton of data: information about the previosu UTXOs needed for signing, BIP32 derivation paths, EC signatures which have not yet been pulled into the actual witness bytes, (in Elements) blinding keys and ECDH keys and explicit data which is hidden in the actual transaction.

The rawtransaction API basically cannot be used for collaborative transaction creation and it is effectively deprecated in favor of PSBT.

@kallewoof
Copy link
Contributor Author

The rawtransaction API basically cannot be used for collaborative transaction creation and it is effectively deprecated in favor of PSBT.

I didn't realize this was the sentiment. Perhaps both would be ideal. I haven't done any PSBT stuff yet, but will look into it.

@kallewoof
Copy link
Contributor Author

kallewoof commented Sep 21, 2021

When you go from a PSBT to a raw tranaction then you lose a ton of data: information about the previosu UTXOs needed for signing, BIP32 derivation paths, EC signatures which have not yet been pulled into the actual witness bytes, (in Elements) blinding keys and ECDH keys and explicit data which is hidden in the actual transaction.

The way I envision this to be used is, you get a PSBT thrown at you, you convert it to raw tx, analyze (simulate) it, check that the result is what you expect it to be, then provide a signature for the PSBT in question.

Since you don't throw away that input you don't technically lose any information regardless of whether this is a raw tx command or a psbt command. That said, if people are of the sentiment that we're getting rid of raw tx entirely, then I should rework it for sure.

@apoelstra
Copy link
Member

I doubt we'll ever get rid of the raw transaction API, for backward compatibility and testing purposes. But I expect that users in multi-party transactions (where the ability to analyze transactions is most important) will always use PSET/PSBT rather than rawtxes, so the extraction would be an extra step for them (and the extraction may lose extra data, such as input values, that could be useful for analysis).

@kallewoof
Copy link
Contributor Author

I was going to say "there's already an analyzepsbt, maybe the functionality should go there", but then I realized it's non-wallet code. Anyway, thanks for the feedback, I will look into psbt'ifying.

achow101 added a commit to bitcoin-core/gui that referenced this pull request Aug 5, 2022
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 6, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants