Silent Payments: Implement BIP352 #28122

josibake · 2023-07-21T18:36:37Z

This PR is part of integrating silent payments into Bitcoin Core. The project is tracked in #28536

Pre-work / Refactors

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

BIP352

This PR focuses strictly on the BIP logic and attempts to separate it from the wallet and transaction implementation details. This is accomplished by working directly with public and private keys, instead of needing a wallet backend and transactions for testing. Labels for the receiver are optional and thus deferred for a later PR.

Test vectors from the BIP are included as unit tests.

Before reviewing, it is strongly recommended you read bitcoin/bips#1458 and take a look at the reference python implementation on the BIP.

DrahtBot · 2023-07-21T18:36:41Z

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:

#30048 (crypto: add NUMS_H const by josibake)
#30047 (refactor: Model the bech32 charlimit as an Enum by josibake)
#27260 (Enhanced error messages for invalid network prefix during address parsing. by russeree)

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.

src/kernel/chainparams.cpp

src/wallet/silentpayments.h

src/key.cpp

aureleoules

Left some comments. I will review further later.

src/pubkey.h

src/wallet/silentpayments.cpp

josibake · 2023-08-01T15:48:43Z

Needs rebase, if still relevant

thanks, fixed!

src/kernel/chainparams.cpp

src/bech32.cpp

src/key_io.cpp

src/key.cpp

src/key_io.cpp

src/wallet/silentpayments.cpp

DrahtBot · 2024-04-22T16:14:39Z

🚧 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/24112092548}

Sjors · 2024-04-26T10:34:35Z

src/primitives/transaction.h

+        DataStream ser_a, ser_b;
+        ser_a << a;
+        ser_b << b;
+        return Span{ser_a} < Span{ser_b};


7a27533: if you're going to change this here instead of adding a custom sort for BIP352, then you should add a note that it's critical for BIP352.

However I'm not sure if we should sort this way by default. If I wanted to list transaction outputs, I'd want to sorted by their integer output position.

That said, I don't know if we currently have any RPC where this sort order is exposed.

From what I could tell, we don’t expose this anywhere. I’m in favor of making this the default since we only ever really talk about outpoints in terms of their serialised encoding. Gonna give it some more thought, but I’m considering opening this commit as its own PR. It seems preferable to me to have one way of sorting outpoints vs creating a special case for silent payments, especially when there doesn’t seem to be any other “sort outpoints” use case (e.g. none of the tests failed when I made this change).

But still need to verify re: displayed in RPCs. At that point tho, we could just sort by vout and ignore the txid in the sort

Separate PR sounds good to get more eyes on it. Meanwhile this is fine a temporary fix for this PR.

opened in #30046

Closed #30046 😄

Sjors · 2024-05-02T08:35:35Z

@josibake can you add a link to bitcoin-core/secp256k1#1519 in the PR description? It currently takes a few "load more" clicks and searching to see what this PR depends on.

92f592023f ci: enable silentpayments module 8ddc4574c9 tests: add BIP-352 test vectors 8315abd830 silentpayments: add benchmark for `scan_outputs` f3a9516ec8 silentpayments: add examples/silentpayments.c 7e11e7613b silentpayments: add recipient light client support 3321771b0e silentpayments: add recipient scanning routine 766567f099 silentpayments: add opaque data type `public_data` 8d0bb06ce7 silentpayments: add recipient label support 9c9bd057bc silentpayments: add sender routine 036e688fd0 silentpayments: implement output pubkey creation 1ffee123d6 silentpayments: implement shared secret creation 7a5683260c silentpayments: add sortable recipient struct a8d6f4b8e1 doc: add module description for silentpayments 1121a4d376 build: add skeleton for new silentpayments (BIP352) module 7d2591ce12 Add secp256k1_pubkey_sort da515074e3 Merge bitcoin-core/secp256k1#1058: Signed-digit multi-comb ecmult_gen algorithm 4c341f89ab Add changelog entry for SDMC a043940253 Permit COMB_BITS < 256 for exhaustive tests 39b2f2a321 Add test case for ecmult_gen recoded = {-1,0,1} 644e86de9a Reintroduce projective blinding 07810d9abb Reduce side channels from single-bit reads a0d32b597d Optimization: use Nx32 representation for recoded bits e03dcc44b5 Make secp256k1_scalar_get_bits support 32-bit reads 5005abee60 Rename scalar_get_bits -> scalar_get_bits_limb32; return uint32_t 6247f485b6 Optimization: avoid unnecessary doublings in precomputation 15d0cca2a6 Optimization: first table lookup needs no point addition 7a33db35cd Optimization: move (2^COMB_BITS-1)/2 term into ctx->scalar_offset ed2a056f3d Provide 3 configurations accessible through ./configure 5f7be9f6a5 Always generate tables for current (blocks,teeth) config fde1dfcd8d Signed-digit multi-comb ecmult_gen algorithm 486518b350 Make exhaustive tests's scalar_inverse(&x,&x) work ab45c3e089 Initial gej blinding -> final ge blinding aa00a6b892 Introduce CEIL_DIV macro and use it REVERT: 0270b14309 labels: actually set the label REVERT: 3d08027789 ci: enable silentpayments module REVERT: 85946762a5 tests: add BIP-352 test vectors REVERT: bf349c2a08 silentpayments: add examples/silentpayments.c REVERT: 9a7106e19c silentpayments: add recipient light client support REVERT: f113564298 silentpayments: add recipient scanning routine REVERT: 4fb8716f4f silentpayments: add opaque data type `public_data` REVERT: 987d829e8f silentpayments: add recipient label support REVERT: 14ca754578 silentpayments: add sender routine REVERT: 9b965927da silentpayments: implement output pubkey creation REVERT: a0fcc2c780 silentpayments: implement shared secret creation REVERT: 13f203dacd silentpayments: add sortable recipient struct REVERT: a9326bdd7a doc: add module description for silentpayments REVERT: 15d3e71cc1 build: add skeleton for new silentpayments (BIP352) module REVERT: cc7d18a8a8 extrakeys: add secp256k1_pubkey_sort git-subtree-dir: src/secp256k1 git-subtree-split: 92f592023f3f4d6a66724772349fbdc4967ab50f

