Skip to content

Conversation

@yyforyongyu
Copy link
Collaborator

This PR fixes #1037 - the old design has evolved here, detailed below.

Initial State: A Powerful but Risky Interface

The initial design, outlined in #1037, proposed a single, powerful Signer interface. It combined routine cryptographic operations (signing transactions, deriving public keys) with highly sensitive, security-critical functions (exporting raw private keys).

While functional, this design presented several significant issues:

  1. High Security Risk: Mixing "safe" and "unsafe" methods in a single interface makes it too easy for a consumer of the API to accidentally call a dangerous method like GetPrivKeyForAddress when they intended to use a safer alternative. Security relied solely on a DANGER comment, which is insufficient for preventing misuse.
  2. Lack of Extensibility: The methods used primitive arguments (e.g., *wire.MsgTx, *input.SignDescriptor). This made the method signatures rigid and hard to extend. Adding a new feature, like support for Taproot signing, would likely require a breaking API change.
  3. Inflexible SignMessage: The message signing function was basic, only supporting ECDSA. It lacked the flexibility to support different signature schemes (like Schnorr) or formats (like compact, recoverable signatures) required by modern applications.

The Design Evolution: Security, Clarity, and Extensibility

The final API was shaped by a strong focus on creating a safer, more expressive, and future-proof interface for cryptographic operations.

Insight 1: Security Through Interface Segregation

The most critical design decision was to split the original Signer into two distinct interfaces:

  • Signer: This interface contains only the "safe" methods that do not export raw private key material. It is intended for the majority of use cases.
  • UnsafeSigner: This interface embeds the Signer interface and adds the two dangerous methods: DerivePrivKey and GetPrivKeyForAddress.

This separation forces developers to make a conscious, explicit decision to use the UnsafeSigner, dramatically reducing the risk of accidental private key exposure. It is a compile-time safeguard that makes the API inherently safer.

Insight 2: Clarity and Extensibility with Parameter Objects

Instead of passing long, complex lists of arguments, the new design uses dedicated parameter objects for its primary methods:

  • ComputeUnlockingScript uses UnlockingScriptParams.
  • ComputeRawSig uses RawSigParams.
  • SignMessage uses SignMessageIntent.

This approach has two major benefits:

  1. Clarity: It makes the code self-documenting. The parameters for each operation are grouped into a single, coherent structure.
  2. Extensibility: It allows for non-breaking API changes. New options can be added to the parameter structs without changing the method signatures, making the API much more adaptable to future protocol changes.

Insight 3: Explicit Support for Spend Types via the Strategy Pattern

To handle the complexity of signing different types of inputs (Legacy, SegWit v0, Taproot), the RawSigParams struct includes a SpendDetails field. This is a sealed interface with concrete implementations for each spend type (LegacySpendDetails, SegwitV0SpendDetails, TaprootSpendDetails).

This is an application of the Strategy Pattern. It cleanly separates the signing logic for each input type, making the code easier to understand, test, and extend. Adding support for a new input type in the future would simply require a new SpendDetails implementation, with no changes to the core Signer interface.

The Final API: Safe, Expressive, and Future-Proof

The final design provides a robust and secure foundation for all cryptographic operations in the wallet.

// Signer provides an interface for common, safe cryptographic operations.
type Signer interface {
	DerivePubKey(ctx context.Context, path BIP32Path) (*btcec.PublicKey, error)
	ECDH(ctx context.Context, path BIP32Path, pub *btcec.PublicKey) ([32]byte, error)
	SignMessage(ctx context.Context, path BIP32Path, intent *SignMessageIntent) (Signature, error)
	ComputeUnlockingScript(ctx context.Context, params *UnlockingScriptParams) (*UnlockingScript, error)
	ComputeRawSig(ctx context.Context, params *RawSigParams) (RawSignature, error)
}

// UnsafeSigner provides an interface for security-sensitive cryptographic
// operations that export raw private key material.
type UnsafeSigner interface {
	Signer
	DerivePrivKey(ctx context.Context, path BIP32Path) (*btcec.PrivateKey, error)
	GetPrivKeyForAddress(ctx context.Context, a btcutil.Address) (*btcec.PrivateKey, error)
}

Why This Design is Superior:

  1. Inherently Safer: The separation of Signer and UnsafeSigner is a powerful, compile-time security feature that prevents the accidental exposure of private keys.
  2. More Extensible: The use of parameter objects and the strategy pattern (SpendDetails) makes the API highly adaptable to future changes in the Bitcoin protocol.
  3. More Expressive and Clear: The API is more self-documenting. The intent of the caller is made explicit through the use of structs like SignMessageIntent and RawSigParams.
  4. More Powerful: The new SignMessage and ComputeRawSig methods provide the flexibility needed to support advanced, multi-party protocols like those used in Lightning.

@GustavoStingelin
Copy link
Collaborator

Is it already supposed to be reviewed?

This commit introduces a new method, GetDerivationInfo, to the
AddressManager interface. This provides a clean, context-aware way to
retrieve BIP-32 derivation information for a given wallet address.

The new method improves upon the older FetchDerivationInfo by accepting a
btcutil.Address directly, which simplifies the API and delegates script
parsing to the caller.

The error handling is also more precise, returning a specific
ErrDerivationPathNotFound for addresses that are not found, are not
public key addresses, or are imported. This is more descriptive than the
generic ErrNotMine.

The implementation uses the existing AddressInfo method to retrieve the
managed address and then calls the DerivationInfo method on it to
construct the final psbt.Bip32Derivation object.
This commit introduces the initial, high-level interfaces for the wallet's
cryptographic signing operations.

The `Signer` interface will serve as the primary, safe API for common
operations like signing and key derivation.

The `UnsafeSigner` interface embeds the `Signer` and is designated for
security-sensitive operations that export raw private key material. This
creates a clear, compile-time boundary that forces developers to
consciously opt-in to using dangerous functions.

This is the foundational step in building a clean, well-defined signing
service for the wallet.
This commit extends the `Signer` interface with two core, non-signing
cryptographic methods: `DerivePubKey` and `ECDH`.

`DerivePubKey` provides a safe way to derive a public key from a full
BIP-32 derivation path.

`ECDH` provides a method for performing a scalar multiplication between a
derived private key and a remote public key, which is a primitive needed
for various cryptographic protocols.

The `BIP32Path` struct is also introduced to serve as a clear and
unambiguous parameter for identifying the key to be used in these
operations.
This commit adds the `SignMessage` method to the `Signer` interface,
providing a flexible way to sign arbitrary messages.

The `SignMessageIntent` struct is introduced to allow the caller to
specify the exact parameters of the signing operation, such as the hash
digest to use and whether to produce an ECDSA or Schnorr signature.

The returned `Signature` is a marker interface. Detailed documentation and
examples are provided to show how the caller can use a type assertion to
retrieve the concrete signature type (`ECDSASignature`, `CompactSignature`,
or `SchnorrSignature`).
This commit introduces the `ComputeUnlockingScript` method to the `Signer`
interface. This is a high-level convenience function that generates the
complete, final script (witness and/or sigScript) required to unlock a
UTXO.

The method takes an `UnlockingScriptParams` struct which bundles all the
necessary parameters for the operation. The returned `UnlockingScript`
struct contains the raw script components, ready to be placed on a
transaction input.

This provides a clear, robust API for the most common signing operations.
This commit introduces the `ComputeRawSig` method to the `Signer`
interface. This is a low-level specialist function that generates a raw
signature for a transaction input, intended for complex, multi-party
scenarios like multisig or Lightning.

To ensure type safety and clarity, this change introduces a `SpendDetails`
sealed interface and a `RawSigParams` struct. This allows the caller to
explicitly and safely provide all necessary parameters for the various
signing protocols (Legacy, SegWit v0, and Taproot).

Compile-time interface checks are included to ensure all concrete
`SpendDetails` types correctly implement the interface.
This commit adds the `DerivePrivKey` and `GetPrivKeyForAddress` methods
to the `UnsafeSigner` interface.

These methods are explicitly marked as "unsafe" because they export raw
private key material from the wallet. By placing them in this separate,
embedded interface, we force any component that needs this level of
access to consciously and explicitly depend on the `UnsafeSigner` type.

This creates a strong, compile-time safety boundary, making it clear
when a component is using dangerous, security-sensitive functionality.
This commit adds the concrete implementation for the `DerivePubKey` method
on the `Wallet` struct, satisfying the `Signer` interface.

A new private helper, `fetchManagedPubKeyAddress`, is introduced to
encapsulate the logic for retrieving a managed address. This helper
follows best practices by minimizing the database transaction scope: it
only holds a read lock long enough to fetch the raw address data, while
all other processing happens outside the transaction.

This helper will be reused by all other `Signer` methods.
This commit adds the concrete implementation for the `SignMessage` method
on the `Wallet` struct, satisfying the `Signer` interface.

The implementation uses the `fetchManagedPubKeyAddress` helper to efficiently
and safely retrieve the required private key. The core signing logic is
dispatched to new private helpers (`signMessageSchnorr` and
`signMessageECDSA`) to handle the different signature schemes.

This provides a complete and secure implementation for signing arbitrary
messages. A `defer` statement is used to ensure the private key is zeroed
from memory after use.
This commit adds the concrete implementation for the `ComputeUnlockingScript`
method on the `Wallet` struct, satisfying the `Signer` interface.

The implementation uses the `ScriptForOutput` method to retrieve the
necessary address and script information. The core signing and script
assembly logic is dispatched to a new private helper,
`signAndAssembleScript`, which handles all supported single-signature
address types (P2PKH, P2WKH, NP2WKH, and P2TR key-path).

A `defer` statement is used to ensure the private key is zeroed from memory
after use.
This commit adds the concrete implementation for the `ComputeRawSig` method
on the `Wallet` struct, satisfying the `Signer` interface.

The implementation uses the `fetchManagedPubKeyAddress` helper to efficiently
and safely retrieve the required private key. It then delegates the final,
version-specific signing logic to the polymorphic `Sign` method on the
`SpendDetails` object provided by the caller.

A `defer` statement is used to ensure the private key is zeroed from memory
after use.
This commit adds the concrete implementations for the `DerivePrivKey` and
`GetPrivKeyForAddress` methods on the `Wallet` struct, satisfying the
`UnsafeSigner` interface.

`DerivePrivKey` reuses the efficient `fetchManagedPubKeyAddress` helper to
get the private key for a given BIP-32 path.

`GetPrivKeyForAddress` is also implemented, providing a way to retrieve
private keys for imported addresses that do not have a derivation path.

Finally, compile-time interface checks are added to ensure that the
`Wallet` struct correctly implements both the `Signer` and `UnsafeSigner`
interfaces.
The ScriptForOutput function previously returned a
`waddrmgr.ManagedPubKeyAddress`. This interface includes a `PrivKey()`
method, which meant that a component designed for managing public
address information (the AddressManager) was leaking an object that
could be used to access private keys. This is a leaky abstraction and
violates the principle of separation of concerns.

This commit refactors ScriptForOutput to return the more general
`waddrmgr.ManagedAddress` interface instead. This interface does not
provide access to private key material.

This change enforces a clean architectural boundary between the
AddressManager (responsible for public data) and the Signer (responsible
for private key operations). The Signer is now the sole component
responsible for retrieving and using private keys, which is its intended
role. This improves the security posture and clarity of the API.
@mohamedawnallah mohamedawnallah changed the base branch from txreader-interface to interface-wallet November 19, 2025 07:43
Copy link
Collaborator

@GustavoStingelin GustavoStingelin left a comment

Choose a reason for hiding this comment

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

Left some comments about the hashing functions.

Comment on lines +163 to +165
// (SHA256d) before signing. If false, a single SHA256 hash is used.
DoubleHash bool

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this flag is false, then Hash160 is being used, which is very different from a single SHA or a double SHA. Maybe we can add an enum so the caller can choose between double SHA and RIPEMD160 in a clear way. That would make the behavior easier to read.

That said, I do not know any situation in Bitcoin where we sign a digest produced with RIPEMD160. As far as I know, signatures are always made over a double SHA digest. If there is a real reason to support RIPEMD160 here, it would be great to explain that context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this flag is false, then Hash160 is being used, which is very different from a single SHA or a double SHA. Maybe we can add an enum so the caller can choose between double SHA and RIPEMD160 in a clear way. That would make the behavior easier to read.

Good point. I have pending review comment which something along those lines. Why aren't we mirroring the LND one (HashB returns 32 bytes when doubleHash is false)?
https://github.com/lightningnetwork/lnd/blob/8c8662c86ae5dfa371941086bb828b0af060f7fd/lnwallet/btcwallet/signer.go#L416-L421

Comment on lines +598 to +602
} else {
digest = btcutil.Hash160(intent.Msg)
}

sig, err := schnorr.Sign(privKey, digest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Schnorr signer only accepts digests that are exactly 32 bytes. RIPEMD160 returns a 20 byte value, so this will fail right away. I am trying to understand the reason for using this hash here. Is there any case where we need this shorter digest for signing?

I also do not see why we cannot sign with Schnorr without a tag. This seems a bit tied to the way Taproot builds its tagged hashing, but Schnorr itself does not require that. This could be an opportunity to improve this design.

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

Labels

P1 High Priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants