Skip to content

wallet: Receive silent payment transactions #28453

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

Closed
wants to merge 47 commits into from

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Sep 11, 2023

This PR implements receiving silent payment transactions through the use of a new ScriptPubKeyMan type: SilentPaymentsSPKM. This is a descriptor wallet only feature, although it does not use descriptors.

This is an optional feature, so the only way to use it is to create a new wallet with createwallet with silent_payments=true. Such wallets will have a single SilentPaymentsSPKM that is used for both receiving and change. Since silent payments only have a single address, repeated calls to getnewaddress and getrawchangeaddress always return the same address, however the receiving and change addresses are different, see the BIP for how the change is generated.

Since silent payments requires the spent coins in order to extract public keys from them, TransactionAddedToMempool is modified to also provide that information. For scanning blocks, the wallet will retrieve that information from the undo data. Additionally, when rescanning, a silent payments wallet must use the slow rescan method.

The labels feature has not been fully implemented yet and is left for a followup.

Based on #28122 and #28201

Alternative to #28202

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 11, 2023

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Approach ACK w0xlt
Approach NACK luke-jr

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/754 (Add BIP324-specific labels to peer details by hebasto)
  • #28560 (wallet, rpc: FundTransaction refactor by josibake)
  • #28553 ([do not merge] validation: assumeutxo params mainnet by Sjors)
  • #28391 (refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access by TheCharlatan)
  • #28368 (Fee Estimator updates from Validation Interface/CScheduler thread by ismaelsadeeq)
  • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
  • #28331 (BIP324 integration by sipa)
  • #28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
  • #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)
  • #27260 (Enhanced error messages for invalid network prefix during address parsing. by russeree)
  • #27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
  • #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)
  • #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)
  • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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 and others added 21 commits October 3, 2023 13:51
`CreateSilentPaymentsOutputs` gets the correct private keys, adds them
together, groups the silent payment destinations and then generates the
taproot script pubkeys. These are then passed back to
CreateTransactionInternal, which uses these scriptPubKeys to update
vecSend before adding them to the transaction outputs.
If sending to a silent payment destination, the change type should be taproot
In order to allow SilentPaymentsSPKMs to coexist with DescriptorSPKMs,
we can no longer enforce that DescriptorSPKMs are the only kind of SPKM
expected in a descriptor wallet.
LoadScriptPubKeyMan shouldn't enforce that a ScriptPubKeyMan cannot be
active in multiple output types and internal-ness. This is instead moved
to the RPC caller where active properties can be changed during import.
Optionally allow users to create a wallet that supports silent payments.
This is signaled through an option in createwallet and a new wallet
flag.
Call IsMineSilentPayment when sending to a SP address if our wallet has
SPs enabled.

In the next commit, we create a change output using silent payments. The
presence of the change output will cause us to not fully check the
transaction and thus never create the silent payment tweaks, which is
why we check for self-transfers here. This feels a bit hacky, but works
for now.
@achow101 achow101 force-pushed the silent-payments-recv branch from e036879 to ed84b80 Compare October 3, 2023 21:06
@josibake
Copy link
Member

josibake commented Oct 4, 2023

running on signet, bitcoin-cli rescanblockchain with a silent payments wallet causes the node to crash, but bitcoin-cli rescanblockchain 63482 (chain tip - 100,000 blocks) worked. will investigate more

@w0xlt
Copy link
Contributor

w0xlt commented Nov 27, 2023

Approach ACK. Will review soon.

@DrahtBot
Copy link
Contributor

⌛ 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.

@achow101
Copy link
Member Author

achow101 commented Apr 8, 2024

Closing for now as up for grabs as I don't have time to work on this.

With having a silent payments module in libsecp, and also the plan to change this to using a descriptor, a lot of this PR is also no longer relevant.

@achow101 achow101 closed this Apr 8, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Apr 8, 2025
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.

5 participants