Silent Payments: sending

josibake commented at 4:37 pm on August 2, 2023: member

This PR is part of integrating silent payments into Bitcoin Core. Status and tracking for the project is managed in #28536

This PR depends on #28122 and is marked as a draft until it is merged. If interested in those commits, please review on #28122

Pre-work / Refactors

The first three commits are pre-work / refactors needed for this PR. They are broken out into the following PRS:

Sending

Silent Payments logic

The main focus of this PR is:

Applying the Taptweak to a taproot internal private key (this is a copy-paste of the code for applying the taptweak in the signing process)
Getting a private key from a given scriptPubKey
Creating silent payment outputs
Applying the created scriptPubKeys back to the vector of CRecipients

The functions are then used together to create silent payment outputs during CreateTransactionInternal.

Final steps

The last commits ensure that:

Coin selection is silent payments aware and knows to exclude taproot script path spends and inputs with unknown witness when funding a transaction which pays to a silent payment address
The change output type is correctly chosen when paying to a silent payment address
Functional tests

DrahtBot commented at 4:37 pm on August 2, 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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30562 (PayToAnchor(P2A) followups by instagibbs)
#30352 (policy: Add PayToAnchor(P2A), OP_1 <0x4e73> as a standard output script for spending by instagibbs)
#30093 (optimization: reserve memory allocation for transaction inputs/outputs by paplorinc)
#29432 (Stratum v2 Template Provider (take 3) by Sjors)
#29295 (CKey: add Serialize and Unserialize by Sjors)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#28241 (Silent payment index (for light wallets and consistency check) by Sjors)
#28122 (Silent Payments: Implement BIP352 by josibake)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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 CI failed on Aug 2, 2023

josibake renamed this:
~~Silent Payments: implement sending~~
Silent Payments: sending
on Aug 3, 2023

josibake force-pushed on Aug 3, 2023

DrahtBot removed the label CI failed on Aug 3, 2023

josibake commented at 9:50 am on August 3, 2023: member

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!

updated! I added the summary in #27827 and added links back to the parent PR in each of the child PRs.

in src/wallet/spend.cpp:942 in 181e329a1d outdated

899+    assert(tx_outpoints.size() > 0);
900+    CKey scalar_ecdh_input = PrepareScalarECDHInput(input_private_keys, tx_outpoints);
901+    std::vector<SilentPaymentRecipient> silent_payment_recipients = GroupSilentPaymentAddresses(silent_payment_destinations);
902+    std::vector<CRecipient> outputs;
903+    for (const auto& recipient : silent_payment_recipients) {
904+        Sender sender{scalar_ecdh_input, recipient};

josibake commented at 10:29 am on August 8, 2023:

This does not need to be a struct, it can just be a function.

in test/functional/wallet_silentpayments_sending.py:91 in e6f7458324 outdated

86+            address=SILENT_PAYMENT_ADDRESS,
87+            amount=21,
88+        )
89+        assert txid
90+
91+    def test_deterministic_send(self):

BrandonOdiwuor commented at 5:32 pm on August 9, 2023:

Incorporating logs within the test is crucial for offering transparent insight into the test’s progression, simplifying the identification of problems, and enhancing comprehension of the test’s overall behavior.

in test/functional/wallet_silentpayments_sending.py:127 in e6f7458324 outdated

122+                assert output["scriptPubKey"]["address"] == "bcrt1p5num3dvry0ffusg3s7v2j5fy025p95jtn6js65jlruwc69r9je2s6qfsj7"
123+                break
124+        else:
125+            assert False
126+
127+    def test_address_reuse(self):

BrandonOdiwuor commented at 5:34 pm on August 9, 2023:

Also consider adding logs to this test as the ones above

josibake force-pushed on Aug 30, 2023

josibake force-pushed on Aug 31, 2023

josibake force-pushed on Sep 8, 2023

josibake force-pushed on Sep 11, 2023

DrahtBot added the label CI failed on Sep 11, 2023

josibake force-pushed on Sep 11, 2023

josibake force-pushed on Sep 12, 2023

josibake force-pushed on Sep 14, 2023

DrahtBot removed the label CI failed on Sep 14, 2023

DrahtBot added the label Needs rebase on Sep 19, 2023

josibake force-pushed on Sep 21, 2023

DrahtBot added the label CI failed on Sep 21, 2023

