Skip to content

[Draft / POC] Silent Payments - #24897

Closed
w0xlt wants to merge 19 commits into
bitcoin:masterfrom
w0xlt:silent_payment_021
Closed

[Draft / POC] Silent Payments#24897
w0xlt wants to merge 19 commits into
bitcoin:masterfrom
w0xlt:silent_payment_021

Conversation

@w0xlt

@w0xlt w0xlt commented Apr 17, 2022

Copy link
Copy Markdown
Contributor

This PR proposes an early version of Silent Payment (author:@RubenSomsen).
In this scheme, the recipient generates a public address, but the sender tweaks the address and the recipient detects the payment by verifying all transactions on the blockchain. An example use case would be private donations.

The purpose of this PR is not a final version, but to start the discussion and get benchmarks based on a real implementation.

This version is built on top of #994 (bitcoin-core/secp256k1) for x-only ECDH support and #23480 (bitcoin/bitcoin) for rawtr(). Each new silent transaction detected is stored in wallet as a rawtr() descriptor.

In this implementation, the sender can tweak the recipient address by passing the silent_payment option to send RPC. The transaction output will be different from the address entered.

For example ./src/bitcoin-cli -regtest -named send outputs="[{\"bcrt1pwlh5xuyrpgfunwyww8cfu78yfs2yqyevl7yturavahh5kgxwdd2q5hzgfu\": 1.1}]" fee_rate=1 options="{ \"silent_payment\": true}".

will generate vout with completely unrelated outputs:

"vout": [
    {
      "value": 1.10000000,
      "n": 0,
      "scriptPubKey": {
        "desc": "rawtr(65b19890c5ca40edb816d26f5f48cd9f3ed51121613b1c2405adc1a6dbbc824a)#8myx9tcu",
        "address": "bcrt1pvkce3yx9efqwmwqk6fh47jxdnuld2yfpvya3cfq94hq6dkausf9qrfjkgz",

      }
    },
    {
      "value": 2.02499835,
      "n": 1,
      "scriptPubKey": {
        "desc": "rawtr(c45cb3d500bbf8f0c8841e8e011b008781d826c16ee348edb822c0f97419bc4d)#26hcce63",
        "address": "bcrt1pc3wt84gqh0u0pjyyr68qzxcqs7qasfkpdm353mdcytq0jaqeh3xsuvlykg",
      }
    }
  ]

Any wallet, as long as it has access to private keys, can send silent payments. Thus, this excludes watch-only wallets or wallets with external signers .

But the recipient's wallet needs a new flag called SILENT_PAYMENT. This flag allows an additional scan that verifies that the wallet keys match the silent payment scheme. When it detects a silent payment that belongs to the wallet, it is stored in a rawtr() descriptor.

./src/bitcoin-cli -regtest -named createwallet wallet_name="recipient" silent_payment=true

Therefore, scanning each address for each transaction is potentially prohibitive overhead, so the node can be initialized with keypool=1 or a descriptor with range [0,1] can be imported into a blank wallet. Until there is more benchmark data, it is the safest option. The proposal recommends one static address.

I've been running some silent payments on signet using wallets with default keypool and default range, I haven't noticed any relevant performance drops on the signet node.
Apparently this implementation is working as expected but I can't guarantee that the scheme is implemented correctly or safely, so I'm opening this PR for reviews, modifications and improvements.

There is a new functional test (test/functional/wallet_silentpayment.py) that can help to better understand the implementation.

@DrahtBot

DrahtBot commented Apr 17, 2022

Copy link
Copy Markdown
Contributor

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

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK ghost, rajarshimaitra, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #bitcoin-core/gui/650 (Add Import to Wallet GUI by KolbyML)
  • #27215 (wallet: Turn destdata entries into CAddressBookData fields by achow101)
  • #26951 (wallet: improve rescan performance 640% by pstratem)
  • #26858 (wallet: Use defined purposes instead of inlining by aureleoules)
  • #26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
  • #26836 (wallet: finish addressbook encapsulation by furszy)
  • #26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
  • #26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)
  • #26129 (wallet, refactor: FundTransaction(): return out-params as util::Result structure by theStack)
  • #26094 (rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo by aureleoules)
  • #26076 (Switch hardened derivation marker to h (in normalized descriptors and new wallets) by Sjors)
  • #26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
  • #25991 (Wallet: Add foreign_outputs metadata to support CoinJoin transactions by luke-jr)
  • #25907 (wallet: rpc to add automatically generated descriptors by achow101)
  • #25806 (wallet: group outputs only once, decouple it from Coin Selection by furszy)
  • #25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
  • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
  • #25634 (wallet, tests: Expand and test when the blank wallet flag should be un/set by achow101)
  • #25620 (wallet: Introduce AddressBookManager by furszy)
  • #25297 (wallet: group independent db writes on single batched db transaction by furszy)
  • #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
  • #24914 (wallet: Load database records in a particular order by achow101)
  • #23561 (BIP324: Handshake prerequisites by dhruv)
  • #23432 (BIP324: CKey encode/decode to elligator-swift by dhruv)
  • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
  • #22341 (rpc: add getxpub by Sjors)
  • #19690 (util: improve FindByte() performance by LarryRuane)
  • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
  • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
  • #10102 (Multiprocess bitcoin by ryanofsky)

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

