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.

Sign up for GitHub

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

Jump to bottom

bitcoin: support sending to silent payment addresses #1220

Merged

benma merged 2 commits into BitBoxSwiss:master from benma:sp

Sep 18, 2024

Collaborator

benma commented May 20, 2024

No description provided.

Collaborator Author

benma commented May 20, 2024

This currently adds 9228 bytes to the binary, luckily less than this PR saves 😄 #1215

benma force-pushed the sp branch from 1291a27 to eaa0fba Compare

June 9, 2024 19:59

Collaborator Author

benma commented Jun 9, 2024 •

edited

Loading

Rebased. Need to double-check if payment requests to silent payment addresses work safely (an output that is both attached to a payment request and is generated by a SP address) and add unit tests for it, or if we should disable that combination.

Edit: I don't see a reason silent payment addresses could not safely be used within payment requests. The payment request signs for address strings, not pubkeyScripts, so it can simply contain the silent payment address.

benma mentioned this pull request

[Bounty] Implement sending to Silent Payment addresses - 0.01 BTC #1251

Closed

benma force-pushed the sp branch 9 times, most recently from 877215a to ade896c Compare

July 24, 2024 21:33

benma force-pushed the sp branch from ade896c to c9697d6 Compare

August 18, 2024 18:49

Collaborator Author

benma commented Aug 18, 2024

Rebased.

Collaborator Author

benma commented Aug 22, 2024

Host side that verifies the generated output correctness using the DLEQ proof and returns them: BitBoxSwiss/bitbox02-api-go#105

This includes a simulator test the complete integration is tested.

benma marked this pull request as ready for review

August 27, 2024 08:51

benma requested a review from NickeZ

September 1, 2024 09:34

Collaborator Author

benma commented Sep 1, 2024

@NickeZ one way to test it (except the integration/unit tests included in this PR) is an integration test on the host side with the simulator:

./scripts/docker_exec.sh make -j simulator

Then in the api-go client repo at BitBoxSwiss/bitbox02-api-go#105, run

SIMULATOR=/path/to/bitbox02-firmware/build-build/bin/simulator go test -v -run SilentPayment ./...

=== RUN   TestSimulatorBTCSignSilentPayment/simulator
--- PASS: TestSimulatorBTCSignSilentPayment (0.33s)
    --- PASS: TestSimulatorBTCSignSilentPayment/simulator (0.33s)

benma force-pushed the sp branch 2 times, most recently from 1368d52 to 77a5079 Compare

September 2, 2024 20:35

benma force-pushed the sp branch 2 times, most recently from 420fb57 to e14f3db Compare

September 17, 2024 10:08

NickeZ approved these changes

View reviewed changes

Collaborator

NickeZ left a comment

In general approved, left some minor comments.