DrahtBot removed the label Needs rebase on Sep 21, 2023

DrahtBot removed the label CI failed on Sep 21, 2023

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

Note: send does not work, but sendall, sendtoaddress does

josibake force-pushed on Oct 2, 2023

DrahtBot added the label CI failed on Oct 2, 2023

josibake force-pushed on Oct 3, 2023

DrahtBot removed the label CI failed on Oct 4, 2023

DrahtBot added the label Needs rebase on Oct 16, 2023

josibake force-pushed on Jan 15, 2024

DrahtBot removed the label Needs rebase on Jan 15, 2024

DrahtBot commented at 9:36 pm on January 17, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20505330863

DrahtBot added the label CI failed on Jan 17, 2024

josibake force-pushed on Jan 19, 2024

josibake commented at 2:30 pm on January 19, 2024: member

Rebased https://github.com/bitcoin/bitcoin/commit/3f4b0f77c7f2ff52b9c84bf1c5a70281a066cfe2 -> https://github.com/bitcoin/bitcoin/commit/3b0dd79cacaf4e97489746ace5c1c8d6fe8db38d (implement-bip352-sending-01 -> implement-bip352-01-rebase, compare)

Fixed silent merge conflict with CKey now returning std::byte*

DrahtBot removed the label CI failed on Jan 19, 2024

willcl-ark added the label Wallet on Jan 24, 2024

willcl-ark added the label Privacy on Jan 24, 2024

DrahtBot added the label Needs rebase on Jan 26, 2024

josibake force-pushed on Jan 26, 2024

DrahtBot removed the label Needs rebase on Jan 26, 2024

DrahtBot added the label CI failed on Feb 2, 2024

DrahtBot removed the label CI failed on Feb 7, 2024

DrahtBot added the label Needs rebase on Feb 20, 2024

josibake force-pushed on Apr 22, 2024

DrahtBot removed the label Needs rebase on Apr 22, 2024

DrahtBot added the label CI failed on Apr 22, 2024

DrahtBot commented at 9:52 pm on April 22, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24114299414

josibake force-pushed on Apr 27, 2024

josibake force-pushed on May 5, 2024

DrahtBot removed the label CI failed on May 5, 2024

DrahtBot added the label Needs rebase on May 20, 2024

achow101 referenced this in commit f0745d028e on Jun 27, 2024

Squashed 'src/secp256k1/' changes from 4af241b320..00b0cb19a9

00b0cb19a9 docs: update README
54b8bc8ec6 ci: enable silentpayments module
96bd71fb8a tests: add BIP-352 test vectors
c30bc013fe silentpayments: add benchmark for `scan_outputs`
91b1b3365b silentpayments: add examples/silentpayments.c
b4475ea80c silentpayments: receiving
23c7aead63 silentpayments: recipient label support
79562d0cd1 silentpayments: sending
35f91359b8 build: add skeleton for new silentpayments (BIP352) module
0055b86780 Merge bitcoin-core/secp256k1#1551: Add ellswift usage example
ea2d5f0f17 Merge bitcoin-core/secp256k1#1563: doc: Add convention for defaults
ca06e58b2c Merge bitcoin-core/secp256k1#1564: build, ci: Adjust the default size of the precomputed table for signing
e2af491263 ci: Switch to the new default value of the precomputed table for signing
d94a9273f8 build: Adjust the default size of the precomputed table for signing
fcc5d7381b Merge bitcoin-core/secp256k1#1565: cmake: Bump CMake minimum required version up to 3.16
9420eece24 cmake: Bump CMake minimum required version up to 3.16
16685649d2 doc: Add convention for defaults
a5269373fa Merge bitcoin-core/secp256k1#1555: Fixed O3 replacement
b8fe33332b cmake: Fixed O3 replacement
31f84595c4 Add ellswift usage example
fe4fbaa7f3 examples: fix case typos in secret clearing paragraphs (s/, Or/, or/)

git-subtree-dir: src/secp256k1
git-subtree-split: 00b0cb19a97718dfaab70aa7505ff157f22a31bd

5a0b27cf3c

Merge commit '5a0b27cf3c122ef9b9caea5727beaea2a9172442' into refresh-secp256k1 6ddbfefcee

josibake force-pushed on Jul 15, 2024

DrahtBot removed the label Needs rebase on Jul 15, 2024

refactor: replace early returns with GetPubKey()

