Skip to content
This repository was archived by the owner on Oct 22, 2026. It is now read-only.

Commit 20596a5

Browse files
committed
Merge #1046: plan: fix underreported witness and scriptSig sizes for wsh and sh() wrapping
ab32aa1 plan: test sig-bearing paths byte-exactly against a real satisfaction (russeree) 46b64ef plan: test reported sizes against an actual satisfaction (russeree) 4c8e227 plan: fix scriptSig size for sh(wpkh) and sh(wsh) (russeree) d72f146 plan: count the witness script in witness_size for wsh and sh(wsh) (russeree) Pull request description: Reimplements #1045, where these bugs were originally found and reported by guggero. Fixes two size-accounting bugs in `Plan` fee-estimation helpers that caused input sizes (and therefore fee estimates) to be underreported: - **Count the witness script in `witness_size` for wsh and sh(wsh).** `Plan::satisfy()` appends the witness script as the final witness element for wsh and sh(wsh) descriptors (#897), but `witness_size()` only summed the template, underestimating the witness by the whole script (varint length prefix plus script bytes). - **Fix scriptSig size for sh(wpkh) and sh(wsh).** The scriptSig of a nested-segwit input is a push of the witness program, and `scriptsig_size()` missed the outer push opcode byte: sh(wpkh) is 24 bytes on the wire, not 23, and sh(wsh) is 36 bytes, not 35. Fee estimates were 4 WU per input too low. - **Regression tests:** hold `witness_size()` and `scriptsig_size()` to the exact wire size of a real satisfaction and `satisfaction_weight()` to `Descriptor::max_weight_to_satisfy()`, for both a sigless wsh/sh(wsh) path (no signature size variance) and — using 72-byte signatures that exactly fill the 73-byte-per-signature budget documented on `types::extra_props::SatData` — signature-bearing wpkh, sh(wpkh), wsh and sh(wsh) paths. Assisted-by: Kimi kimi-latest ACKs for top commit: Abeeujah: ACK ab32aa1 apoelstra: ACK ab32aa1; successfully ran local tests Tree-SHA512: 357e7426fc540fadc13464aa10a12f5645349c231ad1eb87e49aaf1b9555188e679bcb1a78d61d51272be943ad5110211219c0b1ecce61921a1a3661e8999ad2
2 parents 699a4bd + ab32aa1 commit 20596a5

1 file changed

Lines changed: 168 additions & 17 deletions

File tree

‎src/plan.rs‎

Lines changed: 168 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use crate::descriptor::{self, Descriptor, DescriptorType, KeyMap};
2828
use crate::miniscript::hash256;
2929
use crate::miniscript::satisfy::{Placeholder, Satisfier, SchnorrSigType};
3030
use crate::prelude::*;
31-
use crate::util::witness_size;
31+
use crate::util::{varint_len, witness_size};
3232
use crate::{DefiniteDescriptorKey, DescriptorPublicKey, Error, MiniscriptKey, ToPublicKey};
3333

