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

Add BIP352 silentpayments module #1519

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,14 @@ option(SECP256K1_ENABLE_MODULE_RECOVERY "Enable ECDSA pubkey recovery module." O
option(SECP256K1_ENABLE_MODULE_EXTRAKEYS "Enable extrakeys module." ON)
option(SECP256K1_ENABLE_MODULE_SCHNORRSIG "Enable schnorrsig module." ON)
option(SECP256K1_ENABLE_MODULE_ELLSWIFT "Enable ElligatorSwift module." ON)
option(SECP256K1_ENABLE_MODULE_SILENTPAYMENTS "Enable Silent Payments module." OFF)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1c74941: since the next commit adds a dependency on extrakeys, and silent payments obviously depends on schnorr:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8b450bf..f036a3f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -55,10 +55,17 @@ option(SECP256K1_ENABLE_MODULE_ELLSWIFT "Enable ElligatorSwift module." ON)
 option(SECP256K1_ENABLE_MODULE_SILENTPAYMENTS "Enable Silent Payments module." OFF)
 
 # Processing must be done in a topological sorting of the dependency graph
 # (dependent module first).
 if(SECP256K1_ENABLE_MODULE_SILENTPAYMENTS)
+  if(DEFINED SECP256K1_ENABLE_MODULE_SCHNORRSIG AND NOT SECP256K1_ENABLE_MODULE_SCHNORRSIG)
+    message(FATAL_ERROR "Module dependency error: You have disabled the schnorrsig module explicitly, but it is required by the silentpayments module.")
+  endif()
+  if(DEFINED SECP256K1_ENABLE_MODULE_EXTRAKEYS AND NOT SECP256K1_ENABLE_MODULE_EXTRAKEYS)
+    message(FATAL_ERROR "Module dependency error: You have disabled the extrakeys module explicitly, but it is required by the silentpayments module.")
+  endif()
+  set(SECP256K1_ENABLE_MODULE_EXTRAKEYS ON)
   add_compile_definitions(ENABLE_MODULE_SILENTPAYMENTS=1)
 endif()
 
 if(SECP256K1_ENABLE_MODULE_ELLSWIFT)
   add_compile_definitions(ENABLE_MODULE_ELLSWIFT=1)
diff --git a/configure.ac b/configure.ac
index fe778b1..da6f707 100644
--- a/configure.ac
+++ b/configure.ac
@@ -397,10 +397,20 @@ SECP_CFLAGS="$SECP_CFLAGS $WERROR_CFLAGS"
 ###
 
 # Processing must be done in a reverse topological sorting of the dependency graph
 # (dependent module first).
 if test x"$enable_module_silentpayments" = x"yes"; then
+  if test x"$enable_module_schnorrsig" = x"no"; then
+    AC_MSG_ERROR([Module dependency error: You have disabled the schnorrsig module explicitly, but it is required by the silentpayments module.])
+  fi
+  enable_module_schnorrsig=yes
+
+  if test x"$enable_module_extrakeys" = x"no"; then
+    AC_MSG_ERROR([Module dependency error: You have disabled the extrakeys module explicitly, but it is required by the silentpayments module.])
+  fi
+  enable_module_extrakeys=yes
+
   SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_SILENTPAYMENTS=1"
 fi
 
 if test x"$enable_module_ellswift" = x"yes"; then
   SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_ELLSWIFT=1"

extrakeys already checks for schnorr, but I added it an explicit check for schnorr because otherwise the error is confusing.

Try with:

  • ./configure --enable-module-silentpayments --disable-module-extrakeys
  • cmake .. -DSECP256K1_ENABLE_MODULE_SILENTPAYMENTS=ON -DSECP256K1_ENABLE_MODULE_SCHNORRSIG=OFF
  • etc


# Processing must be done in a topological sorting of the dependency graph
# (dependent module first).
if(SECP256K1_ENABLE_MODULE_SILENTPAYMENTS)
add_compile_definitions(ENABLE_MODULE_SILENTPAYMENTS=1)
endif()

if(SECP256K1_ENABLE_MODULE_ELLSWIFT)
add_compile_definitions(ENABLE_MODULE_ELLSWIFT=1)
endif()
Expand Down Expand Up @@ -298,6 +303,7 @@ message(" ECDSA pubkey recovery ............... ${SECP256K1_ENABLE_MODULE_RECOV
message(" extrakeys ........................... ${SECP256K1_ENABLE_MODULE_EXTRAKEYS}")
message(" schnorrsig .......................... ${SECP256K1_ENABLE_MODULE_SCHNORRSIG}")
message(" ElligatorSwift ...................... ${SECP256K1_ENABLE_MODULE_ELLSWIFT}")
message(" Silent Payments ..................... ${SECP256K1_ENABLE_MODULE_SILENTPAYMENTS}")
message("Parameters:")
message(" ecmult window size .................. ${SECP256K1_ECMULT_WINDOW_SIZE}")
message(" ecmult gen table size ............... ${SECP256K1_ECMULT_GEN_KB} KiB")
Expand Down
4 changes: 4 additions & 0 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -284,3 +284,7 @@ endif
if ENABLE_MODULE_ELLSWIFT
include src/modules/ellswift/Makefile.am.include
endif

if ENABLE_MODULE_SILENTPAYMENTS
include src/modules/silentpayments/Makefile.am.include
endif
10 changes: 10 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ AC_ARG_ENABLE(module_ellswift,
AS_HELP_STRING([--enable-module-ellswift],[enable ElligatorSwift module [default=yes]]), [],
[SECP_SET_DEFAULT([enable_module_ellswift], [yes], [yes])])

AC_ARG_ENABLE(module_silentpayments,
AS_HELP_STRING([--enable-module-silentpayments],[enable Silent Payments module [default=no]]), [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1c74941: can we just break the convention and make it --enable-module-silent-payments? :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha no :)

[SECP_SET_DEFAULT([enable_module_silentpayments], [no], [yes])])

AC_ARG_ENABLE(external_default_callbacks,
AS_HELP_STRING([--enable-external-default-callbacks],[enable external default callback functions [default=no]]), [],
[SECP_SET_DEFAULT([enable_external_default_callbacks], [no], [no])])
Expand Down Expand Up @@ -394,6 +398,10 @@ SECP_CFLAGS="$SECP_CFLAGS $WERROR_CFLAGS"

# Processing must be done in a reverse topological sorting of the dependency graph
# (dependent module first).
if test x"$enable_module_silentpayments" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_SILENTPAYMENTS=1"
fi

if test x"$enable_module_ellswift" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_ELLSWIFT=1"
fi
Expand Down Expand Up @@ -450,6 +458,7 @@ AM_CONDITIONAL([ENABLE_MODULE_RECOVERY], [test x"$enable_module_recovery" = x"ye
AM_CONDITIONAL([ENABLE_MODULE_EXTRAKEYS], [test x"$enable_module_extrakeys" = x"yes"])
AM_CONDITIONAL([ENABLE_MODULE_SCHNORRSIG], [test x"$enable_module_schnorrsig" = x"yes"])
AM_CONDITIONAL([ENABLE_MODULE_ELLSWIFT], [test x"$enable_module_ellswift" = x"yes"])
AM_CONDITIONAL([ENABLE_MODULE_SILENTPAYMENTS], [test x"$enable_module_silentpayments" = x"yes"])
AM_CONDITIONAL([USE_EXTERNAL_ASM], [test x"$enable_external_asm" = x"yes"])
AM_CONDITIONAL([USE_ASM_ARM], [test x"$set_asm" = x"arm32"])
AM_CONDITIONAL([BUILD_WINDOWS], [test "$build_windows" = "yes"])
Expand All @@ -472,6 +481,7 @@ echo " module recovery = $enable_module_recovery"
echo " module extrakeys = $enable_module_extrakeys"
echo " module schnorrsig = $enable_module_schnorrsig"
echo " module ellswift = $enable_module_ellswift"
echo " module silentpayments = $enable_module_silentpayments"
echo
echo " asm = $set_asm"
echo " ecmult window size = $set_ecmult_window"
Expand Down
114 changes: 114 additions & 0 deletions include/secp256k1_silentpayments.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
#ifndef SECP256K1_SILENTPAYMENTS_H
#define SECP256K1_SILENTPAYMENTS_H

#include "secp256k1.h"
#include "secp256k1_extrakeys.h"

#ifdef __cplusplus
extern "C" {
#endif

/* This module provides an implementation for Silent Payments, as specified in
* BIP352. This particularly involves the creation of input tweak data by
* summing up private or public keys and the derivation of a shared secret using
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/private/secret key for consistency with the rest of the lib

* Elliptic Curve Diffie-Hellman. Combined are either:
* - spender's private keys and recipient's public key (a * B, sender side)
* - spender's public keys and recipient's private key (A * b, recipient side)
* With this result, the necessary key material for ultimately creating/scanning
* or spending Silent Payment outputs can be determined.
*
* Note that this module is _not_ a full implementation of BIP352, as it
* inherently doesn't deal with higher-level concepts like addresses, output
* script types or transactions. The intent is to provide a module for
* abstracting away the elliptic-curve operations required for the protocol. For
* any wallet software already using libsecp256k1, this API should provide all
* the functions needed for a Silent Payments implementation without requiring
* any further elliptic-curve operations from the wallet.
*/

/* This struct serves as an In param for passing the silent payment address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* This struct serves as an In param for passing the silent payment address
/* This struct serves as an input argument for passing the silent payment address

would be more consistent with secp256k1.h

* data. The index field is for when more than one address is being sent to in
* a transaction. Index is set based on the original ordering of the addresses
* and used to return the generated outputs matching the original ordering.
* When more than one recipient is used the recipient array will be sorted in
* place as part of generating the outputs, but the generated outputs will be
* returned in the original ordering specified by the index to ensure the
* caller is able to match up the generated outputs to the correct silent
* payment address (e.g. to be able to assign the correct amounts to the
Comment on lines +33 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* When more than one recipient is used the recipient array will be sorted in
* place as part of generating the outputs, but the generated outputs will be
* returned in the original ordering specified by the index to ensure the
* caller is able to match up the generated outputs to the correct silent
* payment address (e.g. to be able to assign the correct amounts to the
* When more than one recipient is used, the recipient array will be sorted in
* place as part of generating the outputs, but the generated outputs will be
* returned in the original ordering specified by the index to ensure the
* caller is able to match up the generated outputs to the correct silent
* payment address (e.g., to be able to assign the correct amounts to the

* correct generated outputs in the final transaction).
*/
typedef struct {
secp256k1_pubkey scan_pubkey;
secp256k1_pubkey spend_pubkey;
size_t index;
} secp256k1_silentpayments_recipient;

/** Create Silent Payment outputs for recipient(s).
*
* Given a list of n private keys a_1...a_n (one for each silent payment
* eligible input to spend), a serialized outpoint, and a list of recipients,
* create the taproot outputs.
*
* `outpoint_smallest36` refers to the smallest outpoint lexicographically
* from the transaction inputs (both silent payments eligible and non-eligible
* inputs). This value MUST be the smallest outpoint out of all of the
* transaction inputs, otherwise the recipient will be unable to find the
* payment.
*
* If necessary, the private keys are negated to enforce the right y-parity.
* For that reason, the private keys have to be passed in via two different
* parameter pairs, depending on whether the seckeys correspond to x-only
* outputs or not.
*
* Returns: 1 if creation of outputs was successful. 0 if an error occured.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: typo nit: s/occured/occurred here and in few more places in the file

* Args: ctx: pointer to a context object
* Out: generated_outputs: pointer to an array of pointers to xonly pubkeys,
* one per recipient.
* The order of outputs here matches the original
* ordering of the recipients array.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: the words "matches the original ordering" had me thoroughly confused, because I thought it referred to the array order. Try instead:

The outputs here are sorted by the index value provided in recipients. 

* In: recipients: pointer to an array of pointers to silent payment
* recipients, where each recipient is a scan public
* key, a spend public key, and an index indicating
* its position in the original ordering. The
* recipient array will be sorted in place, but
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: grouped by scan and spend key,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an implementation detail the caller does not need to be aware of, since we handle the grouping internally. From a callers perspective, all they need to do is pass an array of recipients, in any order.

* generated outputs are saved in the
* `generated_outputs` array to match the ordering
* from the index field. This ensures the caller is
* able to match the generated outputs to the
* correct silent payment addresses. The same
* recipient can be passed multiple times to create
* multiple outputs for the same recipient.
* n_recipients: the number of recipients. This is equal to the
* total number of outputs to be generated as each
* recipient may passed multiple times to generate
* multiple outputs for the same recipient
* outpoint_smallest36: serialized smallest outpoint (lexicographically)
* from the transaction inputs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: the 36 suffix is weird. outpoint36_smallest would be slightly more clear.

Suggest adding to the doc (instead): (36 bytes)

Given how many times this has gone wrong, the following warning seems justified:

Lexicographical sorting applies to the concatenated outpoint hash and
index (vout). The latter is little-endian encoded, so must not be
sorted in its integer representation.

The test vector in this PR doesn't prevent this, because sorting is left to the implementer.

Test vectors in the BIP don't necessary prevent this either, because implementers will assume it's all covered in libsecp.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a separate discussion on the Musig2 PR that I need to follow up on regarding this, where there might be a more clever way to enforce the array length without these suffix hints. For now, I think I will remove the 36 and add it to the docs, as you suggest.

Agree that we can't enforce they are passing the correct outpoint out of a list of outpoints here, but I do think we can make it clear in the header file that this is not handled by libsecp and implementations MUST ensure they are following the test vectors from the BIP. I'll add something as such to the header documentation.

* taproot_seckeys: pointer to an array of pointers to 32-byte
* private keys of taproot inputs (can be NULL if no
* private keys of taproot inputs are used)
* n_taproot_seckeys: the number of sender's taproot input private keys
* plain_seckeys: pointer to an array of pointers to 32-byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: why can't this be secp256k1_keypair as well? If not documented in the API, at least a clarifying code comment would be useful.

The PR description implies the logic is the other way around:

taproot_seckeys are passed as keypairs to avoid the function needing to compute the public key to determine parity.

So you want to save the caller from having to use secp256k1_keypair? But only for legacy inputs, so that doesn't seem that useful towards the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this is outdated documentation. The function does expect taproot_seckeys to be secp256k1_keypairs.

* private keys of non-taproot inputs (can be NULL
* if no private keys of non-taproot inputs are
* used)
* n_plain_seckeys: the number of sender's non-taproot input private
* keys
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_sender_create_outputs(
const secp256k1_context *ctx,
secp256k1_xonly_pubkey **generated_outputs,
const secp256k1_silentpayments_recipient **recipients,
Copy link

@jlest01 jlest01 Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason not to make the outer pointer constant since it is not modified in this function?

Suggested change
const secp256k1_silentpayments_recipient **recipients,
const secp256k1_silentpayments_recipient * const *recipients,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recipients array is passed to secp256k1_hsort, where the array is sorted in place.

size_t n_recipients,
const unsigned char *outpoint_smallest36,
const secp256k1_keypair * const *taproot_seckeys,
size_t n_taproot_seckeys,
const unsigned char * const *plain_seckeys,
size_t n_plain_seckeys
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(5);

#ifdef __cplusplus
}
#endif

#endif /* SECP256K1_SILENTPAYMENTS_H */
2 changes: 2 additions & 0 deletions src/modules/silentpayments/Makefile.am.include
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
include_HEADERS += include/secp256k1_silentpayments.h
noinst_HEADERS += src/modules/silentpayments/main_impl.h
Loading