Panic on duplicate keys in Unvault descriptor (again!)

[darosior]

Some of revaultd's unit test are using a hardcoded configuration and transactions (notably the RPC unit tests). The configuration contained a key of the managers that was, when derived at index 0, the same as one of the raw cosigning servers' key. This causes a panic in

revault_tx/src/txouts.rs

Lines 47 to 57 in f2735d8

	/// Get the maximum size, in weight units, a satisfaction for this scriptPubKey would cost.
	fn max_sat_weight(&self) -> usize {
	miniscript::descriptor::Wsh::new(
	miniscript::Miniscript::parse(self.witness_script())
	.expect("The witness_script is always created from a Miniscript"),
	)
	.expect("The witness_script is always a P2WSH")
	.max_satisfaction_weight()
	.expect("It's a sane Script, derived from a Miniscript")
	}
	}

failures:

---- commands::utils::tests::test_presigned_txs stdout ----
thread 'commands::utils::tests::test_presigned_txs' panicked at 'The witness_script is always created from a Miniscript: AnalysisError(RepeatedPubkeys)', /home/darosior/.cargo/git/checkouts/revault_tx-03f31ef291600939/6d0ad62/src/txouts.rs:54:18
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/panicking.rs:143:14
   2: core::result::unwrap_failed
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/result.rs:1749:5
   3: core::result::Result<T,E>::expect
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/result.rs:1022:23
   4: revault_tx::txouts::RevaultInternalTxOut::max_sat_weight
             at /home/darosior/.cargo/git/checkouts/revault_tx-03f31ef291600939/6d0ad62/src/txouts.rs:53:13
   5: revault_tx::transactions::cancel::CancelTransaction::new
             at /home/darosior/.cargo/git/checkouts/revault_tx-03f31ef291600939/6d0ad62/src/transactions/cancel.rs:50:26
   6: revault_tx::transactions::CancelTransactionsBatch::new
             at /home/darosior/.cargo/git/checkouts/revault_tx-03f31ef291600939/6d0ad62/src/transactions/mod.rs:602:25
   7: revault_tx::transactions::transaction_chain_manager
             at /home/darosior/.cargo/git/checkouts/revault_tx-03f31ef291600939/6d0ad62/src/transactions/mod.rs:731:24
   8: revaultd::commands::utils::presigned_txs
             at ./src/commands/utils.rs:164:33
   9: revaultd::commands::utils::tests::test_presigned_txs
             at ./src/commands/utils.rs:978:25
  10: revaultd::commands::utils::tests::test_presigned_txs::{{closure}}
             at ./src/commands/utils.rs:963:5
  11: core::ops::function::FnOnce::call_once
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/ops/function.rs:227:5
  12: core::ops::function::FnOnce::call_once
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    commands::utils::tests::test_presigned_txs

test result: FAILED. 32 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.55s

So, multiple things.

1. It's probably a better design to just have the Miniscript be part of the txouts structs instead of the raw Bitcoin Script?

   revault_tx/src/txouts.rs

   Lines 63 to 67 in f2735d8

   	pub struct $struct_name {
   	txout: TxOut,
   	witness_script: Script,
   	bip32_derivation: Bip32Deriv,
   	}

   Note this would only hide the issue as it would not perform the sanity check.
2. In any case, this panic should never happen: we should have checked the descriptor when deriving it, since we can't check the general descriptor for this case.

   revault_tx/src/scripts.rs

   Lines 169 to 194 in f2735d8

   	/// Derives all wildcard keys in the descriptor using the supplied `child_number`
   	pub fn derive<C: secp256k1::Verification>(
   	&self,
   	child_number: bip32::ChildNumber,
   	secp: &secp256k1::Secp256k1<C>,
   	) -> $derived_struct_name {
   	$derived_struct_name(
   	self.0
   	.derive(child_number.into())
   	.translate_pk2(|xpk| {
   	xpk.derive_public_key(secp).map(|key| {
   	// FIXME: rust-miniscript will panic if we call
   	// xpk.master_fingerprint() on a key without origin
   	let origin = match xpk {
   	DescriptorPublicKey::XPub(..) => {
   	(xpk.master_fingerprint(), child_number)
   	}
   	_ => (bip32::Fingerprint::from(&[0, 0, 0, 0][..]), 0.into()),
   	};
   	DerivedPublicKey { key, origin }
   	})
   	})
   	.expect("All pubkeys are derived, no wildcard."),
   	)
   	}

I think we should do both.