3434
/// Trait describing a present/missing lookup table for constructing witness templates
@@ -242,10 +242,10 @@ impl<Pk: MiniscriptKey + ToPublicKey> Plan<Pk> {
242242
(None, _) => witness_size(self.template.as_ref()),
243243
// Taproot doesn't have a "wrapped" version (scriptSig len (1))
244244
(Some(WitnessVersion::V1), _) => 1,
245-
// scriptSig len (1) + OP_0 (1) + OP_PUSHBYTES_20 (1) + <pk hash> (20)
246-
(_, DescriptorType::ShWpkh) => 1 + 1 + 1 + 20,
247-
// scriptSig len (1) + OP_0 (1) + OP_PUSHBYTES_32 (1) + <script hash> (32)
248-
(_, DescriptorType::ShWsh) => 1 + 1 + 1 + 32,
245+
// scriptSig len (1) + OP_PUSHBYTES_22 (1) + OP_0 (1) + OP_PUSHBYTES_20 (1) + <pk hash> (20)
246+
(_, DescriptorType::ShWpkh) => 1 + 1 + 1 + 1 + 20,
247+
// scriptSig len (1) + OP_PUSHBYTES_34 (1) + OP_0 (1) + OP_PUSHBYTES_32 (1) + <script hash> (32)
248+
(_, DescriptorType::ShWsh) => 1 + 1 + 1 + 1 + 32,
249249
// Native Segwit v0 (scriptSig len (1))
250250
_ => 1,
251251
}
@@ -257,7 +257,20 @@ impl<Pk: MiniscriptKey + ToPublicKey> Plan<Pk> {
257257
/// if there's at least one segwit input in the tx. See ["Empty script witnesses are encoded as a zero byte"](https://github.com/bitcoin/bips/blob/d8a56c9f2b521bf4af5d588f217e7618cc44952c/bip-0144.mediawiki#serialization).
258258
pub fn witness_size(&self) -> usize {
259259
if self.descriptor.desc_type().segwit_version().is_some() {
260-
witness_size(self.template.as_ref())
260+
let template_size = witness_size(self.template.as_ref());
261+
match self.descriptor.desc_type() {
262+
DescriptorType::Wsh | DescriptorType::ShWsh => {
263+
let script_len = self
264+
.descriptor
265+
.explicit_script()
266+
.expect("wsh descriptors have explicit script")
267+
.len();
268+
template_size + varint_len(script_len) + script_len
269+
- varint_len(self.template.len())
270+
+ varint_len(self.template.len() + 1)
271+
}
272+
_ => template_size,
273+
}
261274
} else {
262275
0
263276
}
@@ -792,10 +805,11 @@ mod test {
792805
let desc = format!("wsh(t:or_c(pk({}),v:pkh({})))", keys[0], keys[1]);
793806

794807
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig)
808+
// + 1 (witness script len) + 63 (witness script)
795809
let tests = vec![
796810
(vec![], vec![], None, None, None),
797-
(vec![0], vec![], None, None, Some(4 + 1 + 73)),
798-
(vec![0, 1], vec![], None, None, Some(4 + 1 + 73)),
811+
(vec![0], vec![], None, None, Some(4 + 1 + 73 + 1 + 63)),
812+
(vec![0, 1], vec![], None, None, Some(4 + 1 + 73 + 1 + 63)),
799813
];
800814

801815
test_inner(&desc, keys, hashes, tests);
@@ -817,10 +831,11 @@ mod test {
817831
let desc = format!("wsh(and_v(v:pk({}),pk({})))", keys[0], keys[1]);
818832

819833
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 2
834+
// + 1 (witness script len) + 70 (witness script)
820835
let tests = vec![
821836
(vec![], vec![], None, None, None),
822837
(vec![0], vec![], None, None, None),
823-
(vec![0, 1], vec![], None, None, Some(4 + 1 + 73 * 2)),
838+
(vec![0, 1], vec![], None, None, Some(4 + 1 + 73 * 2 + 1 + 70)),
824839
];
825840

826841
test_inner(&desc, keys, hashes, tests);
@@ -853,7 +868,8 @@ mod test {
853868
(vec![], vec![], None, None, None),
854869
(vec![0, 1], vec![], None, None, None),
855870
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 3 + 1 (dummy push)
856-
(vec![0, 1, 3], vec![], None, None, Some(4 + 1 + 73 * 3 + 1)),
871+
// + 1 (witness script len) + 139 (witness script)
872+
(vec![0, 1, 3], vec![], None, None, Some(4 + 1 + 73 * 3 + 1 + 1 + 139)),
857873
];
858874

859875
test_inner(&desc, keys, hashes, tests);
@@ -887,24 +903,28 @@ mod test {
887903
),
888904
(vec![0], vec![], None, None, None),
889905
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) + 1 (OP_0) + 1 (OP_ZERO)
906+
// + 1 (witness script len) + 85 (witness script)
890907
(
891908
vec![0],
892909
vec![],
893910
Some(Sequence(1000).to_relative_lock_time().unwrap()),
894911
None,
895-
Some(80),
912+
Some(166),
896913
),
897914
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 2 + 2 (OP_PUSHBYTE_1 0x01)
898-
(vec![0, 1], vec![], None, None, Some(153)),
915+
// + 1 (witness script len) + 85 (witness script)
916+
(vec![0, 1], vec![], None, None, Some(239)),
899917
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) + 1 (OP_0) + 1 (OP_ZERO)
918+
// + 1 (witness script len) + 85 (witness script)
900919
(
901920
vec![0, 1],
902921
vec![],
903922
Some(Sequence(1000).to_relative_lock_time().unwrap()),
904923
None,
905-
Some(80),
924+
Some(166),
906925
),
907926
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 2 + 2 (OP_PUSHBYTE_1 0x01)
927+
// + 1 (witness script len) + 85 (witness script)
908928
(
909929
vec![0, 1],
910930
vec![],
@@ -914,7 +934,7 @@ mod test {
914934
.unwrap(),
915935
),
916936
None,
917-
Some(153),
937+
Some(239),
918938
), // incompatible timelock
919939
];
920940

