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

wallet: Receive silent payment transactions #28453

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 23 commits October 3, 2023 11:16
Add a method for retreiving a private key for a given scriptPubKey.
If the scriptPubKey is a taproot output, tweak the private key with the
merkle root or hash of the public key, if applicable.
Add a flag to the `CoinControl` object if silent payment destinations
are provided. Before adding the flag, call a function which checks if:

* The wallet has private keys
* The wallet is unlocked

Without both of the above being true, we cannot send to a silent payment
address.

During coin selection, if this flag is set, skip taproot inputs when
script spend data is available. This is based on the assumption that if
a user provides script spend data, they don't have access to the key
path spend. As future improvement, we could instead check to see if we
have access to the key path spend, and only exclude the output when we
don't regardless of whether or not the user provides script spend data.

Also skip UTXOs of type `WITNESS_UNKNOWN`, although it is very unlikely
our wallet would ever try to spend a witness unknown output.
`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.
@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
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

5 participants