From 8f5c10a36c247b6ba352e38f354b53abf9093e46 Mon Sep 17 00:00:00 2001 From: Marko Bencun Date: Fri, 2 May 2025 12:34:03 +0200 Subject: [PATCH 1/2] firmware/btc: return struct in BTCSign and BTCSignMessage Better clarity and composes better with generic functions returning a single value. --- api/firmware/btc.go | 88 ++++++++++++++++++++++++-------------- api/firmware/btc_test.go | 24 +++++------ cmd/miniscript/main.go | 6 +-- cmd/paymentrequest/main.go | 2 +- cmd/playground/main.go | 2 +- 5 files changed, 73 insertions(+), 49 deletions(-) diff --git a/api/firmware/btc.go b/api/firmware/btc.go index f9b2b57..d5cc419 100644 --- a/api/firmware/btc.go +++ b/api/firmware/btc.go @@ -277,22 +277,29 @@ type BTCTx struct { PaymentRequests []*messages.BTCPaymentRequestRequest } +// BTCSignResult is the result of `BTCSign()`. +type BTCSignResult struct { + // Signatures contains the input signatures. One 64 byte signature per input. + Signatures [][]byte + // GeneratedOutputs contains the outputs generated (silent payments). The map key is the input + // index, the map value is the generated pkScript. + GeneratedOutputs map[int][]byte +} + // BTCSign signs a bitcoin or bitcoin-like transaction. The previous transactions of the inputs // need to be provided if `BTCSignNeedsPrevTxs()` returns true. -// -// Returns one 64 byte signature per input. func (device *Device) BTCSign( coin messages.BTCCoin, scriptConfigs []*messages.BTCScriptConfigWithKeypath, outputScriptConfigs []*messages.BTCScriptConfigWithKeypath, tx *BTCTx, formatUnit messages.BTCSignInitRequest_FormatUnit, -) ([][]byte, map[int][]byte, error) { +) (*BTCSignResult, error) { generatedOutputs := map[int][]byte{} if !device.version.AtLeast(semver.NewSemVer(9, 10, 0)) { for _, sc := range scriptConfigs { if isTaproot(sc) { - return nil, nil, UnsupportedError("9.10.0") + return nil, UnsupportedError("9.10.0") } } } @@ -308,11 +315,11 @@ func (device *Device) BTCSign( } if containsSilentPaymentOutputs && !device.version.AtLeast(semver.NewSemVer(9, 21, 0)) { - return nil, nil, UnsupportedError("9.21.0") + return nil, UnsupportedError("9.21.0") } if len(outputScriptConfigs) > 0 && !device.version.AtLeast(semver.NewSemVer(9, 22, 0)) { - return nil, nil, UnsupportedError("9.22.0") + return nil, UnsupportedError("9.22.0") } signatures := make([][]byte, len(tx.Inputs)) @@ -330,7 +337,7 @@ func (device *Device) BTCSign( OutputScriptConfigs: outputScriptConfigs, }}}) if err != nil { - return nil, nil, err + return nil, err } isInputsPass2 := false @@ -349,7 +356,7 @@ func (device *Device) BTCSign( if performAntiklepto { nonce, err := generateHostNonce() if err != nil { - return nil, nil, err + return nil, err } hostNonce = nonce input.HostNonceCommitment = &messages.AntiKleptoHostNonceCommitment{ @@ -361,12 +368,12 @@ func (device *Device) BTCSign( BtcSignInput: input, }}) if err != nil { - return nil, nil, err + return nil, err } if performAntiklepto { if next.Type != messages.BTCSignNextResponse_HOST_NONCE || next.AntiKleptoSignerCommitment == nil { - return nil, nil, errp.New("unexpected response; expected signer nonce commitment") + return nil, errp.New("unexpected response; expected signer nonce commitment") } signerCommitment := next.AntiKleptoSignerCommitment.Commitment next, err = device.nestedQueryBtcSign( @@ -378,7 +385,7 @@ func (device *Device) BTCSign( }, }) if err != nil { - return nil, nil, err + return nil, err } err := antikleptoVerify( hostNonce, @@ -386,12 +393,12 @@ func (device *Device) BTCSign( next.Signature, ) if err != nil { - return nil, nil, err + return nil, err } } if isInputsPass2 { if !next.HasSignature { - return nil, nil, errp.New("unexpected response; expected signature") + return nil, errp.New("unexpected response; expected signature") } signatures[inputIndex] = next.Signature } @@ -413,7 +420,7 @@ func (device *Device) BTCSign( }, }) if err != nil { - return nil, nil, err + return nil, err } case messages.BTCSignNextResponse_PREVTX_INPUT: prevtxInput := tx.Inputs[next.Index].PrevTx.Inputs[next.PrevIndex] @@ -424,7 +431,7 @@ func (device *Device) BTCSign( }, }) if err != nil { - return nil, nil, err + return nil, err } case messages.BTCSignNextResponse_PREVTX_OUTPUT: prevtxOutput := tx.Inputs[next.Index].PrevTx.Outputs[next.PrevIndex] @@ -435,7 +442,7 @@ func (device *Device) BTCSign( }, }) if err != nil { - return nil, nil, err + return nil, err } case messages.BTCSignNextResponse_OUTPUT: outputIndex := next.Index @@ -444,7 +451,7 @@ func (device *Device) BTCSign( BtcSignOutput: tx.Outputs[outputIndex], }}) if err != nil { - return nil, nil, err + return nil, err } if next.GeneratedOutputPkscript != nil { generatedOutputs[int(outputIndex)] = next.GeneratedOutputPkscript @@ -455,13 +462,13 @@ func (device *Device) BTCSign( next.GeneratedOutputPkscript, ) if err != nil { - return nil, nil, err + return nil, err } } case messages.BTCSignNextResponse_PAYMENT_REQUEST: paymentRequestIndex := next.Index if int(paymentRequestIndex) >= len(tx.PaymentRequests) { - return nil, nil, errp.New("payment request index out of bounds") + return nil, errp.New("payment request index out of bounds") } paymentRequest := tx.PaymentRequests[paymentRequestIndex] next, err = device.nestedQueryBtcSign( @@ -472,10 +479,13 @@ func (device *Device) BTCSign( }, ) if err != nil { - return nil, nil, err + return nil, err } case messages.BTCSignNextResponse_DONE: - return signatures, generatedOutputs, nil + return &BTCSignResult{ + Signatures: signatures, + GeneratedOutputs: generatedOutputs, + }, nil } } } @@ -542,18 +552,28 @@ func (device *Device) BTCRegisterScriptConfig( return nil } +// BTCSignMessageResult is the result of `BTCSignMessage()`. +type BTCSignMessageResult struct { + // Signature is the 64 byte raw signature. + Signature []byte + // RecID is the recoverable ID. + RecID byte + // ElectrumSig65 is the 65 byte signature in Electrum format. + ElectrumSig65 []byte +} + // BTCSignMessage signs a Bitcoin message. The 64 byte raw signature, the recoverable ID and the 65 // byte signature in Electrum format are returned. func (device *Device) BTCSignMessage( coin messages.BTCCoin, scriptConfig *messages.BTCScriptConfigWithKeypath, message []byte, -) (raw []byte, recID byte, electrum65 []byte, err error) { +) (*BTCSignMessageResult, error) { if isTaproot(scriptConfig) { - return nil, 0, nil, errp.New("taproot not supported") + return nil, errp.New("taproot not supported") } if !device.version.AtLeast(semver.NewSemVer(9, 2, 0)) { - return nil, 0, nil, UnsupportedError("9.2.0") + return nil, UnsupportedError("9.2.0") } supportsAntiklepto := device.version.AtLeast(semver.NewSemVer(9, 5, 0)) @@ -564,7 +584,7 @@ func (device *Device) BTCSignMessage( var err error hostNonce, err = generateHostNonce() if err != nil { - return nil, 0, nil, err + return nil, err } hostNonceCommitment = &messages.AntiKleptoHostNonceCommitment{ Commitment: antikleptoHostCommit(hostNonce), @@ -583,14 +603,14 @@ func (device *Device) BTCSignMessage( } response, err := device.queryBTC(request) if err != nil { - return nil, 0, nil, err + return nil, err } var signature []byte if supportsAntiklepto { signerCommitment, ok := response.Response.(*messages.BTCResponse_AntikleptoSignerCommitment) if !ok { - return nil, 0, nil, errp.New("unexpected response") + return nil, errp.New("unexpected response") } response, err := device.queryBTC(&messages.BTCRequest{ Request: &messages.BTCRequest_AntikleptoSignature{ @@ -600,12 +620,12 @@ func (device *Device) BTCSignMessage( }, }) if err != nil { - return nil, 0, nil, err + return nil, err } signResponse, ok := response.Response.(*messages.BTCResponse_SignMessage) if !ok { - return nil, 0, nil, errp.New("unexpected response") + return nil, errp.New("unexpected response") } signature = signResponse.SignMessage.Signature err = antikleptoVerify( @@ -614,12 +634,12 @@ func (device *Device) BTCSignMessage( signature[:64], ) if err != nil { - return nil, 0, nil, err + return nil, err } } else { signResponse, ok := response.Response.(*messages.BTCResponse_SignMessage) if !ok { - return nil, 0, nil, errp.New("unexpected response") + return nil, errp.New("unexpected response") } signature = signResponse.SignMessage.Signature } @@ -628,5 +648,9 @@ func (device *Device) BTCSignMessage( // See https://github.com/spesmilo/electrum/blob/84dc181b6e7bb20e88ef6b98fb8925c5f645a765/electrum/ecc.py#L521-L523 const compressed = 4 // BitBox02 uses only compressed pubkeys electrumSig65 := append([]byte{27 + compressed + recID}, sig...) - return sig, recID, electrumSig65, nil + return &BTCSignMessageResult{ + Signature: sig, + RecID: recID, + ElectrumSig65: electrumSig65, + }, nil } diff --git a/api/firmware/btc_test.go b/api/firmware/btc_test.go index 54e92ce..00552cb 100644 --- a/api/firmware/btc_test.go +++ b/api/firmware/btc_test.go @@ -161,7 +161,7 @@ func TestSimulatorBTCSignMessage(t *testing.T) { pubKey := simulatorPub(t, device, keypath...) - sig, _, _, err := device.BTCSignMessage( + result, err := device.BTCSignMessage( coin, &messages.BTCScriptConfigWithKeypath{ ScriptConfig: NewBTCScriptConfigSimple(messages.BTCScriptConfig_P2WPKH_P2SH), @@ -171,7 +171,7 @@ func TestSimulatorBTCSignMessage(t *testing.T) { ) require.NoError(t, err) sigHash := chainhash.DoubleHashB([]byte("\x18Bitcoin Signed Message:\n\x07message")) - require.True(t, parseECDSASignature(t, sig).Verify(sigHash, pubKey)) + require.True(t, parseECDSASignature(t, result.Signature).Verify(sigHash, pubKey)) }) } @@ -352,7 +352,7 @@ func TestBTCSignMessage(t *testing.T) { generateHostNonce = func() ([]byte, error) { return hostNonce, nil } - sig, recID, electrumSig65, err := env.device.BTCSignMessage( + result, err := env.device.BTCSignMessage( messages.BTCCoin_BTC, &messages.BTCScriptConfigWithKeypath{ ScriptConfig: NewBTCScriptConfigSimple(messages.BTCScriptConfig_P2WPKH_P2SH), @@ -362,9 +362,9 @@ func TestBTCSignMessage(t *testing.T) { ) if env.version.AtLeast(semver.NewSemVer(9, 2, 0)) { require.NoError(t, err) - require.Equal(t, expectedSig[:64], sig) - require.Equal(t, byte(0), recID) - require.Equal(t, electrumSig65, append([]byte{31}, expectedSig[:64]...)) + require.Equal(t, expectedSig[:64], result.Signature) + require.Equal(t, byte(0), result.RecID) + require.Equal(t, result.ElectrumSig65, append([]byte{31}, expectedSig[:64]...)) } else { require.EqualError(t, err, UnsupportedError("9.2.0").Error()) } @@ -422,7 +422,7 @@ func TestSimulatorBTCSignTaprootKeySpend(t *testing.T) { require.False(t, BTCSignNeedsPrevTxs(scriptConfigs)) prevTxHash := prevTx.TxHash() - _, _, err := device.BTCSign( + _, err := device.BTCSign( coin, scriptConfigs, nil, @@ -551,7 +551,7 @@ func TestSimulatorBTCSignMixed(t *testing.T) { require.True(t, BTCSignNeedsPrevTxs(scriptConfigs)) prevTxHash := prevTx.TxHash() - _, _, err := device.BTCSign( + _, err := device.BTCSign( coin, scriptConfigs, nil, @@ -648,7 +648,7 @@ func TestSimulatorBTCSignSilentPayment(t *testing.T) { LockTime: 0, } prevTxHash := prevTx.TxHash() - _, generatedOutputs, err := device.BTCSign( + result, err := device.BTCSign( coin, []*messages.BTCScriptConfigWithKeypath{ { @@ -707,7 +707,7 @@ func TestSimulatorBTCSignSilentPayment(t *testing.T) { map[int][]byte{ 1: unhex("5120f99b8e8d97aa7b068dd7b4e7ae31f51784f5c2a0cae280748cfd23832b7dcba7"), }, - generatedOutputs, + result.GeneratedOutputs, ) } else { require.EqualError(t, err, UnsupportedError("9.21.0").Error()) @@ -753,7 +753,7 @@ func TestSimulatorSignBTCTransactionSendSelfSameAccount(t *testing.T) { } prevTxHash := prevTx.TxHash() - _, _, err := device.BTCSign( + _, err := device.BTCSign( coin, scriptConfigs, nil, @@ -847,7 +847,7 @@ func TestSimulatorSignBTCTransactionSendSelfDifferentAccount(t *testing.T) { outputScriptConfigIndex := uint32(0) prevTxHash := prevTx.TxHash() - _, _, err := device.BTCSign( + _, err := device.BTCSign( coin, scriptConfigs, outputScriptConfigs, diff --git a/cmd/miniscript/main.go b/cmd/miniscript/main.go index e56b996..380f584 100644 --- a/cmd/miniscript/main.go +++ b/cmd/miniscript/main.go @@ -331,7 +331,7 @@ func main() { } accountKeypath := keyOriginInfo.Keypath mp := multipaths[i] - signatures, _, err := device.BTCSign( + result, err := device.BTCSign( coin, []*messages.BTCScriptConfigWithKeypath{ { @@ -390,8 +390,8 @@ func main() { errpanic(err) // Convert to DER encoding - signaturesDER := make([][]byte, len(signatures)) - for sigIndex, signature := range signatures { + signaturesDER := make([][]byte, len(result.Signatures)) + for sigIndex, signature := range result.Signatures { r := new(btcec.ModNScalar) r.SetByteSlice(signature[:32]) s := new(btcec.ModNScalar) diff --git a/cmd/paymentrequest/main.go b/cmd/paymentrequest/main.go index 78e66f7..1deda1c 100644 --- a/cmd/paymentrequest/main.go +++ b/cmd/paymentrequest/main.go @@ -167,7 +167,7 @@ func main() { errpanic(err) paymentRequest.Signature = signature[1:] - _, _, err = device.BTCSign( + _, err = device.BTCSign( messages.BTCCoin_TBTC, []*messages.BTCScriptConfigWithKeypath{ { diff --git a/cmd/playground/main.go b/cmd/playground/main.go index aae3dce..a0f99ee 100644 --- a/cmd/playground/main.go +++ b/cmd/playground/main.go @@ -153,7 +153,7 @@ func signFromTxID(device *firmware.Device, txID string) { Value: outp.Value, }) } - _, _, err := device.BTCSign( + _, err := device.BTCSign( messages.BTCCoin_TBTC, []*messages.BTCScriptConfigWithKeypath{ { From 89d6c998a60cd4767d6871d0e444910b54d976cb Mon Sep 17 00:00:00 2001 From: Marko Bencun Date: Fri, 2 May 2025 12:20:41 +0200 Subject: [PATCH 2/2] api/firmware: make sure a series of request doesn't get interrupted Some API calls, like BTCSign, or BluetoothUpgrade, require a series of request<>response pairs. If the device gets an unexpected message, it fails. However, this is not concurrency safe - if another goroutine sends something in the middle of it, the API call in progress and the interrupting call both fail. To solve this, we introduce `apiLock` that ensures a series of queries does not get interrupted. Most API functions only use a single query, so we acquire this lock by default in `.query()`. We add a `nonAtomicQuery()` variant with a `atomicQueries()` helper function to enable making a series of calls. --- api/firmware/bluetooth.go | 72 +++++++++++++++++------------------ api/firmware/btc.go | 80 ++++++++++++++++++++++++++++----------- api/firmware/device.go | 7 ++++ api/firmware/query.go | 39 ++++++++++++++++++- 4 files changed, 136 insertions(+), 62 deletions(-) diff --git a/api/firmware/bluetooth.go b/api/firmware/bluetooth.go index ff2464b..23dcf48 100644 --- a/api/firmware/bluetooth.go +++ b/api/firmware/bluetooth.go @@ -19,12 +19,12 @@ import ( "github.com/BitBoxSwiss/bitbox02-api-go/util/errp" ) -// queryBluetooth is like query, but nested one level deeper for Bluetooth. -func (device *Device) queryBluetooth(request *messages.BluetoothRequest) (*messages.BluetoothResponse, error) { +// nonAtomicQueryBluetooth is like query, but nested one level deeper for Bluetooth. +func (device *Device) nonAtomicQueryBluetooth(request *messages.BluetoothRequest) (*messages.BluetoothResponse, error) { if !device.SupportsBluetooth() { return nil, errp.New("this device does not support Bluetooth") } - response, err := device.query(&messages.Request{ + response, err := device.nonAtomicQuery(&messages.Request{ Request: &messages.Request_Bluetooth{ Bluetooth: request, }, @@ -41,44 +41,42 @@ func (device *Device) queryBluetooth(request *messages.BluetoothRequest) (*messa // BluetoothUpgrade attempts an upgrade of the Bluetooth firmware. func (device *Device) BluetoothUpgrade(firmware []byte) error { - // Send initial upgrade request - initReq := &messages.BluetoothUpgradeInitRequest{ - FirmwareLength: uint32(len(firmware)), - } - req := &messages.BluetoothRequest{ - Request: &messages.BluetoothRequest_UpgradeInit{ - UpgradeInit: initReq, - }, - } - - currentResponse, err := device.queryBluetooth(req) - if err != nil { - return err - } + return device.atomicQueries(func() error { + currentResponse, err := device.nonAtomicQueryBluetooth(&messages.BluetoothRequest{ + Request: &messages.BluetoothRequest_UpgradeInit{ + UpgradeInit: &messages.BluetoothUpgradeInitRequest{ + FirmwareLength: uint32(len(firmware)), + }, + }, + }) + if err != nil { + return err + } - for { - switch resp := currentResponse.Response.(type) { - case *messages.BluetoothResponse_RequestChunk: - chunkReq := resp.RequestChunk - chunkData := firmware[chunkReq.Offset : chunkReq.Offset+chunkReq.Length] + for { + switch resp := currentResponse.Response.(type) { + case *messages.BluetoothResponse_RequestChunk: + chunkReq := resp.RequestChunk + chunkData := firmware[chunkReq.Offset : chunkReq.Offset+chunkReq.Length] - currentResponse, err = device.queryBluetooth(&messages.BluetoothRequest{ - Request: &messages.BluetoothRequest_Chunk{ - Chunk: &messages.BluetoothChunkRequest{ - Data: chunkData, + currentResponse, err = device.nonAtomicQueryBluetooth(&messages.BluetoothRequest{ + Request: &messages.BluetoothRequest_Chunk{ + Chunk: &messages.BluetoothChunkRequest{ + Data: chunkData, + }, }, - }, - }) - if err != nil { - return err - } + }) + if err != nil { + return err + } - case *messages.BluetoothResponse_Success: - // Upgrade complete - return nil + case *messages.BluetoothResponse_Success: + // Upgrade complete + return nil - default: - return errp.New("unexpected response type during bluetooth upgrade") + default: + return errp.New("unexpected response type during bluetooth upgrade") + } } - } + }) } diff --git a/api/firmware/btc.go b/api/firmware/btc.go index d5cc419..c9ff0cd 100644 --- a/api/firmware/btc.go +++ b/api/firmware/btc.go @@ -30,9 +30,9 @@ import ( const multisigNameMaxLen = 30 -// queryBTC is like query, but nested one level deeper. -func (device *Device) queryBTC(request *messages.BTCRequest) (*messages.BTCResponse, error) { - response, err := device.query(&messages.Request{ +// nonAtomicQueryBTC is like nonAtomicQuery, but nested one level deeper. +func (device *Device) nonAtomicQueryBTC(request *messages.BTCRequest) (*messages.BTCResponse, error) { + response, err := device.nonAtomicQuery(&messages.Request{ Request: &messages.Request_Btc{ Btc: request, }, @@ -47,6 +47,13 @@ func (device *Device) queryBTC(request *messages.BTCRequest) (*messages.BTCRespo return btcResponse.Btc, nil } +// queryBTC is like query, but nested one level deeper. +func (device *Device) queryBTC(request *messages.BTCRequest) (*messages.BTCResponse, error) { + return atomicQueriesValue(device, func() (*messages.BTCResponse, error) { + return device.nonAtomicQueryBTC(request) + }) +} + // NewBTCScriptConfigSimple is a helper to construct the correct script config for simple script // types. func NewBTCScriptConfigSimple(typ messages.BTCScriptConfig_SimpleType) *messages.BTCScriptConfig { @@ -172,9 +179,9 @@ func (device *Device) BTCAddress( return pubResponse.Pub.Pub, nil } -func (device *Device) queryBtcSign(request proto.Message) ( +func (device *Device) nonAtomicQueryBtcSign(request proto.Message) ( *messages.BTCSignNextResponse, error) { - response, err := device.query(request) + response, err := device.nonAtomicQuery(request) if err != nil { return nil, err } @@ -185,9 +192,9 @@ func (device *Device) queryBtcSign(request proto.Message) ( return next.BtcSignNext, nil } -func (device *Device) nestedQueryBtcSign(request *messages.BTCRequest) ( +func (device *Device) nonAtomicNestedQueryBtcSign(request *messages.BTCRequest) ( *messages.BTCSignNextResponse, error) { - response, err := device.queryBTC(request) + response, err := device.nonAtomicQueryBTC(request) if err != nil { return nil, err } @@ -286,9 +293,7 @@ type BTCSignResult struct { GeneratedOutputs map[int][]byte } -// BTCSign signs a bitcoin or bitcoin-like transaction. The previous transactions of the inputs -// need to be provided if `BTCSignNeedsPrevTxs()` returns true. -func (device *Device) BTCSign( +func (device *Device) nonAtomicBTCSign( coin messages.BTCCoin, scriptConfigs []*messages.BTCScriptConfigWithKeypath, outputScriptConfigs []*messages.BTCScriptConfigWithKeypath, @@ -323,7 +328,7 @@ func (device *Device) BTCSign( } signatures := make([][]byte, len(tx.Inputs)) - next, err := device.queryBtcSign(&messages.Request{ + next, err := device.nonAtomicQueryBtcSign(&messages.Request{ Request: &messages.Request_BtcSignInit{ BtcSignInit: &messages.BTCSignInitRequest{ Coin: coin, @@ -363,7 +368,7 @@ func (device *Device) BTCSign( Commitment: antikleptoHostCommit(hostNonce), } } - next, err = device.queryBtcSign(&messages.Request{ + next, err = device.nonAtomicQueryBtcSign(&messages.Request{ Request: &messages.Request_BtcSignInput{ BtcSignInput: input, }}) @@ -376,7 +381,7 @@ func (device *Device) BTCSign( return nil, errp.New("unexpected response; expected signer nonce commitment") } signerCommitment := next.AntiKleptoSignerCommitment.Commitment - next, err = device.nestedQueryBtcSign( + next, err = device.nonAtomicNestedQueryBtcSign( &messages.BTCRequest{ Request: &messages.BTCRequest_AntikleptoSignature{ AntikleptoSignature: &messages.AntiKleptoSignatureRequest{ @@ -408,7 +413,7 @@ func (device *Device) BTCSign( } case messages.BTCSignNextResponse_PREVTX_INIT: prevtx := tx.Inputs[next.Index].PrevTx - next, err = device.nestedQueryBtcSign( + next, err = device.nonAtomicNestedQueryBtcSign( &messages.BTCRequest{ Request: &messages.BTCRequest_PrevtxInit{ PrevtxInit: &messages.BTCPrevTxInitRequest{ @@ -424,7 +429,7 @@ func (device *Device) BTCSign( } case messages.BTCSignNextResponse_PREVTX_INPUT: prevtxInput := tx.Inputs[next.Index].PrevTx.Inputs[next.PrevIndex] - next, err = device.nestedQueryBtcSign( + next, err = device.nonAtomicNestedQueryBtcSign( &messages.BTCRequest{ Request: &messages.BTCRequest_PrevtxInput{ PrevtxInput: prevtxInput, @@ -435,7 +440,7 @@ func (device *Device) BTCSign( } case messages.BTCSignNextResponse_PREVTX_OUTPUT: prevtxOutput := tx.Inputs[next.Index].PrevTx.Outputs[next.PrevIndex] - next, err = device.nestedQueryBtcSign( + next, err = device.nonAtomicNestedQueryBtcSign( &messages.BTCRequest{ Request: &messages.BTCRequest_PrevtxOutput{ PrevtxOutput: prevtxOutput, @@ -446,7 +451,7 @@ func (device *Device) BTCSign( } case messages.BTCSignNextResponse_OUTPUT: outputIndex := next.Index - next, err = device.queryBtcSign(&messages.Request{ + next, err = device.nonAtomicQueryBtcSign(&messages.Request{ Request: &messages.Request_BtcSignOutput{ BtcSignOutput: tx.Outputs[outputIndex], }}) @@ -471,7 +476,7 @@ func (device *Device) BTCSign( return nil, errp.New("payment request index out of bounds") } paymentRequest := tx.PaymentRequests[paymentRequestIndex] - next, err = device.nestedQueryBtcSign( + next, err = device.nonAtomicNestedQueryBtcSign( &messages.BTCRequest{ Request: &messages.BTCRequest_PaymentRequest{ PaymentRequest: paymentRequest, @@ -490,6 +495,26 @@ func (device *Device) BTCSign( } } +// BTCSign signs a bitcoin or bitcoin-like transaction. The previous transactions of the inputs +// need to be provided if `BTCSignNeedsPrevTxs()` returns true. +func (device *Device) BTCSign( + coin messages.BTCCoin, + scriptConfigs []*messages.BTCScriptConfigWithKeypath, + outputScriptConfigs []*messages.BTCScriptConfigWithKeypath, + tx *BTCTx, + formatUnit messages.BTCSignInitRequest_FormatUnit, +) (*BTCSignResult, error) { + return atomicQueriesValue(device, func() (*BTCSignResult, error) { + return device.nonAtomicBTCSign( + coin, + scriptConfigs, + outputScriptConfigs, + tx, + formatUnit, + ) + }) +} + // BTCIsScriptConfigRegistered returns true if the script config / account is already registered. func (device *Device) BTCIsScriptConfigRegistered( coin messages.BTCCoin, @@ -562,9 +587,7 @@ type BTCSignMessageResult struct { ElectrumSig65 []byte } -// BTCSignMessage signs a Bitcoin message. The 64 byte raw signature, the recoverable ID and the 65 -// byte signature in Electrum format are returned. -func (device *Device) BTCSignMessage( +func (device *Device) nonAtomicBTCSignMessage( coin messages.BTCCoin, scriptConfig *messages.BTCScriptConfigWithKeypath, message []byte, @@ -601,7 +624,7 @@ func (device *Device) BTCSignMessage( }, }, } - response, err := device.queryBTC(request) + response, err := device.nonAtomicQueryBTC(request) if err != nil { return nil, err } @@ -612,7 +635,7 @@ func (device *Device) BTCSignMessage( if !ok { return nil, errp.New("unexpected response") } - response, err := device.queryBTC(&messages.BTCRequest{ + response, err := device.nonAtomicQueryBTC(&messages.BTCRequest{ Request: &messages.BTCRequest_AntikleptoSignature{ AntikleptoSignature: &messages.AntiKleptoSignatureRequest{ HostNonce: hostNonce, @@ -654,3 +677,14 @@ func (device *Device) BTCSignMessage( ElectrumSig65: electrumSig65, }, nil } + +// BTCSignMessage signs a Bitcoin message. +func (device *Device) BTCSignMessage( + coin messages.BTCCoin, + scriptConfig *messages.BTCScriptConfigWithKeypath, + message []byte, +) (*BTCSignMessageResult, error) { + return atomicQueriesValue(device, func() (*BTCSignMessageResult, error) { + return device.nonAtomicBTCSignMessage(coin, scriptConfig, message) + }) +} diff --git a/api/firmware/device.go b/api/firmware/device.go index cc9c9c1..c84b7fc 100644 --- a/api/firmware/device.go +++ b/api/firmware/device.go @@ -94,7 +94,14 @@ type Device struct { // nonces must match the incrementing of the device nonces, so the decryption works. Avoid the // situation where we Encrypt() locally twice, and send the queries out of order, which will // result in a failed decryption on the device. + // + // Also, every request is followed by a response, so this lock ensures the responses is matched + // to the request. queryLock sync.Mutex + // While `queryLock` ensures that the response matches the request, `apiLock` ensures that a + // series of request<>response pairs does not get interrupted. Some API calls require a series + // of calls, like signing a BTC transactions. + apiLock sync.Mutex status Status diff --git a/api/firmware/query.go b/api/firmware/query.go index 77f46ad..c150e0f 100644 --- a/api/firmware/query.go +++ b/api/firmware/query.go @@ -27,7 +27,7 @@ import ( ) const ( - // sinve v7.0.0, requets and responses are framed with hww* codes. + // sinve v7.0.0, requests and responses are framed with hww* codes. // hwwReq* are HWW-level framing opcodes of requests. @@ -130,7 +130,12 @@ func (device *Device) rawQuery(msg []byte) ([]byte, error) { return device.communication.Query(msg) } -func (device *Device) query(request proto.Message) (*messages.Response, error) { +// Sends one request and returns the matching response. +// This acquires the queryLock, so other queries can't interrupt and need to wait. +// This *does not* acquire the apiLock. +// Use this if you need to perform a series of requests that should not get interrupted. +// Only use this inside `atomicQueries()`. +func (device *Device) nonAtomicQuery(request proto.Message) (*messages.Response, error) { device.queryLock.Lock() defer device.queryLock.Unlock() if device.sendCipher == nil || !device.channelHashDeviceVerified || !device.channelHashAppVerified { @@ -180,3 +185,33 @@ func (device *Device) query(request proto.Message) (*messages.Response, error) { return response, nil } + +// Sends one request and returns the matching response. +// This acquires the queryLock, so other queries can't interrupt and need to wait. +// This also acquires the apiLock, so series of request<>responses that should not be interrupted +// don't get interrupted. Use `nonAtomicQuery()` if you need a series of request<>response pairs +// that should not be interrupted. +func (device *Device) query(request proto.Message) (*messages.Response, error) { + device.apiLock.Lock() + defer device.apiLock.Unlock() + return device.nonAtomicQuery(request) +} + +// atomicQueries runs a series of device queries while holding the `apiLock` mutex, to ensure the +// series of quries does not get interrupted. +// +// Important: only use `nonAtomicQuery` inside the callback (`query()` also acquires `apiLock` which +// would lead to a deadlock). +func (device *Device) atomicQueries(run func() error) error { + device.apiLock.Lock() + defer device.apiLock.Unlock() + return run() +} + +// atomicQueriesValue is like `atomicQueries`, but allows returning a result value for convenience. +// It is a function and not a method because Go does not support generic methods. +func atomicQueriesValue[R any](device *Device, run func() (R, error)) (R, error) { + device.apiLock.Lock() + defer device.apiLock.Unlock() + return run() +}