Feat: admin subprotocol hardware wallet compatibility. #1163

irnb · 2025-11-25T10:01:56Z

Description

the admin subprotocol now uses individual ECDSA signatures instead of MuSig2 aggregated Schnorr signatures for hardware wallet compatibility.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests
Dependency Update

Notes to Reviewers

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added (where necessary) tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.
I have disclosed my use of AI in the body of this PR.

Related Issues

barakshani · 2025-11-25T11:23:20Z

Does the admin spec represent this change?

irnb · 2025-11-25T13:04:09Z

@barakshani

I'm not sure at the moment, but I'll make sure to update the spec accordingly before marking this PR as ready for merge.

prajwolrg

Good work! Looks clean! I just have some minor nits and feedbacks. As part of the PR, the spec docs also needs to be updated.

crates/crypto/src/test_utils/schnorr.rs

crates/crypto/src/threshold_signing/indexed_signatures/errors.rs

crates/crypto/src/threshold_signature/indexed/signature.rs

crates/crypto/src/threshold_signing/indexed_signatures/verification.rs

barakshani

Using threshold for naming is welcome!

crates/asm/subprotocols/admin/src/authority.rs

crates/crypto/src/threshold_signing/indexed_signatures/verification.rs

crates/asm/subprotocols/admin/src/error.rs

crates/crypto/src/threshold_signature/indexed/config.rs

crates/crypto/src/threshold_signature/indexed/signature.rs

crates/crypto/src/threshold_signing/indexed_signatures/verification.rs

…ECDSA signatures

storopoli

I really don't like having threshold as an u8 since it can include 0. We should use NonZero<u8> from Rust's standard library.

crates/crypto/src/threshold_signature/indexed/pubkey.rs

crates/crypto/src/threshold_signature/indexed/config.rs

crates/crypto/src/test_utils/schnorr.rs

crates/crypto/src/threshold_signature/indexed/config.rs

crates/crypto/src/threshold_signing/indexed_signatures/errors.rs

crates/crypto/src/threshold_signature/indexed/signature.rs

crates/crypto/src/threshold_signing/indexed_signatures/verification.rs

crates/asm/subprotocols/admin/src/authority.rs

…toring

codecov · 2025-12-01T18:53:26Z

Codecov Report

❌ Patch coverage is 91.03535% with 71 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.17%. Comparing base (2a072bc) to head (f758030).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/crypto/src/threshold_signature/indexed/pubkey.rs	71.26%	25 Missing ⚠️
...s/crypto/src/threshold_signature/indexed/config.rs	87.74%	19 Missing ⚠️
...rypto/src/threshold_signature/indexed/signature.rs	89.21%	11 Missing ⚠️
...s/crypto/src/threshold_signature/indexed/errors.rs	0.00%	6 Missing ⚠️
crates/asm/subprotocols/admin/src/config.rs	25.00%	3 Missing ⚠️
...ates/asm/txs/admin/src/actions/updates/multisig.rs	0.00%	3 Missing ⚠️
crates/crypto/src/musig2/aggregation.rs	95.00%	2 Missing ⚠️
.../threshold_signature/indexed/verification/ecdsa.rs	98.01%	2 Missing ⚠️

@@            Coverage Diff             @@
##             main    #1163      +/-   ##
==========================================
+ Coverage   75.90%   76.17%   +0.27%     
==========================================
  Files         528      546      +18     
  Lines       43905    45087    +1182     
==========================================
+ Hits        33325    34345    +1020     
- Misses      10580    10742     +162

Flag	Coverage Δ
functional	`51.25% <5.41%> (+0.52%)`	⬆️
unit	`56.28% <91.03%> (+0.72%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
crates/asm/subprotocols/admin/src/authority.rs	`90.00% <100.00%> (ø)`
crates/asm/subprotocols/admin/src/handler.rs	`93.93% <100.00%> (+1.09%)`	⬆️
crates/asm/subprotocols/admin/src/state.rs	`99.40% <100.00%> (+2.63%)`	⬆️
...s/asm/subprotocols/bridge-v1/src/state/operator.rs	`97.19% <ø> (-0.02%)`	⬇️
crates/asm/txs/admin/src/parser.rs	`100.00% <100.00%> (ø)`
crates/asm/txs/admin/src/test_utils/mod.rs	`100.00% <100.00%> (ø)`
crates/crypto/src/test_utils/schnorr.rs	`98.27% <100.00%> (+0.06%)`	⬆️
...to/src/threshold_signature/indexed/verification.rs	`100.00% <100.00%> (ø)`
crates/l1tx/src/utils.rs	`70.87% <ø> (ø)`
crates/crypto/src/musig2/aggregation.rs	`95.00% <95.00%> (ø)`
... and 7 more

... and 122 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…lds, and fix zkvm serde

github-actions · 2025-12-01T21:19:05Z

Commit: b691a8e

SP1 Execution Results

program	cycles	success
EVM EE STF	1,304,622	✅
CL STF	211,071	✅
Checkpoint	2,242	✅

…t compatibility

irnb · 2025-12-02T10:37:17Z

@prajwolrg @barakshani @storopoli

Thanks for the review!
I’ve addressed most of the comments, improved the naming, fixed a potential issue with how the hardware wallet’s recovery_Id is handled, and updated the spec accordingly.

For reference, here’s the signature format I used while verifying the fix:

┌─────────────────────────────────────────────────────────────────┐
│                    SIGNATURE FORMAT                             │
├─────────────────────────────────────────────────────────────────┤
│                                                                 │
│  Hardware Wallet Output (BIP-137 Bitcoin Message Signing):      │
│  ┌────────┬──────────────────────┬──────────────────────┐      │
│  │ Header │         R            │         S            │      │
│  │ 1 byte │      32 bytes        │      32 bytes        │      │
│  │ 27-42  │                      │                      │      │
│  └────────┴──────────────────────┴──────────────────────┘      │
│                                                                 │
│  secp256k1 library expects:                                     │
│  ┌────────┬──────────────────────┬──────────────────────┐      │
│  │ recid  │         R            │         S            │      │
│  │ 0-3    │      32 bytes        │      32 bytes        │      │
│  └────────┴──────────────────────┴──────────────────────┘      │
│                                                                 │
│  NORMALIZATION NEEDED:                                          │
│  ───────────────────────────────────────────────────────        │
│  27-30 → subtract 27 → recid 0-3 (uncompressed P2PKH)           │
│  31-34 → subtract 31 → recid 0-3 (compressed P2PKH)             │
│  35-38 → subtract 35 → recid 0-3 (SegWit P2SH)                  │
│  39-42 → subtract 39 → recid 0-3 (Native SegWit)                │
│  0-3   → use as-is   → recid 0-3 (raw format)                   │
│                                                                 │
│  NOTE:                                                          │
│  The address-type hint (27 vs 31 vs 35 vs 39 base) is just      │
│  "noise" that hardware wallets add for Bitcoin specific         │
│  workflows where an address is derived and shown to users.      │
│                                                                 │
└─────────────────────────────────────────────────────────────────┘

I’ll do another review pass to ensure everything is fully addressed.

crates/crypto/src/threshold_signature/indexed/config.rs

…toring

crates/l1tx/src/utils.rs

crates/crypto/src/threshold_signature/indexed/signature.rs

crates/crypto/src/threshold_signature/indexed/config.rs

crates/asm/txs/admin/Cargo.toml

storopoli

ACK 5cf0e39

From my side OK. Note that we have other review comments. Especially the trait-based one from @barakshani.

irnb · 2025-12-03T08:14:47Z

Especially the trait-based

#1163 (comment)

@storopoli

storopoli · 2025-12-03T16:28:37Z

Ooops removed from the queue. @AaronFeickert can you please take a look at the cryptographic functions in strata-crypto please?

AaronFeickert · 2025-12-04T19:44:27Z

crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs

+/// Verifies each ECDSA signature in the set against the corresponding public key.
+///
+/// This function performs the actual ECDSA signature recovery and verification.
+/// It assumes the SignatureSet has already been validated for duplicates.


This comment seems a bit off. Is it not the case that a SignatureSet is already guaranteed not to contain duplicates? If so, this function's caller does not need to concern itself with this.

AaronFeickert · 2025-12-04T19:49:50Z

crates/crypto/src/threshold_signature/indexed/signature.rs

+        // Check for duplicate indices
+        for window in signatures.windows(2) {
+            if window[0].index == window[1].index {
+                return Err(ThresholdSignatureError::DuplicateSignerIndex(
+                    window[0].index,
+                ));
+            }
+        }


Recommend using an internal hash set to check for duplicates instead. It's a very clean approach.

AaronFeickert · 2025-12-04T19:53:13Z

crates/crypto/src/threshold_signature/indexed/signature.rs

+
+/// A set of indexed ECDSA signatures for threshold verification.
+///
+/// Signatures are sorted by index and must not contain duplicates.


This comment seems a bit wonky. Since the constructor enforces uniqueness, why not change to indicate that a SignatureSet will not contain duplicates?

barakshani · 2025-12-04T20:00:00Z

crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs

+
+/// Verifies each ECDSA signature in the set against the corresponding public key.
+///
+/// This function performs the actual ECDSA signature recovery and verification.


Suggested change

/// This function performs the actual ECDSA signature recovery and verification.

/// This function performs pk extraction from the ECDSA signature, and matches with the config.

barakshani · 2025-12-04T20:01:05Z

crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs

+            RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id)
+                .map_err(|_| ThresholdSignatureError::InvalidSignatureFormat)?;
+
+        // Hardware wallets emit recoverable signatures (with a header). Recover first so we


Suggested change

// Hardware wallets emit recoverable signatures (with a header). Recover first so we

// Hardware wallets support recoverable public key from signatures. Recover first so we

barakshani · 2025-12-05T21:03:41Z

crates/asm/subprotocols/admin/src/authority.rs

-/// seqno.
-#[derive(Clone, Debug, Eq, PartialEq, Arbitrary, BorshDeserialize, BorshSerialize)]
+/// Manages threshold signature operations for a given role and key set, with replay protection via
+/// a seqno.


Suggested change

/// a seqno.

/// a sequence number.

barakshani · 2025-12-05T21:08:20Z

crates/asm/subprotocols/admin/src/authority.rs

+    /// This function is intentionally ECDSA-specific as part of the hardware wallet
+    /// compatibility design (BIP-137 format support). A trait-based abstraction
+    /// could be added in the future if multiple signature schemes are needed.


Convert to a comment rather than a docstring.

barakshani · 2025-12-05T21:13:18Z

crates/asm/txs/admin/src/test_utils/mod.rs

+    actions::MultisigAction, constants::ADMINISTRATION_SUBPROTOCOL_ID, parser::SignedPayload,
+};
+
+/// Creates an ECDSA recoverable signature for a message hash.


Suggested change

/// Creates an ECDSA recoverable signature for a message hash.

/// Creates an ECDSA signature with recoverable pk for a message hash.

barakshani · 2025-12-05T21:17:47Z

crates/crypto/src/threshold_signature/indexed/mod.rs

@@ -0,0 +1,17 @@
+//! Individual ECDSA signature set for threshold signatures (M-of-N).


What does "individual" mean here? I think we can omit this word.

barakshani · 2025-12-05T21:18:01Z

crates/crypto/src/threshold_signature/indexed/mod.rs

@@ -0,0 +1,17 @@
+//! Individual ECDSA signature set for threshold signatures (M-of-N).
+//!
+//! This module provides types and functions for verifying a set of individual


barakshani · 2025-12-05T21:18:57Z

crates/crypto/src/threshold_signature/indexed/signature.rs

+
+use super::ThresholdSignatureError;
+
+/// An individual ECDSA signature with its signer index.


Suggested change

/// An individual ECDSA signature with its signer index.

/// An ECDSA signature with its signer index.

irnb changed the title ~~change admin subprotocol from aggregated Schnorr to individual ecdsa signature~~ Feat: admin subprotocol hardware wallet compatibility. Nov 25, 2025

prajwolrg requested changes Nov 25, 2025

View reviewed changes

crates/crypto/src/test_utils/schnorr.rs Show resolved Hide resolved

crates/crypto/src/threshold_signing/indexed_signatures/errors.rs Outdated Show resolved Hide resolved

crates/crypto/src/threshold_signature/indexed/signature.rs Outdated Show resolved Hide resolved

prajwolrg requested changes Nov 25, 2025

View reviewed changes

crates/crypto/src/threshold_signing/indexed_signatures/verification.rs Outdated Show resolved Hide resolved

prajwolrg assigned irnb Nov 27, 2025

barakshani reviewed Nov 30, 2025

View reviewed changes

irnb added 2 commits December 1, 2025 20:25

feat: change admin subprotocol from aggregated Schnorr to individual …

9c1f472

…ECDSA signatures

fix: lint

4d5bdcc

storopoli requested changes Dec 1, 2025

View reviewed changes

fix(crypto): address PR review comments for threshold signature refac…

b08f24d

…toring

irnb force-pushed the STR-1875-change-aggregated-signature-to-multiple-signature branch from 2d9915d to b08f24d Compare December 1, 2025 18:23

fix(crypto): use NonZero<u8> for threshold, encapsulate signature fie…

3124d65

…lds, and fix zkvm serde

fix(crypto): add BIP-137 recovery ID normalization for hardware walle…

52c2955

…t compatibility

irnb marked this pull request as ready for review December 2, 2025 11:39

irnb requested review from a team as code owners December 2, 2025 11:39

storopoli requested changes Dec 2, 2025

View reviewed changes

crates/crypto/src/threshold_signature/indexed/config.rs Outdated Show resolved Hide resolved

fix(crypto): address PR review comments for threshold signature refac…

5cf0e39

…toring

irnb requested review from barakshani, prajwolrg and storopoli December 2, 2025 14:21

fix(crypto): use NonZero<u8> for threshold

8ace955

prajwolrg requested changes Dec 2, 2025

View reviewed changes

fix(crypto): use derive for Borsh serialization and fix rustdoc link

11e876e

irnb added 2 commits December 2, 2025 20:17

fix(crypto): improve comment & fix off-by-one error

5e45b58

fix(crypto): fix lint

f758030

storopoli approved these changes Dec 2, 2025

View reviewed changes

irnb requested a review from prajwolrg December 3, 2025 08:08

prajwolrg approved these changes Dec 3, 2025

View reviewed changes

prajwolrg requested a review from AaronFeickert December 3, 2025 09:49

storopoli added this pull request to the merge queue Dec 3, 2025

storopoli removed this pull request from the merge queue due to a manual request Dec 3, 2025

storopoli requested a review from prajwolrg December 3, 2025 16:31

prajwolrg approved these changes Dec 4, 2025

View reviewed changes

AaronFeickert requested changes Dec 4, 2025

View reviewed changes

barakshani reviewed Dec 4, 2025

View reviewed changes

barakshani reviewed Dec 5, 2025

View reviewed changes

	/// This function performs the actual ECDSA signature recovery and verification.
	/// This function performs pk extraction from the ECDSA signature, and matches with the config.

	// Hardware wallets emit recoverable signatures (with a header). Recover first so we
	// Hardware wallets support recoverable public key from signatures. Recover first so we

	/// Creates an ECDSA recoverable signature for a message hash.
	/// Creates an ECDSA signature with recoverable pk for a message hash.

		@@ -0,0 +1,17 @@
		//! Individual ECDSA signature set for threshold signatures (M-of-N).


		use super::ThresholdSignatureError;

		/// An individual ECDSA signature with its signer index.

	/// An individual ECDSA signature with its signer index.
	/// An ECDSA signature with its signer index.

Feat: admin subprotocol hardware wallet compatibility. #1163

Are you sure you want to change the base?

Feat: admin subprotocol hardware wallet compatibility. #1163

Uh oh!

Conversation

irnb commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

Uh oh!

barakshani commented Nov 25, 2025

Uh oh!

irnb commented Nov 25, 2025

Uh oh!

prajwolrg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barakshani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

irnb commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

irnb commented Dec 3, 2025

Uh oh!

storopoli commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

irnb commented Nov 25, 2025 •

edited

Loading

prajwolrg left a comment •

edited

Loading

codecov bot commented Dec 1, 2025 •

edited

Loading

github-actions bot commented Dec 1, 2025 •

edited

Loading

irnb commented Dec 2, 2025 •

edited

Loading