…secp256k1

Bech32(m) was defined with a 90 character limit so that certain guarantees for error detection could be made for segwit addresses. However, there is nothing about the encoding scheme itself that requires a limit and in practice bech32(m) has been used without the 90 char limit (e.g. lightning invoices). Further, increasing the character limit doesn't do away with error detection, it simply lessons the guarantees. Model charlimit as an Enum, so that if a different address scheme is using bech32(m), the character limit for that address scheme can be used, rather than always using the 90 charlimit defined for segwit addresses.

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

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.

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

josibake mentioned this pull request Jul 21, 2023

Silent Payments: send and receive #27827

Closed

3 tasks

josibake force-pushed the implement-bip352 branch from db0ab56 to be049d2 Compare July 21, 2023 18:54

DrahtBot added the CI failed label Jul 21, 2023

josibake force-pushed the implement-bip352 branch from be049d2 to 13529c9 Compare July 21, 2023 19:23

This was referenced Jul 22, 2023

build: pass sanitize flags to instrument libsecp256k1 code #27991

Closed

wallet: add seeds argument to importdescriptors #27351

Closed

theStack reviewed Jul 22, 2023

View reviewed changes

src/kernel/chainparams.cpp Show resolved Hide resolved

josibake force-pushed the implement-bip352 branch from 13529c9 to 1d75df2 Compare July 23, 2023 12:18

jonatack reviewed Jul 23, 2023

View reviewed changes

src/wallet/silentpayments.h Outdated Show resolved Hide resolved

josibake force-pushed the implement-bip352 branch from 1d75df2 to dc18efa Compare July 26, 2023 09:10

DrahtBot removed the CI failed label Jul 26, 2023

rajarshimaitra mentioned this pull request Jul 26, 2023

Silent Payments: Implement BIP352 #28122 Bitshala/BitcoinCore-PR-Review-Club#39

Closed

achow101 reviewed Jul 26, 2023

View reviewed changes

src/key.cpp Outdated Show resolved Hide resolved

src/key.cpp Outdated Show resolved Hide resolved

aureleoules reviewed Jul 28, 2023

View reviewed changes

DrahtBot added the CI failed label Jul 29, 2023

josibake force-pushed the implement-bip352 branch from dc18efa to fa00edf Compare August 1, 2023 15:48

DrahtBot removed the CI failed label Aug 1, 2023

josibake force-pushed the implement-bip352 branch from fa00edf to 8ee791e Compare August 2, 2023 12:42

This was referenced Aug 2, 2023

Silent Payments: sending #28201

Draft

Silent Payments: receiving #28202

Closed

murchandamus reviewed Aug 2, 2023

View reviewed changes

src/kernel/chainparams.cpp Show resolved Hide resolved

josibake commented Aug 3, 2023

View reviewed changes

src/bech32.cpp Outdated Show resolved Hide resolved

BrandonOdiwuor reviewed Aug 4, 2023

View reviewed changes

src/key_io.cpp Outdated Show resolved Hide resolved