src/rust/streaming-silent-payments/src/lib.rs Outdated


		const DLEQ_PROOF_SIZE: usize = 33 + 64;

		pub struct Output {

Collaborator

NickeZ Sep 17, 2024

Perhaps name it TransactionOutput since Output can be a bit generic

Collaborator Author

benma Sep 17, 2024

Done

src/rust/streaming-silent-payments/src/lib.rs

+              }
+              impl Network {
+                  fn sp_hrp(&self) -> &str {

Collaborator

NickeZ Sep 17, 2024

hrp is maybe a bit cryptic?

Collaborator Author

benma Sep 17, 2024

I'll add a comment what this is, but HRP is the name for that part of a bech32 address:

Referred to as HRP in https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki
Used everywhere, like

Collaborator Author

benma Sep 17, 2024

Added comment

src/rust/streaming-silent-payments/src/lib.rs Outdated

+                  SecretKey::from_slice(&hash).map_err(|_| ())
+              }
+              fn decode_address(address: &str, expected_hrp: &str) -> Result<(PublicKey, PublicKey), ()> {

Collaborator

NickeZ Sep 17, 2024

Consider returning a struct so that you can name the fields. You can still do destructuring at the call site if you like. https://doc.rust-lang.org/rust-by-example/flow_control/match/destructuring/destructure_structures.html

Collaborator Author

benma Sep 17, 2024

Done

src/rust/streaming-silent-payments/src/lib.rs

+              fn decode_address(address: &str, expected_hrp: &str) -> Result<(PublicKey, PublicKey), ()> {
+                  let mut decoded_addr =
+                      bech32::primitives::decode::CheckedHrpstring::new::<bech32::Bech32m>(address)
+                          .map_err(|_| ())?;

Collaborator

NickeZ Sep 17, 2024

For error handling, thiserror is a common crate for creating library level error enums. It has a convenient macro for wrapping dependencies errors as well. https://docs.rs/thiserror/latest/thiserror/

Another, simpler, option is to use &'static str as error type.

Throwing away errors with Err(()) feels a bit awkward. If you think it is programmer errors and not user errors, consider using .expect(reason) instead.

Collaborator Author

benma Sep 17, 2024

Skpping for now

src/rust/streaming-silent-payments/src/lib.rs Outdated

+              // - re-computing the output (https://github.com/bitcoin/bips/blob/ad1d3bc2a7b0d84247c29f847e85c35283094e2f/bip-0352.mediawiki#creating-outputs)
+              //   using ecdsa_shared_secret = input_hash·a_sum·scan_pubkey = input_hash·c_pubkey
+              // - verifying that the created output matches the re-computed output.
+              fn create_dleq_proof<C: secp256k1::Verification>(

Collaborator

NickeZ Sep 17, 2024

Is it possible to make your function non-generic and instead put Secp256k1<secp256k1::All> as the type in the argument list?

Collaborator Author

benma Sep 17, 2024

Done, worked.

src/rust/streaming-silent-payments/src/lib.rs Outdated

+                  }
+                  let data: Vec<u8> = decoded_addr.byte_iter().collect();
+                  if data.len() != 66 {

Collaborator

NickeZ Sep 17, 2024

Should magic number 66 be a const?

Collaborator Author

benma Sep 17, 2024

added const PUBKEY_LEN: usize = 33;, and used 2 * PUBKEYLEN here.

src/rust/streaming-silent-payments/src/lib.rs Outdated

+                  if data.len() != 66 {
+                      return Err(());
+                  }
+                  let scan_pubkey = PublicKey::from_slice(&data[..33]).map_err(|_| ())?;

Collaborator

NickeZ Sep 17, 2024

should magic number 33 be a const?

Collaborator Author

benma Sep 17, 2024

Done

src/rust/streaming-silent-payments/src/lib.rs

+                      input_type: InputType,
+                      input_key: &SecretKey,
+                      prevout: bitcoin::OutPoint,
+                  ) -> Result<(), ()> {

Collaborator

NickeZ Sep 17, 2024

Consider using an error type other than ().

src/keystore.h

@@ @@ -303,6 +303,12 @@ USE_RESULT bool keystore_secp256k1_schnorr_bip86_sign( @@
                   const uint8_t* msg32,
                   uint8_t* sig64_out);
+              USE_RESULT bool keystore_secp256k1_get_private_key(

Collaborator

NickeZ Sep 17, 2024

Missing doc comment for this function.

Collaborator Author

benma Sep 17, 2024

Added

messages/btc.proto Outdated

@@ @@ -137,6 +138,9 @@ message BTCSignNextResponse { @@
                 // Previous tx's input/output index in case of PREV_INPUT or PREV_OUTPUT, for the input at `index`.
                 uint32 prev_index = 5;
                 AntiKleptoSignerCommitment anti_klepto_signer_commitment = 6;
+                // Generated output. The host *must* verify its correctness using silent_payment_dleq_proof.

Collaborator

NickeZ Sep 17, 2024

If you write silent_payment_dleq_proof (with ticks `) it will be nicer formatted in the generated docs.

Collaborator Author

benma Sep 17, 2024

Done

benma added 2 commits

September 17, 2024 16:09


          bitcoin: add silent payments crate to support sending to SP addresses

39d761c

BIP-352: https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki

A silent payment address contains two pubkeys, B_scan and
B_spend. From that, Taproot outputs can be created by using a ECDH
shared secret - see the BIP Overview section.

The created output is returned to the host along with a DLEQ proof the
host can use to independently verify the output was created
correctly. This mitigates bugs, potential memory
corruption (bit-flips), etc. in the firmware leading to catastrophic
failure.

The added vendored deps serde/serde_json etc. are dev-dependencies to
run the tests in the new crate, not used for firmware builds.

**A note about multiple silent payment outputs per transaction:**

In general, a transaction can send to an arbitrary amount of silent
payment addresses. If there are multiple, then for each unique
B_scan (multiple outputs to the same recipient/B_scan), a counter `k`
is incremented. See
https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki#creating-outputs.

To be able to verify the silent payment address, we need to derive the
output and check that it matches the provided output. For each SP
output, we'd then need to know the SP address and the counter `k`.

The problem with this is that we also need to check, per B_scan, that
each `k=0, 1, ...` is used starting at zero with no holes. This
requires non-constant memory. We could still do it and support a
limited (but likely high) number of SP outputs per transaction, but
that complicates the code. For this reason, we restrict to only one SP
output per transaction.


          bitcoin: support sending to silent payment addresses

3b91825

benma force-pushed the sp branch from e14f3db to 3b91825 Compare

September 17, 2024 14:09

benma merged commit 83cd6c8 into BitBoxSwiss:master

3 checks passed

benma deleted the sp branch

September 18, 2024 07:49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet