wallet: Receive silent payment transactions

achow101 commented at 8:39 pm on September 11, 2023: member

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 commented at 8:39 pm on September 11, 2023: contributor

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.

DrahtBot added the label Wallet on Sep 11, 2023

DrahtBot added the label CI failed on Sep 11, 2023

achow101 force-pushed on Sep 12, 2023

luke-jr commented at 4:08 pm on September 13, 2023: member

Approach NACK.

It should be possible to use silent payments without a new/different wallet.
getnewaddress should never return the same thing twice (absent a new option explicitly requesting that behaviour).

achow101 commented at 4:51 pm on September 13, 2023: member

It should be possible to use silent payments without a new/different wallet.

Upgrading will be possible to implement after #26728. Note that it is currently difficult to upgrade a descriptor wallet created before Taproot’s activation to be able to give out Taproot addresses. #25907 (which depends on #26728) resolves that issue, and I would prefer to have upgrade paths for both new descriptor types and for silent payments that use approximately the same code paths rather than a bespoke solution.

getnewaddress should never return the same thing twice

It’s not clear to me what the interface for this should look like, other than just a completely new RPC? I’m working on implementing the labels part of silent payments so it would not always be the same address, just optionally if the user has chose to apply a label

(absent a new option explicitly requesting that behaviour).

Since the user must set the address type to silent-payment, there is essentially a new option that explicitly requests this behavior. Perhaps that should just be documented?

DrahtBot added the label Needs rebase on Sep 13, 2023

achow101 force-pushed on Sep 13, 2023

DrahtBot removed the label Needs rebase on Sep 13, 2023

josibake commented at 2:40 pm on September 14, 2023: member

It’s not clear to me what the interface for this should look like, other than just a completely new RPC? I’m working on implementing the labels part of silent payments so it would not always be the same address, just optionally if the user has chose to apply a label

In an older draft, we did implement a new RPC getsilentpaymentaddress, which could also take a label argument. Testers and reviewers seemed to like that approach better. Personally, I prefer using getnewaddress and I think passing the output type of “silent-payment” makes it clear enough that this is a static payment code. The reason I like this approach better is it avoids introducing a new RPC that will only be used by a handful of users. Seems annoying to have an RPC that just doesn’t work 90% of the time. But I’d be happy with either approach.

achow101 force-pushed on Sep 15, 2023

DrahtBot added the label Needs rebase on Sep 19, 2023

josibake commented at 4:32 pm on September 26, 2023: member

~~Note: send does not work, but sendall, sendtoaddress do work, working on a fix in #28201~~ Fixed by https://github.com/bitcoin/bitcoin/pull/28560

achow101 force-pushed on Sep 27, 2023

DrahtBot removed the label Needs rebase on Sep 27, 2023

achow101 commented at 3:14 pm on September 28, 2023: member

Discussion at CoreDev concluded that using a descriptor for receiving is probably still useful. Will rework this to do that later (after 26.0 release probably).

DrahtBot added the label Needs rebase on Oct 2, 2023

DrahtBot commented at 9:51 pm on October 2, 2023: contributor

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

conf: add EDCH module 65621e11be

crypto: update CKey, CPubkey for silent payments

* Add methods tweaking a CKey by adding a scalar value or by multiplying
* Add a method for adding two CPubKeys
* Add a method for doing ECDH with a CPubKey and scalar
* Add a method for converting a BIP340 pubkey to a compressed pubkey
* Add BIP341 NUMS point H

These are the CKey,CPubkey primitives we need to implement
the silent payments protocol.

c29883390e

Increase the Bech32m limit if decoding SP address

Bech32m imposes a 90 character limit when decoding strings. Since a
silent payment address is the concatenation of two public keys, raise
this limit to 1024 when decoding a silent payment address.

The new limit is much higher than needed to account for forward
compatibility: new silent payment versions may add new data to the data
part of the address.

ad4b7b182f

Add "sp" HRP 3c7a5b7069

Add V0SilentPaymentDestination address type c1a63e807f

Implement BIP352: Silent Payments

Implement sending and receiving, per BIP352. This is done without
requiring a full wallet, in order to simplify unit testing and to create
a more clear boundary as to what pertains to the BIP and what is left to
the wallet to decided as an implementation.

To correctly hash the public keys, per the BIP, update the HashWriter.write
method to accept a Span<unsigned char>. This allows us to easily pass a
public key in for hashing, where only the data from the public key is hashed.

699577c911

Add BIP352 test vectors as unit tests

Use the test vectors to test sending and receiving. A few cases are not
covered here, namely anything that requires testing specific to the
wallet. For example:

* Taproot script path spending is not tested, as that is better tested in
  a wallets coin selection / signing logic
* Re-computing outputs during RBF is not tested, as that is better
  tested in a wallets RBF logic

The unit tests are written in such a way that adding new test cases is
as easy as updating the JSON file.

64bcc23a11

wallet: disable sending to silent payment address

Have `IsValidDestination` return false for silent payment destinations
and set an error string when decoding a silent payment address.

This prevents anyone from sending to a silent payment address before
sending is implemented in the wallet, but also allows the functions to
be used in the unit testing famework.

30bbeb3804

wallet: Introduce and use PreselectedInput class in CCoinControl

Instead of having different maps for selected inputs, external inputs,
and input weight in CCoinControl, have a class PreselectedInput which
tracks stores that information for each input.

573af924dd

coincontrol: Replace HasInputWeight with returning optional from Get 4c627eb060

wallet: Replace SelectExternal with SetTxOut