Instead of getting the public key from the keypair object, get it
directly using `CKey::GetPubKey()`.

This commit is a staging refactor to simplify the diff for moving the
merkle tweaking logic out of this function in a later commit, but also
makes this code simpler and more concise.

1cde57ffdc

tests: add key tweak smoke test

Sanity check that using CKey/CPubKey directly vs using secp256k1_keypair objects
returns the same results for BIP341 key tweaking.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

b4afd4b2cf

crypto: add KeyPair wrapper class

Add a `KeyPair` class which wraps the `secp256k1_keypair`. This keeps
the secret data in secure memory and enables passing the
`KeyPair` object directly to libsecp256k1 functions expecting a
`secp256k1_keypair`.

Motivation: when passing `CKeys` for taproot outputs to libsecp256k1 functions,
the first step is to create a `secp256k1_keypair` data type and use that
instead. This is so the libsecp256k1 function can determine if the key
needs to be negated, e.g., when signing.

This is a bit clunky in that it creates an extra step when using a `CKey`
for a taproot output and also involves copying the secret data into a
temporary object, which the caller must then take care to cleanse. In
addition, the logic for applying the merkle_root tweak currently
only exists in the `SignSchnorr` function.

In a later commit, we will add the merkle_root tweaking logic to this
function, which will make the merkle_root logic reusable outside of
signing by using the `KeyPair` class directly.

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>

5b7a788173

refactor: use KeyPair in SignSchnorr

Use `KeyPair` instead of creating a `secp256k1_keypair` object. The
main change here is creating a `KeyPair` instead of a
`secp256k1_keypair` and then passing it to the libsec256k1 functions
using `reinterpret_cast<secp256k1_keypair*>(keypair)`.

The variable name `keypair` is used for the reinterpret_cast to simplify the
diff in a later commit when all of the logic in SignSchnorr is moved into the
KeyPair class.

Note: we no longer need to call memory_cleanse since `KeyPair` is now
using a secure allocator (same as CKey). See src/support/allocator/secure.h

c8608ea2a1

refactor: move SignSchnorr logic to KeyPair

Move `SignSchnorr` to `KeyPair`. This makes `CKey::SignSchnorr` now
compute a `KeyPair` object and then call `KeyPair::SignSchorr`. The
signing logic is move-only with the exception of changing
`keypair.data()` to `my_keypair->data()`, since we now have access to
the private member `m_keypair`.

26c88477ae

tests: add tests for KeyPair

Reuse existing BIP340 tests, as there should be
no behavior change between the two

7bf40fd223

conf: add ECDH,SILENTPAYMENTS secp256k1 modules 1a598f786f

Add "sp" HRP 112c1246a8

Add V0SilentPaymentDestination address type 4590441bc7

common: add bip352.{h,cpp} secp256k1 module

Wrap the silentpayments module from libsecp256k1. This is placed in
common as it is intended to be used by:

  * RPCs: for parsing addresses
  * Wallet: for sending, receiving, spending silent payment outputs
  * Node: for creating silent payment indexes for light clients

38067a6ef0

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.

d48013ba9d

tests: 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

9836a1c12c

wallet: get serialized size for `V0SilentPayments`

BIP352 v0 specifies that a silent payment output is a taproot output.
Taproot scriptPubKeys are a fixed size, so when calculating the
serialized size for a CRecipient with a V0SilentPayments destination,
use WitnessV1Taproot for the serialized txout size.

f1086b07e5

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.

c9de1e6957

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.

206e0c26c3

wallet: add `IsInputForSharedSecretDerivation` function 89a981a7a2

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.

916db91368

wallet: update TransactionChangeType

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

e41c64cca7

wallet: enable sending to silent payment address edf5a3501d

tests: add sending functional tests 6924c45134

josibake force-pushed on Jul 23, 2024

DrahtBot added the label CI failed on Jul 23, 2024

DrahtBot commented at 10:45 am on July 23, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27794809832

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label Needs rebase on Aug 2, 2024

DrahtBot commented at 5:59 pm on August 2, 2024: contributor

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

ryanofsky referenced this in commit b38fb19b7e on Aug 7, 2024

DrahtBot commented at 0:11 am on October 30, 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.

Silent Payments: sending #28201

Pre-work / Refactors

Sending

Silent Payments logic

Final steps

Code Coverage

Reviews

Conflicts