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

Silent Payments: send and receive #27827

Closed

Conversation

josibake
Copy link
Member

@josibake josibake commented Jun 5, 2023

For reviewers

In an attempt to make reviewing a bit more sane, I'm breaking this up into a few smaller PRs, but will keep this one open as the parent PR and keep it rebased on the child PRs. The main purpose of having this PR is to track progress on child PRs and also have an easy way to compile bitcoind with both send and receive support for testing. Additionally, I'll be adding more functional tests to this PR since it's much easier to test when bitcoind can both send and receive.

PRs

  • Silent Payments: Implement BIP352 #28122
    • Implements the logic from BIP352 without any wallet code. This PR adds the necessary cryptographic functions and implements the logic needed for sending and scanning. This PR also includes the test vectors from the BIP as unit tests. Both the send and receive PRs have this as a dependency. In terms of priority, this PR should be reviewed first
  • Silent Payments: sending #28201
    • Implements sending in the Bitcoin Core wallet. This PR allows a wallet to send to a silent payment address, regardless of whether or not the wallet can receive silent payments
    • Ready for review, but marked as a draft until dependencies are merged
  • Silent Payments: receiving #28202
    • Implements receiving in the Bitcoin Core wallet. This PR allows a wallet to generate silent payment addresses and scan for silent payments, regardless of whether or not the wallet can send to a silent payment address
    • Ready for review but marked as a draft until dependencies are merged

For the silent payments specification, see bitcoin/bips#1458

Overall

This PR implements the full silent payments scheme: sending and receiving. The following items are not covered in this PR and are intended for follow-up PRs:

  • Adding labels for the receiver wallet
  • Full RPC coverage (only sendtoaddress and sendmany are covered in this PR)
  • Light client support (vending the tweak data per block, either in an index or to serve to indexer, such as electrum server)
  • Add benchmarks to validate that there are no DoS concerns for doing silent payment verification for transactions in the mempool
  • External signer support (dependent on hardware wallets supporting silent payments)
  • More unit / functional test coverage

Major changes

This PR is a continuation of the work done in #24897. Below is a summary of the major changes:

  • Remove labels
    • The original draft included labels, but this has been deferred for a later PR. Labels are not necessary for sending and receiving and there are still some open questions on how best to implement them in Bitcoin Core. Labels can also be added at any point by the receiver without requiring any changes from the sender
  • Remove indexes
    • In the original draft, indexes were used when scanning for silent payments and when doing wallet rescans. This has been removed in favor of using rev*.dat files for rescanning. It may make sense to add an index in the future, but for the purpose of vending tweak data to light clients, which is still an open question
  • Update to implement the most current version of BIP352
    • Since the original draft, there have been a few changes in the BIP which are reflected in the current PR. Most notably, using 33-byte compressed keys for the silent payment address (as opposed to X-only keys in the original draft)

It may be helpful for context to read through the discussions on #24897 , but ongoing review should happen in the relevant child PRs listed above.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 5, 2023

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
Approach ACK w0xlt

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:

  • #28453 (wallet: Receive silent payment transactions by achow101)
  • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
  • #28246 (wallet: Use CTxDestination in CRecipient instead of just scriptPubKey by achow101)
  • #28201 (Silent Payments: sending by josibake)
  • #28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
  • #28122 (Silent Payments: Implement BIP352 by josibake)
  • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
  • #27788 (rpc: Be able to access RPC parameters by name by achow101)
  • #27596 (assumeutxo (2) by jamesob)
  • #27351 (wallet: add seeds argument to importdescriptors by apoelstra)
  • #27307 (wallet: track mempool conflicts with wallet transactions by ishaanam)
  • #27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
  • #26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
  • #26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
  • #26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
  • #25907 (wallet: rpc to add automatically generated descriptors by achow101)
  • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
  • #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
  • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
  • #22341 (rpc: add getxpub by Sjors)

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.

@josibake josibake changed the title Silent Payments [DRAFT] Silent Payments Jun 5, 2023
@josibake josibake marked this pull request as ready for review June 5, 2023 13:31
@josibake josibake changed the title [DRAFT] Silent Payments [Silent Payments]: Base functionality Jun 5, 2023
@DrahtBot DrahtBot changed the title [Silent Payments]: Base functionality [Silent Payments]: Base functionality Jun 5, 2023
@josibake josibake force-pushed the silent-payments-base-pr-slim-down branch from ef70044 to ecc8b79 Compare June 5, 2023 13:50
This was referenced Jun 5, 2023
@josibake josibake force-pushed the silent-payments-base-pr-slim-down branch from ecc8b79 to 9f75c2e Compare June 6, 2023 12:49
@Sjors
Copy link
Member

Sjors commented Aug 3, 2023

Maybe add a quick summary in the description with the main implementation differences relative to #24897. It seems a big one is that this doesn't require an index!

You should also link from the child PRs back to this one.

@DrahtBot
Copy link
Contributor

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

@josibake
Copy link
Member Author

Closing in favor of #28536, where on-going tracking will be done.

To test, compile #28453 (has both send and receive support)

@josibake josibake closed this Sep 26, 2023
@josibake josibake deleted the silent-payments-base-pr-slim-down branch January 26, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants