nik/liana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

commands: do not include BIP32 derivations for other spending paths

Browse Source 
When creating a PSBT, only include the BIP32 derivations in each input
for the spending path this PSBT was created for.

This is to workaround Bitbox only providing a single signature per
input. Most likely other signing devices will have this behaviour too in
the future. See
https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808.
This commit is contained in:
Antoine Poinsot 2023-10-06 17:57:17 +02:00
parent 19491d1d00
commit a81d39c81a
No known key found for this signature in database
GPG Key ID: E13FC145CD3F4304
5 changed files with 79 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
doc/API.md
View File

@ -287,8 +287,8 @@ Create a transaction that sweeps all coins for which a timelocked recovery path
currently available to a provided address with the provided feerate.
The `timelock` parameter can be used to specify which recovery path to use. By default,
we'll use the first recovery path available. If created for a later timelock a recovery
transaction may be satisfied using an earlier timelock but not the opposite.
we'll use the first recovery path available. The provided timelock must match the value of one
of the recovery path.
Due to the fact coins are generally received at different block heights, not all coins may be
spendable through a single recovery path at the same time.
@ -296,6 +296,14 @@ spendable through a single recovery path at the same time.
This command will error if no such coins are available or the sum of their value is not enough to
cover the requested feerate.
Note that although a transaction created for a later available timelock can be satisfied using the
keys for an earlier timelock, a PSBT created with this command will only be usable for a single
recovery path. If the user creates a PSBT for recovery path "B" and then ends up willing to satisfy
it using signatures for earlier recovery path "A", they will have to create a new recovery PSBT for
path "A" using `createrecovery`. This is because we do not include the information necessary to sign
using other recovery paths to workaround some signing devices' surprising behaviour. (See
https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808 for more details.)
#### Request
| Field | Type | Description |

58
src/commands/mod.rs
View File