@@ -924,20 +944,22 @@ mod test {
924944

925945
let tests = vec![
926946
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) + 1 (OP_0) + 1 (OP_ZERO)
947+
// + 1 (witness script len) + 85 (witness script)
927948
(
928949
vec![0],
929950
vec![],
930951
None,
931952
Some(absolute::LockTime::from_height(1000).unwrap()),
932-
Some(80),
953+
Some(166),
933954
),
934955
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) * 2 + 2 (OP_PUSHBYTE_1 0x01)
956+
// + 1 (witness script len) + 85 (witness script)
935957
(
936958
vec![0, 1],
937959
vec![],
938960
None,
939961
Some(absolute::LockTime::from_time(500_001_000).unwrap()),
940-
Some(153),
962+
Some(239),
941963
), // incompatible timelock
942964
];
943965

@@ -1081,7 +1103,8 @@ mod test {
10811103
(vec![], vec![0], None, None, None),
10821104
// Key + hash
10831105
// expected weight: 4 (scriptSig len) + 1 (witness len) + 73 (sig) + 1 (OP_PUSH) + 32 (preimage)
1084-
(vec![0], vec![0], None, None, Some(111)),
1106+
// + 1 (witness script len) + 62 (witness script)
1107+
(vec![0], vec![0], None, None, Some(174)),
10851108
];
10861109