ghost commented Apr 18, 2022

Copy link
Copy Markdown

Concept ACK

Comment thread src/wallet/silentpayment.cpp Outdated
@luke-jr

luke-jr commented Apr 19, 2022

Copy link
Copy Markdown
Contributor

Need some way to avoid users trying to send a silent payment to a wallet that doesn't support it... am I missing something?

@w0xlt

w0xlt commented Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

@luke-jr Yes, it will need to be further discussed if this concept proves to be valid.

In this PR, all descriptor wallets support silent transactions as the SILENT_PAYMENT flag can be enabled or disabled at any time. The rescanblockchain RPC can be used to retrieve previous silent payments.

As only Taproot addresses can be used for silent payments and only descriptor wallets support Taproot, this may suffice, but it certainly needs further discussion.

@RubenSomsen

Copy link
Copy Markdown

Need some way to avoid users trying to send a silent payment to a wallet that doesn't support it

@luke-jr When the recipient publishes their silent payment address, it needs to be clear that it is not a regular taproot address. The address format should be distinct. The tweaked silent payment address that is generated by the sender and is ultimately committed on-chain should be indistinguishable from a regular taproot output.

only Taproot addresses can be used for silent payments

@w0xlt Note that nothing about the silent payment scheme is exclusive to taproot, but regardless I do think it's good to limit the specifications to creating taproot outputs only as it encourages taproot use and means transactions without taproot outputs (albeit hopefully rare in the eventual future) don't need to be scanned.

@luke-jr

luke-jr commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

When the recipient publishes their silent payment address, it needs to be clear that it is not a regular taproot address. The address format should be distinct.

But I don't see that in this current spec/code?

@DrahtBot

DrahtBot commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

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

@Sjors Sjors left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't forget to rebase. The bech32 hrp stuff moved to kernel/chainparams.{h,cpp}.

The silentpayment index has been removed from the codebase. … A new PR, with the index, will be added soon.

That's nice. It's more important to prove functionality, can improve performance later.

The rawtr() functionality has been merged, so the dependency can be removed from the PR description. Either this PR description or a BIP should remind the reader why rawtr() is necessary (you can't know the untweaked key) and safe (there's no way for the sender to unilaterally add a hidden script path?).

Enabling the ECDH module in libsecp256k1 might be a reason to put silent payments behind a configure flag initially. Then again, we also need it for BIP324 P2P encryption #23561 and I'm not seeing any discussion there about enabling the module either. That PR also introduces fuzz tests, so maybe it's worth combining efforts and getting those merged jointly.

Most of the commits here are reasonably short. It might be worth splitting f0f9817f44fb8dc43c1f8ca0a456fcd3780350af a bit further.

I would suggest moving PSBT and light client support to a followup PR. It's important to prove that it works, but that can be done by just keeping it rebased on this PR with the minimum functionality.

Comment thread src/outputtype.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

3742cc75571a60bda52a41923c8244535af9ff23: maybe move that above UNKNOWN?

Comment thread src/wallet/rpc/backup.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

9143c7e1f1710f123072cc272b66ab2cad0eb57a: this commit won't compile, probably needs something from a later commit

Comment thread src/wallet/rpc/addresses.cpp Outdated

@Sjors Sjors Apr 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The term "identity" is confusing here. It's also confusing that you get this error when you generate a new silent payment address, but not when you request an existing one (by reusing a label).

I guess what you're trying to say here is that an outside observer can tell that two silent payment addresses belong to the same person? Maybe just say that:

The silent payment addresses are unique for accounting purposes only. They can be linked to the same wallet. For privacy a new wallet is required.

That said, I think this feature is a footgun. Also because poor coin control will eventually link funds from all labels.

@Sjors

Sjors commented Apr 7, 2023

Copy link
Copy Markdown
Member

Silent watch-only wallets have not yet been implemented. Some architectural improvements are needed first.

This would be nice though, maybe in a followup. I was trying to test it with bitcoin-cli -signet -named createwallet wallet_name=SilentWatcher silent_payment=true disable_private_keys=true and then import a descriptor like sp(…).

I guess one thing you need for that is the ability to dump the watch key. You could add an argument scan_privkey to decodesilentaddress which would return the private, rather the public. Ideally in the form of a sp(…) descriptor ready for import (and a warning that leaking that descriptor hurts your privacy, though coins can't be stolen)

Finally I noticed that calling listdescriptors true on the wallet reveals only 1 private key for the sp() descriptor; I guess because they are derived from each other?

@josibake josibake mentioned this pull request Jun 5, 2023
3 tasks
@josibake

josibake commented Jun 5, 2023

Copy link
Copy Markdown
Member

I've rebased and created a slimmed-down version of this PR in #27827. Most notably, I took out labels and removed some of the RPC support, and made several updates as the silent payments spec has changed a bit since this PR was last updated. For anyone reviewing this PR, I'd appreciate your feedback on #27827.

Big thanks to @w0xlt for moving this draft along as far as they did! My plan is to keep cherry-picking commits from this draft for follow-up PRs to add back in label support, RPC coverage, etc

@fanquake

fanquake commented Jun 5, 2023

Copy link
Copy Markdown
Member

Closing this, in favour of #27827.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.