@ -7,7 +7,8 @@ mod utils;
use crate::{
bitcoin::BitcoinInterface,
database::{Coin, DatabaseInterface},
descriptors, DaemonControl, VERSION,
descriptors::{self, keys},
DaemonControl, VERSION,
};
pub use crate::database::{CoinStatus, LabelItem};
@ -71,6 +72,7 @@ pub enum CommandError {
InsaneRescanTimestamp(u32),
/// An error that might occur in the racy rescan triggering logic.
RescanTrigger(String),
UnknownRecoveryTimelock(u16),
RecoveryNotAvailable,
}
@ -132,6 +134,7 @@ impl fmt::Display for CommandError {
),
Self::InsaneRescanTimestamp(t) => write!(f, "Insane timestamp '{}'.", t),
Self::RescanTrigger(s) => write!(f, "Error while starting rescan: '{}'", s),
Self::UnknownRecoveryTimelock(tl) => write!(f, "Provided timelock does not correspond to any recovery path: '{}'.", tl),
Self::RecoveryNotAvailable => write!(
f,
"No coin currently spendable through this timelocked recovery path."
@ -345,6 +348,23 @@ impl DaemonControl {
}
let mut db_conn = self.db.connection();
// Some signing devices (such as the Bitbox02) would not sign for all keys present in a
// script. This can lead to disruptions. For instance it could not provide a signature for
// the same key across a PSBT inputs. (See
// https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808 for details.)
// Therefore, only include in the PSBT_IN_BIP32_DERIVATION field the keys for the spending
// path we are interested in. Here the primary path. This guides the signer to sign for
// what we expect, and as long as the same signer isn't reused within the same spending
// path the signatures it will return will be consistent across inputs.
let (_, prim_path_origins) = self
.config
.main_descriptor
.policy()
.primary_path()
.thresh_origins();
let is_prim_path_key =
|pubkey: &keys::DerivedPublicKey| prim_path_origins.contains_key(&pubkey.origin.0);
// Iterate through given outpoints to fetch the coins (hence checking their existence
// at the same time). We checked there is at least one, therefore after this loop the
// list of coins is not empty.
@ -393,7 +413,7 @@ impl DaemonControl {
script_pubkey: coin_desc.script_pubkey(),
});
let non_witness_utxo = spent_txs.get(op).cloned();
let bip32_derivation = coin_desc.bip32_derivations();
let bip32_derivation = coin_desc.bip32_derivations(is_prim_path_key);
psbt_ins.push(PsbtIn {
witness_script,
witness_utxo,
@ -428,7 +448,9 @@ impl DaemonControl {
} else {
self.config.main_descriptor.receive_descriptor()
};
desc.derive(index, &self.secp).bip32_derivations()
// NOTE: include all derivations for change outputs, to make sure signers are
// able to re-compute change addresses.
desc.derive(index, &self.secp).bip32_derivations(|_| true)
} else {
Default::default()
};
@ -506,8 +528,10 @@ impl DaemonControl {
// TODO: shuffle once we have Taproot
change_txo.value = change_amount.to_sat();
tx.output.push(change_txo);
// NOTE: include all derivations for change outputs, to make sure signers are
// able to re-compute change addresses.
psbt_outs.push(PsbtOut {
bip32_derivation: change_desc.bip32_derivations(),
bip32_derivation: change_desc.bip32_derivations(|_| true),
..PsbtOut::default()
});
} else if is_self_send {
@ -761,6 +785,30 @@ impl DaemonControl {
.unwrap_or(false)
});
// Some signing devices (such as the Bitbox02) would not sign for all keys present in a
// script. This can lead to disruptions. For instance it could not provide a signature for
// the same key across a PSBT inputs. (See
// https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808 for details.)
// Therefore, only include in the PSBT_IN_BIP32_DERIVATION field the keys for the requested
// recovery path. This guides the signer toward signing only for this path, and as long as
// the same signer isn't reused within the same spending path the signatures it will return
// will be consistent across inputs.
// Note it's particularly unfortunate for a recovery PSBT. This is because absent this
// restriction a (set of) user(s) could satisfy any previously available paths (the primary
// path and any timelocked path which necessitates a lower sequence) to finalize a PSBT.
// Now they can only create and pass around a PSBT that can be used to sign for a single
// recovery path.
let (_, reco_path_origins) = self
.config
.main_descriptor
.policy()
.recovery_paths()
.get(&timelock)
.ok_or(CommandError::UnknownRecoveryTimelock(timelock))?
.thresh_origins();
let is_key_for_this_path =
|pubkey: &keys::DerivedPublicKey| reco_path_origins.contains_key(&pubkey.origin.0);
// Fill-in the transaction inputs and PSBT inputs information. Record the value
// that is fed to the transaction while doing so, to compute the fees afterward.
let mut in_value = bitcoin::Amount::from_sat(0);
@ -793,7 +841,7 @@ impl DaemonControl {
script_pubkey: coin_desc.script_pubkey(),
});
let non_witness_utxo = spent_txs.get(&coin.outpoint).cloned();
let bip32_derivation = coin_desc.bip32_derivations();
let bip32_derivation = coin_desc.bip32_derivations(is_key_for_this_path);
psbt.inputs.push(PsbtIn {
witness_script,
witness_utxo,

16
src/descriptors/mod.rs
View File

@ -378,7 +378,11 @@ impl DerivedSinglePathLianaDesc {
self.0.explicit_script().expect("Not a Taproot descriptor")
}
pub fn bip32_derivations(&self) -> Bip32Deriv {
/// Get the BIP32 derivations for this derived descriptor, filtered by the given predicate.
pub fn bip32_derivations<P>(&self, mut predicate: P) -> Bip32Deriv
where
P: FnMut(&DerivedPublicKey) -> bool,
{
let ms = match self.0 {
descriptor::Descriptor::Wsh(ref wsh) => match wsh.as_inner() {
descriptor::WshInner::Ms(ms) => ms,
@ -391,7 +395,13 @@ impl DerivedSinglePathLianaDesc {
// For DerivedPublicKey, Pk::Hash == Self.
ms.iter_pk()
.map(|k| (k.key.inner, (k.origin.0, k.origin.1)))
.filter_map(|k| {
if predicate(&k) {
Some((k.key.inner, (k.origin.0, k.origin.1)))
} else {
None
}
})
.collect()
}
}
@ -630,7 +640,7 @@ mod tests {
// Sanity check we can call the methods on the derived desc
der_desc.script_pubkey();
der_desc.witness_script();
assert!(!der_desc.bip32_derivations().is_empty());
assert!(!der_desc.bip32_derivations(|_| true).is_empty());
}
#[test]

1
src/jsonrpc/mod.rs
View File

@ -164,6 +164,7 @@ impl From<commands::CommandError> for Error {
| commands::CommandError::SpendFinalization(..)
| commands::CommandError::InsaneRescanTimestamp(..)
| commands::CommandError::AlreadyRescanning
| commands::CommandError::UnknownRecoveryTimelock(..)
| commands::CommandError::RecoveryNotAvailable => {
Error::new(ErrorCode::InvalidParams, e.to_string())
}

4
src/signer.rs
View File

@ -456,7 +456,7 @@ mod tests {
unknown: BTreeMap::new(),
inputs: vec![PsbtIn {
witness_script: Some(spent_coin_desc.witness_script()),
bip32_derivation: spent_coin_desc.bip32_derivations(),
bip32_derivation: spent_coin_desc.bip32_derivations(|_| true),
witness_utxo: Some(bitcoin::TxOut {
value: 19_000,
script_pubkey: spent_coin_desc.script_pubkey(),
@ -499,7 +499,7 @@ mod tests {
let other_spent_coin_desc = desc.receive_descriptor().derive(84.into(), &secp);
dummy_psbt.inputs.push(PsbtIn {
witness_script: Some(other_spent_coin_desc.witness_script()),
bip32_derivation: other_spent_coin_desc.bip32_derivations(),
bip32_derivation: other_spent_coin_desc.bip32_derivations(|_| true),
witness_utxo: Some(bitcoin::TxOut {
value: 19_000,
script_pubkey: other_spent_coin_desc.script_pubkey(),