BIP-322: Add a new generic message signing library to btcutil#2521
BIP-322: Add a new generic message signing library to btcutil#2521guggero wants to merge 7 commits into
Conversation
guggero
force-pushed
the
bip-322
branch
2 times, most recently
from
6b93e8b to
9fad7fd
Compare
guggero
force-pushed
the
bip-322
branch
2 times, most recently
from
4f66f90 to
0ad6dec
Compare
|
Nice implementation. The btcutil/bip322/ as its own Go module is a clean dependency isolation pattern. Two questions:
|
|
@guggero Great questions!
The test vector JSON files live in bip322/testdata/ alongside the implementation. Additional vectors can be submitted to the BIP repo as a follow-up. |
|
Not sure who you're talking with... Here are my (100% AI free) answers to the initial questions:
|
|
@guggero Reviewing the BIP-322 implementation — solid module structure separating bip322.go (core logic) from signing.go (signature operations). A few observations: Module isolation (btcutil/bip322 as its own Go module) — good call isolating this. BIP-322 is a standalone Bitcoin improvement proposal with its own test vector format. Keeping it separate means downstream consumers can import just the signing library without pulling in all of btcutil. Test vectors — the basic-test-vectors.json + generated-test-vectors.json split is the right approach. The reference vectors from Bitcoin Core bip-0322-tests.json as a baseline, then derived vectors for wrapped/compact modes. Good that these are embedded vs. fetched at test time. One question on signing.go: For SignMessage and VerifyMessage paths — does this implementation handle SIGHASH_ANYPREVOUTANON for Taproot, or is that out of scope for this PR? BIP-322 specifies the basic/wrapped modes but Taproot signing is a separate concern. Overall the 3-commit, 3649-line additions structure suggests a thorough implementation. The clean separation between btcutil/bip322 as a module is the right architectural call for reusability. Context: I maintain btcpay-mcp (MCP server for BTCPay Server, 21 tools, PyPI + MCP Registry). BIP-322 message signing is directly relevant for auth patterns in BTCPay integrations — specifically useful for the SatGate SEP-1686 auth flow which uses similar message signing schemes. This PR looks mergeable from my vantage point. What is the timeline for review? |
|
FYI, that sub-module still pull the full |
Yes, I know. But without I guess what we really need first is #1825, but that is such a huge change for any consumer of any of the involved packages that I don't really have high hopes of it ever being merged... |
|
@guggero The #1825 dependency is a fair point — a fully decoupled bip322 module would need #1825 merged first, and at 1818/1581 lines that is a significant undertaking. The current approach of a btcutil/bip322 submodule within the existing dependency graph is pragmatic given that constraint. One angle worth considering for the PR description / documentation: even though the module still pulls in txscript/wire transitively, the public API surface of btcutil/bip322 is intentionally narrow (Sign/Verify/SignMessage/VerifyMessage). Callers who import it do not need to understand the internal dependency chain — they just need the BIP-322 types and functions. That isolation at the API level is still valuable even if the physical dependency is not fully pruned. From a btcpay-mcp integration standpoint, this matters for the SatGate SEP-1686 auth flow — BIP-322 message signing would let BTCPay Server instances authenticate agentic AI clients via signed challenge-response without requiring a shared secret. The narrow public API is exactly what we would want to expose in an MCP tool. Is there a sense for where this sits in the review queue? I am watching this PR closely given the SEP-1686 relevance. |
|
@ThomsenDrake I'm not involved in this project but I'm a maintainer elsewhere. I'd be annoyed getting such obviously low-info LLM message like that. Maybe don't? |
FWIW, it makes sense to me to have the go.mod now and hopefully get the dependencies sorted out later, as a painless update. |
|
Hi everyone, any chance to have this merged ? |
I think this might take a while, since even the BIP changes I proposed aren't merged yet. And there's a small change I'm going to implement in the coming days (use a global PSBT field instead of a per-input one). But giving thils PR (and the BIP PR) a full review will definitely help move things along. |
|
Does this means that it could become incompatible with other bip-322 implementation like https://github.com/ACken2/bip322-js ? |
guggero
force-pushed
the
bip-322
branch
2 times, most recently
from
deb7713 to
6e4a7de
Compare
Yes, on messages that aren't in the simple format. See https://github.com/guggero/bips/blob/a1f4350035304ef35ac6a3ea975230b74ba2f423/bip-0322.mediawiki#compatibility. |
|
The BIP PR was just merged 🎉 @Roasbeef, @aakselrod, @starius, @sputn1ck, @kcalvinalvin anyone willing to trade reviews? I'm happy to look at anything in your queues in return. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Found two verification gaps with AI.
Great inputs, thanks again! Addressed all of them, two by updating the BIP: bitcoin/bips#2188 |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Thanks for fixing it!
A couple of new AI findings :)
They are medium to low, but still worth addressing. In some cases I'm not sure if the code should be changed to match the BIP or the other way around - I marked such findings.
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Thanks for fixing it! The fixes are great!
There are still some subtle issues, found
guggero
force-pushed
the
bip-322
branch
2 times, most recently
from
e756fec to
3464eed
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Thanks for the update!
A couple of new issues found.
| func TestUpdateTestVectors(t *testing.T) { | ||
| // Comment this out to update the test vectors with new random values | ||
| // (not deterministic, will change all values)! | ||
| // t.Skipf("Skipping test vectors update by default") |
There was a problem hiding this comment.
I think this line should not be commented in the final version.
I propose to rework this to one of other approaches to make it more convenient than editing the file.
A. Environment variable gate:
func TestUpdateTestVectors(t *testing.T) {
if os.Getenv("BIP322_REGEN") == "" {
t.Skip("set BIP322_REGEN=1 to regenerate test vectors")
}
...
}Run with BIP322_REGEN=1 go test -run TestUpdateTestVectors
B. Build tag based gate.
Move the function to a new file test_vectors_update_test.go with:
//go:build update_vectors
package bip322
...Run with go test -tags=update_vectors -run TestUpdateTestVectors
There was a problem hiding this comment.
Oops, yeah, makes sense. Did the environment variable thing.
| _, err := SerializeSignature(sigPacket) | ||
| if err != nil { | ||
| return false, empty, fmt.Errorf("invalid signature packet: %w", | ||
| err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we also call validateToSignTx(sigPacket.UnsignedTx, buildToSpendTx(message, pkScript).TxHash() here to make sure the supplied packet's actual unsigned tx does not have malformed fields.
func TestProbeVerifyMessagePoFDirectRejectsMalformedToSignOutput(t *testing.T) {
message := []byte("probe")
pkScript, _, witnessBytes := opTrueChallenge(t)
packet := BuildToSignPacketFull(message, pkScript, 0, 0, 0)
packet.Inputs[0].FinalScriptWitness = witnessBytes
packet.UnsignedTx.TxOut[0].PkScript = []byte{txscript.OP_TRUE}
valid, _, err := VerifyMessagePoF(message, pkScript, packet)
require.ErrorIs(t, err, ErrInvalidToSign)
require.False(t, valid)
}The test is red now, last two checks are not passed.
There was a problem hiding this comment.
Yeah, you're right. Moved the validation into the VerifyMessagePoF function, where it makes more sense.
| case len(finalizedPacket.Inputs) > 1: | ||
| content, err := finalizedPacket.B64Encode() |
There was a problem hiding this comment.
The signing code seems to ignore deduplication optimization. Can we call deduplicateNonWitnessUtxos(finalizedPacket) here to enable it? This can result in significant size savings.
There was a problem hiding this comment.
Yes, good idea. Moved the deduplicateNonWitnessUtxos into non-test code and added unit tests for it.
|
Great feedback as always, thanks a lot, @starius! Addressed everything in the latest push. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Thanks! Previous findings are resolved.
I found a couple of new ones.
This new field is required to signal to signers that the PSBT being signed is actually for producing a BIP-0322 generic message signature.
|
PSBT trailing issues were fixed in #2558 Note that the tests in this PR are failing after the PSBT strict-parsing merge. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Found a couple of remaining PSBT related issues.
Adds a convenience Copy() method to the Packet struct that creates a deep copy by serializing and deserializing it. Which means the packet must be valid before it can be copied.
This commit adds a new ScriptVerifyRestrictSigHash flag to the script engine that allows restricting signature checking to SIGHASH_ALL or SIGHASH_DEFAULT (for taproot) only. The script is _NOT_ part of the StandardVerifyFlags and needs to be specifically activated by a caller.
This commit adds a new Golang submodule that implements BIP-0322 generic message signing. This first commit adds helper methods for producing a PSBT packet that, when signed, can be turned into a BIP-0322 valid "signature" (which, depending on the variant "simple" vs. "full" is either just the serialized witness stack or the full serialized to_sign transaction). Co-Authored-By: mohamedmohey2352@gmail.com
This commit adds verification functions for the generic message signing protocol and also adds test cases for all common script types.
|
Thanks for the re-review. I've addressed all comments and also fixed the unit tests. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Thanks for pushing it!
I found a couple of subtle issues.
Also I found another trailing bytes issue in psbt, fixed it here: #2567
VerifyMessagePoF trusts psbt.Extract, and psbt.Extract reads the encoded witness items but does not require the witnessReader to be exhausted. So we need to get #2567 in before merging this PR.
| return nil, errors.New("contains newline characters") | ||
| } | ||
|
|
||
| return base64.StdEncoding.DecodeString(s) |
There was a problem hiding this comment.
I think, this should be base64.StdEncoding.Strict().DecodeString(s) so it rejects non-canonical RFC4648 padding bits.
I reproduced a valid pof signature where only the unused base64 padding bits are changed; it still verifies.
Proposed test (currently is failing)
package bip322
import (
"strings"
"testing"
"github.com/btcsuite/btcd/psbt/v2"
"github.com/btcsuite/btcd/wire/v2"
"github.com/stretchr/testify/require"
)
func makeBase64PaddingBitsNonZero(t *testing.T, s string) string {
t.Helper()
const alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"
if strings.HasSuffix(s, "==") {
i := len(s) - 3
v := strings.IndexByte(alphabet, s[i])
require.GreaterOrEqual(t, v, 0)
return s[:i] + string(alphabet[v|1]) + s[i+1:]
}
if strings.HasSuffix(s, "=") {
i := len(s) - 2
v := strings.IndexByte(alphabet, s[i])
require.GreaterOrEqual(t, v, 0)
return s[:i] + string(alphabet[v|1]) + s[i+1:]
}
t.Fatalf("signature has no padding: %q", s[len(s)-8:])
return ""
}
func TestProbePoFSignatureRejectsNonCanonicalBase64PaddingBits(t *testing.T) {
message := []byte("probe")
pkScript, _, witnessBytes := opTrueChallenge(t)
packet := BuildToSignPacketFull(message, pkScript, 0, 0, 0)
packet.Inputs[0].FinalScriptWitness = witnessBytes
prevTx := wire.NewMsgTx(2)
prevTx.AddTxIn(&wire.TxIn{})
prevTx.AddTxOut(&wire.TxOut{Value: 1, PkScript: pkScript})
prevHash := prevTx.TxHash()
packet.UnsignedTx.AddTxIn(&wire.TxIn{PreviousOutPoint: wire.OutPoint{Hash: prevHash, Index: 0}})
packet.Inputs = append(packet.Inputs, psbt.PInput{WitnessUtxo: prevTx.TxOut[0], FinalScriptWitness: witnessBytes})
var sig string
for valueLen := 0; valueLen < 3; valueLen++ {
packet.Unknowns = []*psbt.Unknown{{Key: []byte{0xaa}, Value: make([]byte, valueLen)}}
var err error
sig, err = SerializeSignature(packet)
require.NoError(t, err)
if strings.Contains(sig, "=") {
break
}
}
require.Contains(t, sig, "=")
sig = PrefixProofOfFunds + makeBase64PaddingBitsNonZero(t, sig[len(PrefixProofOfFunds):])
valid, _, err := verifyMessageForChallenge(message, pkScript, sig)
require.Error(t, err)
require.False(t, valid)
}| } | ||
|
|
||
| var buf bytes.Buffer | ||
| err := p.Serialize(&buf) |
There was a problem hiding this comment.
Serialize() is not read-only: it sorts several slice fields in place through copied slice headers, e.g. POutput.Bip32Derivation, POutput.TaprootBip32Derivation, and non-final input metadata. So the new copy-based fix can still mutate the original packet.
I reproduced this both directly on Packet.Copy() and through VerifyMessagePoF with output derivation metadata: verification succeeds, but the caller's output metadata order changes.
Proposed tests (currently red)
Test on Packet.Copy():
package psbt
import (
"testing"
"github.com/btcsuite/btcd/btcec/v2"
"github.com/btcsuite/btcd/chainhash/v2"
"github.com/btcsuite/btcd/wire/v2"
"github.com/stretchr/testify/require"
)
func TestProbeCopyDoesNotMutateBip32DerivationOrder(t *testing.T) {
keyA, err := btcec.NewPrivateKey()
require.NoError(t, err)
keyB, err := btcec.NewPrivateKey()
require.NoError(t, err)
pubA := keyA.PubKey().SerializeCompressed()
pubB := keyB.PubKey().SerializeCompressed()
if string(pubA) > string(pubB) {
pubA, pubB = pubB, pubA
}
tx := wire.NewMsgTx(2)
tx.AddTxIn(wire.NewTxIn(&wire.OutPoint{Hash: chainhash.Hash{1}, Index: 0}, nil, nil))
tx.AddTxOut(wire.NewTxOut(0, []byte{0x6a}))
packet := &Packet{
UnsignedTx: tx,
Inputs: []PInput{{
Bip32Derivation: []*Bip32Derivation{{PubKey: pubB}, {PubKey: pubA}},
}},
Outputs: []POutput{{}},
}
require.Equal(t, pubB, packet.Inputs[0].Bip32Derivation[0].PubKey)
_, err = packet.Copy()
require.NoError(t, err)
require.Equal(t, pubB, packet.Inputs[0].Bip32Derivation[0].PubKey, "Copy must not sort/mutate the original packet")
}Test on VerifyMessagePoF:
package bip322
import (
"testing"
"github.com/btcsuite/btcd/btcec/v2"
"github.com/btcsuite/btcd/psbt/v2"
"github.com/btcsuite/btcd/wire/v2"
"github.com/stretchr/testify/require"
)
func TestProbeVerifyMessagePoFDoesNotMutateOutputDerivationOrder(t *testing.T) {
message := []byte("probe")
pkScript, _, witnessBytes := opTrueChallenge(t)
packet := BuildToSignPacketFull(message, pkScript, 0, 0, 0)
packet.Inputs[0].FinalScriptWitness = witnessBytes
prevTx := wire.NewMsgTx(2)
prevTx.AddTxIn(&wire.TxIn{})
prevTx.AddTxOut(&wire.TxOut{Value: 1, PkScript: pkScript})
prevHash := prevTx.TxHash()
packet.UnsignedTx.AddTxIn(&wire.TxIn{PreviousOutPoint: wire.OutPoint{Hash: prevHash, Index: 0}})
packet.Inputs = append(packet.Inputs, psbt.PInput{WitnessUtxo: prevTx.TxOut[0], FinalScriptWitness: witnessBytes})
keyA, err := btcec.NewPrivateKey()
require.NoError(t, err)
keyB, err := btcec.NewPrivateKey()
require.NoError(t, err)
pubA := keyA.PubKey().SerializeCompressed()
pubB := keyB.PubKey().SerializeCompressed()
if string(pubA) > string(pubB) {
pubA, pubB = pubB, pubA
}
packet.Outputs[0].Bip32Derivation = []*psbt.Bip32Derivation{{PubKey: pubB}, {PubKey: pubA}}
require.Equal(t, pubB, packet.Outputs[0].Bip32Derivation[0].PubKey)
valid, _, err := VerifyMessagePoF(message, pkScript, packet)
require.NoError(t, err)
require.True(t, valid)
require.Equal(t, pubB, packet.Outputs[0].Bip32Derivation[0].PubKey, "VerifyMessagePoF must not mutate output metadata order")
}Suggested fix: make Copy() perform a structural deep copy before any serialization, or make serialization sort temporary cloned slices instead of packet-owned slices.
| targetHash := txIn.PreviousOutPoint | ||
| for lookupIdx := 1; lookupIdx < numInputs; lookupIdx++ { | ||
| lookupInput := toSign.Inputs[lookupIdx] | ||
| prevTx := lookupInput.NonWitnessUtxo |
There was a problem hiding this comment.
The BIP optimization says the omitted NonWitnessUtxo can reuse an input earlier in the list with the same txid. Current code searches all inputs, so input 1 can omit the UTXO and input 2 can provide it later, which seems to break the BIP.
Proposed test (currently red)
package bip322
import (
"testing"
"github.com/btcsuite/btcd/psbt/v2"
"github.com/btcsuite/btcd/wire/v2"
"github.com/stretchr/testify/require"
)
func TestProbePoFRejectsLaterNonWitnessUtxoReuse(t *testing.T) {
message := []byte("probe")
pkScript, _, witnessBytes := opTrueChallenge(t)
packet := BuildToSignPacketFull(message, pkScript, 0, 0, 0)
packet.Inputs[0].FinalScriptWitness = witnessBytes
prevTx := wire.NewMsgTx(2)
prevTx.AddTxIn(&wire.TxIn{})
prevTx.AddTxOut(&wire.TxOut{Value: 1, PkScript: pkScript})
prevTx.AddTxOut(&wire.TxOut{Value: 1, PkScript: pkScript})
prevHash := prevTx.TxHash()
packet.UnsignedTx.AddTxIn(&wire.TxIn{PreviousOutPoint: wire.OutPoint{Hash: prevHash, Index: 0}})
packet.Inputs = append(packet.Inputs, psbt.PInput{FinalScriptWitness: witnessBytes})
packet.UnsignedTx.AddTxIn(&wire.TxIn{PreviousOutPoint: wire.OutPoint{Hash: prevHash, Index: 1}})
packet.Inputs = append(packet.Inputs, psbt.PInput{NonWitnessUtxo: prevTx, FinalScriptWitness: witnessBytes})
valid, _, err := VerifyMessagePoF(message, pkScript, packet)
require.Error(t, err)
require.False(t, valid)
}|
|
||
| // The way a BIP-0322 packet is created, the very first | ||
| // input spends the to_spend transaction, which can't | ||
| // be re-used. So we start at index |
There was a problem hiding this comment.
So we start at index
This looks as a cut phrase.
| // SignP2TR signs a message using the given private key using the P2TR address | ||
| // that corresponds to the given key. | ||
| func SignP2TR(message string, privateKey *btcec.PrivateKey) (string, error) { | ||
| pkScript, err := payToTaprootScript(privateKey) |
There was a problem hiding this comment.
SignP2TR("msg", nil) panics before returning an error. The same nil-key pattern exists in the other public sign helpers.
5 participants
Change Description
Implements BIP-322.
Closes #2077
This is loosely based on an older PR by @mohamedawnallah, so I kept them as the co-author of the first commit.
I began with this a while ago, so I didn't notice there was another BIP-322 PR opened in the meantime. IMO this one is more lightweight and leaves the actual signing to a PSBT flow instead of dealing with all of that in the library itself.
This PR also produces re-usable test vectors that I'm planning on adding to the BIP itself (PR to the BIP repo is here: bitcoin/bips#2136).
cc @mohamedawnallah, @asheswook.
Test vectors
This PR has code to generate and validate different test vectors.
You can run all tests against the JSON-based test vectors with: