Skip to content

Commit a4fd565

Browse files
glozowglozow
committed
Merge bitcoin/bitcoin#31727: miniscript: convert non-critical asserts to CHECK_NONFATAL
ff0194a miniscript: convert non-critical asserts to CHECK_NONFATAL (Antoine Poinsot) Pull request description: The Miniscript code contains assertions to prevent ending up in an insane state or prevent UB, but also to enforce logical invariants. For the latter it is not necessary to crash the program if they are broken. Raising an exception suffices, especially as this code is often called through the RPC interface which can in turn handle the exception and the user can report it to developers. This revives #28678 from Pieter Wuille. ACKs for top commit: hodlinator: ACK ff0194a TheCharlatan: ACK ff0194a brunoerg: code review ACK ff0194a Tree-SHA512: 8ed8f7b494e46ecf7cdebe75120cd0ffe543b6bc289bf882dac631fe2ec2cae590d5f7bc2316e52db085791694b136dffbc71c40c1e16886fa53ab00bd8cabd0
2 parents e364e6b + ff0194a commit a4fd565

File tree

2 files changed

+54
-51
lines changed

2 files changed

+54
-51
lines changed

‎src/script/miniscript.cpp

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,67 +19,67 @@ namespace internal {
1919
Type SanitizeType(Type e) {
2020
int num_types = (e << "K"_mst) + (e << "V"_mst) + (e << "B"_mst) + (e << "W"_mst);
2121
if (num_types == 0) return ""_mst; // No valid type, don't care about the rest
22-
assert(num_types == 1); // K, V, B, W all conflict with each other
23-
assert(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o
24-
assert(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z
25-
assert(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W
26-
assert(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d
27-
assert(!(e << "K"_mst) || (e << "u"_mst)); // K implies u
28-
assert(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u
29-
assert(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f
30-
assert(!(e << "e"_mst) || (e << "d"_mst)); // e implies d
31-
assert(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e
32-
assert(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f
33-
assert(!(e << "V"_mst) || (e << "f"_mst)); // V implies f
34-
assert(!(e << "K"_mst) || (e << "s"_mst)); // K implies s
35-
assert(!(e << "z"_mst) || (e << "m"_mst)); // z implies m
22+
CHECK_NONFATAL(num_types == 1); // K, V, B, W all conflict with each other
23+
CHECK_NONFATAL(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o
24+
CHECK_NONFATAL(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z
25+
CHECK_NONFATAL(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W
26+
CHECK_NONFATAL(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d
27+
CHECK_NONFATAL(!(e << "K"_mst) || (e << "u"_mst)); // K implies u
28+
CHECK_NONFATAL(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u
29+
CHECK_NONFATAL(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f
30+
CHECK_NONFATAL(!(e << "e"_mst) || (e << "d"_mst)); // e implies d
31+
CHECK_NONFATAL(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e
32+
CHECK_NONFATAL(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f
33+
CHECK_NONFATAL(!(e << "V"_mst) || (e << "f"_mst)); // V implies f
34+
CHECK_NONFATAL(!(e << "K"_mst) || (e << "s"_mst)); // K implies s
35+
CHECK_NONFATAL(!(e << "z"_mst) || (e << "m"_mst)); // z implies m
3636
return e;
3737
}
3838

3939
Type ComputeType(Fragment fragment, Type x, Type y, Type z, const std::vector<Type>& sub_types, uint32_t k,
4040
size_t data_size, size_t n_subs, size_t n_keys, MiniscriptContext ms_ctx) {
4141
// Sanity check on data
4242
if (fragment == Fragment::SHA256 || fragment == Fragment::HASH256) {
43-
assert(data_size == 32);
43+
CHECK_NONFATAL(data_size == 32);
4444
} else if (fragment == Fragment::RIPEMD160 || fragment == Fragment::HASH160) {
45-
assert(data_size == 20);
45+
CHECK_NONFATAL(data_size == 20);
4646
} else {
47-
assert(data_size == 0);
47+
CHECK_NONFATAL(data_size == 0);
4848
}
4949
// Sanity check on k
5050
if (fragment == Fragment::OLDER || fragment == Fragment::AFTER) {
51-
assert(k >= 1 && k < 0x80000000UL);
51+
CHECK_NONFATAL(k >= 1 && k < 0x80000000UL);
5252
} else if (fragment == Fragment::MULTI || fragment == Fragment::MULTI_A) {
53-
assert(k >= 1 && k <= n_keys);
53+
CHECK_NONFATAL(k >= 1 && k <= n_keys);
5454
} else if (fragment == Fragment::THRESH) {
55-
assert(k >= 1 && k <= n_subs);
55+
CHECK_NONFATAL(k >= 1 && k <= n_subs);
5656
} else {
57-
assert(k == 0);
57+
CHECK_NONFATAL(k == 0);
5858
}
5959
// Sanity check on subs
6060
if (fragment == Fragment::AND_V || fragment == Fragment::AND_B || fragment == Fragment::OR_B ||
6161
fragment == Fragment::OR_C || fragment == Fragment::OR_I || fragment == Fragment::OR_D) {
62-
assert(n_subs == 2);
62+
CHECK_NONFATAL(n_subs == 2);
6363
} else if (fragment == Fragment::ANDOR) {
64-
assert(n_subs == 3);
64+
CHECK_NONFATAL(n_subs == 3);
6565
} else if (fragment == Fragment::WRAP_A || fragment == Fragment::WRAP_S || fragment == Fragment::WRAP_C ||
6666
fragment == Fragment::WRAP_D || fragment == Fragment::WRAP_V || fragment == Fragment::WRAP_J ||
6767
fragment == Fragment::WRAP_N) {
68-
assert(n_subs == 1);
68+
CHECK_NONFATAL(n_subs == 1);
6969
} else if (fragment != Fragment::THRESH) {
70-
assert(n_subs == 0);
70+
CHECK_NONFATAL(n_subs == 0);
7171
}
7272
// Sanity check on keys
7373
if (fragment == Fragment::PK_K || fragment == Fragment::PK_H) {
74-
assert(n_keys == 1);
74+
CHECK_NONFATAL(n_keys == 1);
7575
} else if (fragment == Fragment::MULTI) {
76-
assert(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTISIG);
77-
assert(!IsTapscript(ms_ctx));
76+
CHECK_NONFATAL(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTISIG);
77+
CHECK_NONFATAL(!IsTapscript(ms_ctx));
7878
} else if (fragment == Fragment::MULTI_A) {
79-
assert(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTI_A);
80-
assert(IsTapscript(ms_ctx));
79+
CHECK_NONFATAL(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTI_A);
80+
CHECK_NONFATAL(IsTapscript(ms_ctx));
8181
} else {
82-
assert(n_keys == 0);
82+
CHECK_NONFATAL(n_keys == 0);
8383
}
8484

8585
// Below is the per-fragment logic for computing the expression types.

‎src/script/miniscript.h

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,8 @@ struct Node {
659659
stack.pop_back();
660660
}
661661
// The final remaining results element is the root result, return it.
662-
assert(results.size() == 1);
662+
assert(results.size() >= 1);
663+
CHECK_NONFATAL(results.size() == 1);
663664
return std::move(results[0]);
664665
}
665666

@@ -1225,7 +1226,7 @@ struct Node {
12251226
// The dissatisfaction consists of as many empty vectors as there are keys, which is the same as
12261227
// satisfying 0 keys.
12271228
auto& nsat{sats[0]};
1228-
assert(node.k != 0);
1229+
CHECK_NONFATAL(node.k != 0);
12291230
assert(node.k <= sats.size());
12301231
return {std::move(nsat), std::move(sats[node.k])};
12311232
}
@@ -1392,38 +1393,38 @@ struct Node {
13921393
// (the actual satisfaction code in ProduceInputHelper does not use GetType)
13931394

13941395
// For 'z' nodes, available satisfactions/dissatisfactions must have stack size 0.
1395-
if (node.GetType() << "z"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 0);
1396-
if (node.GetType() << "z"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 0);
1396+
if (node.GetType() << "z"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() == 0);
1397+
if (node.GetType() << "z"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() == 0);
13971398

13981399
// For 'o' nodes, available satisfactions/dissatisfactions must have stack size 1.
1399-
if (node.GetType() << "o"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 1);
1400-
if (node.GetType() << "o"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 1);
1400+
if (node.GetType() << "o"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() == 1);
1401+
if (node.GetType() << "o"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() == 1);
14011402

14021403
// For 'n' nodes, available satisfactions/dissatisfactions must have stack size 1 or larger. For satisfactions,
14031404
// the top element cannot be 0.
1404-
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() >= 1);
1405-
if (node.GetType() << "n"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() >= 1);
1406-
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.stack.back().empty());
1405+
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() >= 1);
1406+
if (node.GetType() << "n"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() >= 1);
1407+
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(!ret.sat.stack.back().empty());
14071408

14081409
// For 'd' nodes, a dissatisfaction must exist, and they must not need a signature. If it is non-malleable,
14091410
// it must be canonical.
1410-
if (node.GetType() << "d"_mst) assert(ret.nsat.available != Availability::NO);
1411-
if (node.GetType() << "d"_mst) assert(!ret.nsat.has_sig);
1412-
if (node.GetType() << "d"_mst && !ret.nsat.malleable) assert(!ret.nsat.non_canon);
1411+
if (node.GetType() << "d"_mst) CHECK_NONFATAL(ret.nsat.available != Availability::NO);
1412+
if (node.GetType() << "d"_mst) CHECK_NONFATAL(!ret.nsat.has_sig);
1413+
if (node.GetType() << "d"_mst && !ret.nsat.malleable) CHECK_NONFATAL(!ret.nsat.non_canon);
14131414

14141415
// For 'f'/'s' nodes, dissatisfactions/satisfactions must have a signature.
1415-
if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.has_sig);
1416-
if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) assert(ret.sat.has_sig);
1416+
if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.has_sig);
1417+
if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.has_sig);
14171418

14181419
// For non-malleable 'e' nodes, a non-malleable dissatisfaction must exist.
1419-
if (node.GetType() << "me"_mst) assert(ret.nsat.available != Availability::NO);
1420-
if (node.GetType() << "me"_mst) assert(!ret.nsat.malleable);
1420+
if (node.GetType() << "me"_mst) CHECK_NONFATAL(ret.nsat.available != Availability::NO);
1421+
if (node.GetType() << "me"_mst) CHECK_NONFATAL(!ret.nsat.malleable);
14211422

14221423
// For 'm' nodes, if a satisfaction exists, it must be non-malleable.
1423-
if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.malleable);
1424+
if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(!ret.sat.malleable);
14241425

14251426
// If a non-malleable satisfaction exists, it must be canonical.
1426-
if (ret.sat.available != Availability::NO && !ret.sat.malleable) assert(!ret.sat.non_canon);
1427+
if (ret.sat.available != Availability::NO && !ret.sat.malleable) CHECK_NONFATAL(!ret.sat.non_canon);
14271428

14281429
return ret;
14291430
};
@@ -1604,7 +1605,8 @@ struct Node {
16041605
case Fragment::THRESH:
16051606
return static_cast<uint32_t>(std::count(subs.begin(), subs.end(), true)) >= node.k;
16061607
default: // wrappers
1607-
assert(subs.size() == 1);
1608+
assert(subs.size() >= 1);
1609+
CHECK_NONFATAL(subs.size() == 1);
16081610
return subs[0];
16091611
}
16101612
});
@@ -2157,7 +2159,8 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
21572159
}
21582160

21592161
// Sanity checks on the produced miniscript
2160-
assert(constructed.size() == 1);
2162+
assert(constructed.size() >= 1);
2163+
CHECK_NONFATAL(constructed.size() == 1);
21612164
assert(constructed[0]->ScriptSize() == script_size);
21622165
if (in.size() > 0) return {};
21632166
NodeRef<Key> tl_node = std::move(constructed.front());

0 commit comments

Comments
 (0)