10871110
test_inner(&desc, keys, hashes, tests);
@@ -1260,4 +1283,132 @@ mod test {
12601283

12611284
assert!(desc.into_plan(&assets).is_err());
12621285
}
1286+
1287+
/// A [`Satisfier`] that knows hash preimages only.
1288+
///
1289+
/// A plan sizes a signature at its 73-byte maximum while a real one varies
1290+
/// in length, so a spend path that carries no signature is the one whose
1291+
/// witness has a size a plan can be held to byte for byte.
1292+
struct Preimages(BTreeMap<sha256::Hash, [u8; 32]>);
1293+
1294+
impl Satisfier<DefiniteDescriptorKey> for Preimages {
1295+
fn lookup_sha256(&self, h: &sha256::Hash) -> Option<[u8; 32]> { self.0.get(h).copied() }
1296+
}
1297+
1298+
#[test]
1299+
fn sizes_match_satisfaction() {
1300+
use bitcoin::hashes::Hash as _;
1301+
1302+
let preimages = [[1u8; 32], [2u8; 32]];
1303+
let hashes = preimages.map(|p| sha256::Hash::hash(&p));
1304+
let satisfier = Preimages(hashes.iter().copied().zip(preimages).collect());
1305+
let assets = Assets::new().add(hashes[0]).add(hashes[1]);
1306+
1307+
// Sigless, so it has to go around the sanity checks of `from_str`.
1308+
let ms = Miniscript::<DefiniteDescriptorKey, Segwitv0>::from_str_insane(&format!(
1309+
"and_v(v:sha256({}),sha256({}))",
1310+
hashes[0], hashes[1]
1311+
))
1312+
.unwrap();
1313+
1314+
for desc in [
1315+
Descriptor::new_wsh(ms.clone()).unwrap(),
1316+
Descriptor::new_sh_wsh(ms).unwrap(),
1317+
] {
1318+
let max_weight = desc.max_weight_to_satisfy().unwrap().to_wu() as usize;
1319+
let plan = desc.into_plan(&assets).unwrap();
1320+
let (witness, script_sig) = plan.satisfy(&satisfier).unwrap();
1321+
1322+
// The witness as it goes on the wire: an element count followed by
1323+
// length-prefixed elements, the witness script among them.
1324+
let witness = bitcoin::Witness::from_slice(&witness);
1325+
assert_eq!(plan.witness_size(), bitcoin::consensus::serialize(&witness).len());
1326+
assert_eq!(plan.scriptsig_size(), varint_len(script_sig.len()) + script_sig.len());
1327+
1328+
// `max_weight_to_satisfy` is the weight that satisfying adds to an
1329+
// input, so it leaves out the scriptSig length byte (4 WU) and the
1330+
// witness count byte (1 WU) that an unsatisfied input carries too.
1331+
// The plan reports the absolute size, so it is larger by exactly those.
1332+
assert_eq!(plan.satisfaction_weight(), max_weight + 4 + 1);
1333+
}
1334+
1335+
// sh(wpkh) needs a signature, whose real size varies, so only what a plan
1336+
// can be held to exactly is checked: the scriptSig it returns, and the
1337+
// relation to `max_weight_to_satisfy`, which sizes a signature the way the
1338+
// plan does.
1339+
let key = DescriptorPublicKey::from_str(
1340+
"02c2fd50ceae468857bb7eb32ae9cd4083e6c7e42fbbec179d81134b3e3830586c",
1341+
)
1342+
.unwrap();
1343+
let desc =
1344+
Descriptor::<DefiniteDescriptorKey>::from_str(&format!("sh(wpkh({}))", key)).unwrap();
1345+
let max_weight = desc.max_weight_to_satisfy().unwrap().to_wu() as usize;
1346+
let script_sig = desc.unsigned_script_sig();
1347+
let plan = desc.into_plan(&Assets::new().add(key)).unwrap();
1348+
assert_eq!(plan.scriptsig_size(), varint_len(script_sig.len()) + script_sig.len());
1349+
assert_eq!(plan.satisfaction_weight(), max_weight + 4 + 1);
1350+
}
1351+
1352+
/// A plan sizes every ECDSA signature at its 73-byte maximum (including the
1353+
/// length prefix, per the convention in `types::extra_props::SatData`), so a
1354+
/// satisfier that hands out signatures exactly filling that budget makes
1355+
/// even signature-bearing paths exact to the byte.
1356+
#[test]
1357+
fn sizes_match_max_sig_satisfaction() {
1358+
// A 72-byte signature: r with the high bit set and s without, so r is a
1359+
// 33-byte DER integer and s a 32-byte one (71 bytes of DER) plus the
1360+
// sighash flag. With its length prefix the witness element is exactly
1361+
// the 73 bytes a plan budgets.
1362+
let mut raw = [0x01u8; 64];
1363+
raw[0] = 0x80;
1364+
let sig = bitcoin::ecdsa::Signature::sighash_all(
1365+
secp256k1::ecdsa::Signature::from_compact(&raw).unwrap(),
1366+
);
1367+
assert_eq!(sig.to_vec().len(), 72);
1368+
1369+
let keys = [
1370+
DescriptorPublicKey::from_str(
1371+
"02c2fd50ceae468857bb7eb32ae9cd4083e6c7e42fbbec179d81134b3e3830586c",
1372+
)
1373+
.unwrap(),
1374+
DescriptorPublicKey::from_str(
1375+
"0257f4a2816338436cccabc43aa724cf6e69e43e84c3c8a305212761389dd73a8a",
1376+
)
1377+
.unwrap(),
1378+
];
1379+
let def_keys = keys
1380+
.iter()
1381+
.map(|k| DefiniteDescriptorKey::new(k.clone()).unwrap())
1382+
.collect::<Vec<_>>();
1383+
1384+
let sigs = def_keys
1385+
.iter()
1386+
.map(|k| (k.clone(), sig))
1387+
.collect::<BTreeMap<_, _>>();
1388+
let pkh_sigs = def_keys
1389+
.iter()
1390+
.map(|k| (k.to_pubkeyhash(SigType::Ecdsa), (k.clone(), sig)))
1391+
.collect::<BTreeMap<_, _>>();
1392+
let satisfier = (sigs, pkh_sigs);
1393+
let assets = Assets::new().add(keys[0].clone()).add(keys[1].clone());
1394+
1395+
for desc in [
1396+
format!("wpkh({})", keys[0]),
1397+
format!("sh(wpkh({}))", keys[0]),
1398+
format!("wsh(and_v(v:pk({}),pk({})))", keys[0], keys[1]),
1399+
format!("sh(wsh(and_v(v:pk({}),pk({}))))", keys[0], keys[1]),
1400+
] {
1401+
let desc = Descriptor::<DefiniteDescriptorKey>::from_str(&desc).unwrap();
1402+
let max_weight = desc.max_weight_to_satisfy().unwrap().to_wu() as usize;
1403+
let plan = desc.clone().into_plan(&assets).unwrap();
1404+
let (witness, script_sig) = plan.satisfy(&satisfier).unwrap();
1405+
1406+
// Same wire-exact checks as `sizes_match_satisfaction`, now on paths
1407+
// that carry signatures.
1408+
let witness = bitcoin::Witness::from_slice(&witness);
1409+
assert_eq!(plan.witness_size(), bitcoin::consensus::serialize(&witness).len());
1410+
assert_eq!(plan.scriptsig_size(), varint_len(script_sig.len()) + script_sig.len());
1411+
assert_eq!(plan.satisfaction_weight(), max_weight + 4 + 1);
1412+
}
1413+
}
12631414
}

0 commit comments

Comments
 (0)