Instead of having a separate CCoinControl::SelectExternal function, we
can use the normal CCoinControl::Select function and explicitly use
PreselectedInput::SetTxOut in the caller. The semantics of what an
external input is remains.

d94bb08c7c

wallet: Set preset input sequence through coin control ea7624ae83

wallet: Explicitly preserve transaction locktime in CreateTransaction

We provide the preset nLockTime to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.

19e61030a6

wallet: Explicitly preserve transaction version in CreateTransaction

We provide the preset nVersion to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.

a2a11be1b9

wallet: Explicitly preserve scriptSig and scriptWitness in CreateTransaction

When creating a transaction with preset inputs, also preserve the
scriptSig and scriptWitness for those preset inputs if they are provided
(e.g. in fundrawtransaction).

4fa59bced7

wallet: Preserve preset input order

fundrawtransaction expects preset inputs to be in the order given in the
raw transaction so the order needs to be provided to CreateTransaction
in order to set the order correctly.

2e88b0a66a

wallet: use optional for change position as an optional in CreateTransaction

Instead of making -1 a magic number meaning no change or random change
position, use an optional to have that meaning.

9a880ac600

wallet: return CreatedTransactionResult from FundTransaction

Instead of using the output parameters, return CreatedTransactionResult
from FundTransaction in the same way that CreateTransaction does.
Additionally, instead of modifying the original CMutableTransaction, the
result from CreateTransactionInternal is used.

d3071b7ebd

wallet: Add function for parsing SFFO options

Different RPCs pass options for SFFO in two different ways:

1. the string addresses from which to subtract
2. The integer index (vout) of the address from which to subtract

Add a function which always returns a set of integers. This will
be used when creating CRecipient objects in a later commit.

174ed79ae6

wallet: Refactor parseRecipients

Refactor to use the set<int> format for SFFO.
Have the function return a vector of CRecipients, instead of using
an out-param.

fc47aa22b1

wallet,rpc: Add ParseOutputs function

Move the options parsing logic for SFFO out of FundTransaction into this
function.

Copy the options parsing logic from AddOutputs (called in
ConstructTransaction) into this function.

This sets us up to use ParseRecipients in the next commit and remove
SFFO logic from FundTransaction.

12160d9437

wallet, refactor: FundTransaction

Instead of parsing tx.vout, make `FundTransaction` take a `CRecipient`
vector directly. This allows us to remove SFFO logic from the wrapper
RPC `FundTransaction` since the `CRecipient` objects have already been
created with the correct SFFO values.

Copy the data field check from `AddOutputs` (used in `ConstructTransaction`)
over to `ParseRecipients`. This lets us use `ParseOutputs` anytime we
are parsing user provided outputs before passing them to
`FundTransaction`.

This sets us up in a future PR to be able to use these RPCs with BIP352
static payment codes.

b2efb358b5

refactor, move-only: move GetSerializeSize call

Move the `GetSerializeSizeForDestination` call up to the first time
loop over vecSend.

b1d383b5c3

crypto: add method for applying the taptweak

The wallet returns an untweaked internal key for taproot outputs. If the
output commits to a tree of scripts, this key needs to be tweaked with
the merkle root. Even if the output does not commit to a tree of
scripts, BIP341/342 recommend commiting to a hash of the public key.

Depending how the taproot output was created, we need to apply the merkle
root tweak or hash of the public key tweak before doing ECDH

e0fd5ba2eb

wallet: add method for retreiving a private key

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.

7b641a1d2d

wallet: make coin selection silent payment aware

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.

36f1c1c9e4

wallet: add `IsInputForSharedSecretDerivation` function 3eb0e981dc

wallet: add `CreateSilentPaymentOutputs` function

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

af268c2d21

wallet: update TransactionChangeType

If sending to a silent payment destination, the change type should be taproot

3e8825342b

wallet: enable sending to silent payment address 3fd432dfec

tests: add sending functional tests 519e5b169f

wallet: Allow other SPKM types when descriptors

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.

68451244b6

interfaces: Provide the coins spent by a tx in TransactionAddedToMempool f1d0fc3c4c

interfaces: Also retrieve undo data in findBlock 4193f6eaca

wallet: Remove restriction on one spkm being active in multiple slots

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.

b96d5ca0fd

wallet: create new type OutputType::SILENT_PAYMENT 9170a20f99

walletdb: storage of silent payments data 34fa51a477

wallet: Add SilentPaymentsSPKM c9fcea559e

wallet: Load SilentPaymentsSPKM from database records 467392279a

Implement IsMineSilentPayments 006d832c3a

wallet: Add wallet flag for silent payments and setup when set

Optionally allow users to create a wallet that supports silent payments.
This is signaled through an option in createwallet and a new wallet
flag.

a8aabd0c9f

wallet: Check IsMineSilentPayments using spent coins provided by mempool 46dda94167

wallet: Gather coins spent by a tx in a block for silent payment 8e5d1c2faa

wallet: Silent payments cannot use fast rescan with filters c83a70a972

wallet: check for self-transfer when sending

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.

244cb7f6be

wallet: Send to a silent payments change address c81a0c4323

tests: add receiving functional test ed84b80c9a

achow101 force-pushed on Oct 3, 2023

josibake commented at 4:43 pm on October 4, 2023: member

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 commented at 4:22 am on November 27, 2023: contributor

Approach ACK. Will review soon.

DrahtBot commented at 1:20 am on February 25, 2024: 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 commented at 8:37 am on April 8, 2024: member

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 closed this on Apr 8, 2024

wallet: Receive silent payment transactions #28453

Code Coverage

Reviews

Conflicts