From ff0194a7ce9dabf1b31b64ca584e45840dce8141 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 23 Jan 2025 10:48:23 -0500 Subject: [PATCH] miniscript: convert non-critical asserts to CHECK_NONFATAL 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 is based on previous work from Pieter Wuille. --- src/script/miniscript.cpp | 62 +++++++++++++++++++-------------------- src/script/miniscript.h | 43 ++++++++++++++------------- 2 files changed, 54 insertions(+), 51 deletions(-) diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp index 4b8d3673f95..fa72de63b31 100644 --- a/src/script/miniscript.cpp +++ b/src/script/miniscript.cpp @@ -19,20 +19,20 @@ namespace internal { Type SanitizeType(Type e) { int num_types = (e << "K"_mst) + (e << "V"_mst) + (e << "B"_mst) + (e << "W"_mst); if (num_types == 0) return ""_mst; // No valid type, don't care about the rest - assert(num_types == 1); // K, V, B, W all conflict with each other - assert(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o - assert(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z - assert(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W - assert(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d - assert(!(e << "K"_mst) || (e << "u"_mst)); // K implies u - assert(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u - assert(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f - assert(!(e << "e"_mst) || (e << "d"_mst)); // e implies d - assert(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e - assert(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f - assert(!(e << "V"_mst) || (e << "f"_mst)); // V implies f - assert(!(e << "K"_mst) || (e << "s"_mst)); // K implies s - assert(!(e << "z"_mst) || (e << "m"_mst)); // z implies m + CHECK_NONFATAL(num_types == 1); // K, V, B, W all conflict with each other + CHECK_NONFATAL(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o + CHECK_NONFATAL(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z + CHECK_NONFATAL(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W + CHECK_NONFATAL(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d + CHECK_NONFATAL(!(e << "K"_mst) || (e << "u"_mst)); // K implies u + CHECK_NONFATAL(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u + CHECK_NONFATAL(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f + CHECK_NONFATAL(!(e << "e"_mst) || (e << "d"_mst)); // e implies d + CHECK_NONFATAL(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e + CHECK_NONFATAL(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f + CHECK_NONFATAL(!(e << "V"_mst) || (e << "f"_mst)); // V implies f + CHECK_NONFATAL(!(e << "K"_mst) || (e << "s"_mst)); // K implies s + CHECK_NONFATAL(!(e << "z"_mst) || (e << "m"_mst)); // z implies m return e; } @@ -40,46 +40,46 @@ Type ComputeType(Fragment fragment, Type x, Type y, Type z, const std::vector= 1 && k < 0x80000000UL); + CHECK_NONFATAL(k >= 1 && k < 0x80000000UL); } else if (fragment == Fragment::MULTI || fragment == Fragment::MULTI_A) { - assert(k >= 1 && k <= n_keys); + CHECK_NONFATAL(k >= 1 && k <= n_keys); } else if (fragment == Fragment::THRESH) { - assert(k >= 1 && k <= n_subs); + CHECK_NONFATAL(k >= 1 && k <= n_subs); } else { - assert(k == 0); + CHECK_NONFATAL(k == 0); } // Sanity check on subs if (fragment == Fragment::AND_V || fragment == Fragment::AND_B || fragment == Fragment::OR_B || fragment == Fragment::OR_C || fragment == Fragment::OR_I || fragment == Fragment::OR_D) { - assert(n_subs == 2); + CHECK_NONFATAL(n_subs == 2); } else if (fragment == Fragment::ANDOR) { - assert(n_subs == 3); + CHECK_NONFATAL(n_subs == 3); } else if (fragment == Fragment::WRAP_A || fragment == Fragment::WRAP_S || fragment == Fragment::WRAP_C || fragment == Fragment::WRAP_D || fragment == Fragment::WRAP_V || fragment == Fragment::WRAP_J || fragment == Fragment::WRAP_N) { - assert(n_subs == 1); + CHECK_NONFATAL(n_subs == 1); } else if (fragment != Fragment::THRESH) { - assert(n_subs == 0); + CHECK_NONFATAL(n_subs == 0); } // Sanity check on keys if (fragment == Fragment::PK_K || fragment == Fragment::PK_H) { - assert(n_keys == 1); + CHECK_NONFATAL(n_keys == 1); } else if (fragment == Fragment::MULTI) { - assert(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTISIG); - assert(!IsTapscript(ms_ctx)); + CHECK_NONFATAL(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTISIG); + CHECK_NONFATAL(!IsTapscript(ms_ctx)); } else if (fragment == Fragment::MULTI_A) { - assert(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTI_A); - assert(IsTapscript(ms_ctx)); + CHECK_NONFATAL(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTI_A); + CHECK_NONFATAL(IsTapscript(ms_ctx)); } else { - assert(n_keys == 0); + CHECK_NONFATAL(n_keys == 0); } // Below is the per-fragment logic for computing the expression types. diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 03f0a7c3654..96d42869553 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -659,7 +659,8 @@ private: stack.pop_back(); } // The final remaining results element is the root result, return it. - assert(results.size() == 1); + assert(results.size() >= 1); + CHECK_NONFATAL(results.size() == 1); return std::move(results[0]); } @@ -1225,7 +1226,7 @@ private: // The dissatisfaction consists of as many empty vectors as there are keys, which is the same as // satisfying 0 keys. auto& nsat{sats[0]}; - assert(node.k != 0); + CHECK_NONFATAL(node.k != 0); assert(node.k <= sats.size()); return {std::move(nsat), std::move(sats[node.k])}; } @@ -1392,38 +1393,38 @@ private: // (the actual satisfaction code in ProduceInputHelper does not use GetType) // For 'z' nodes, available satisfactions/dissatisfactions must have stack size 0. - if (node.GetType() << "z"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 0); - if (node.GetType() << "z"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 0); + if (node.GetType() << "z"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() == 0); + if (node.GetType() << "z"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() == 0); // For 'o' nodes, available satisfactions/dissatisfactions must have stack size 1. - if (node.GetType() << "o"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 1); - if (node.GetType() << "o"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 1); + if (node.GetType() << "o"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() == 1); + if (node.GetType() << "o"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() == 1); // For 'n' nodes, available satisfactions/dissatisfactions must have stack size 1 or larger. For satisfactions, // the top element cannot be 0. - if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() >= 1); - if (node.GetType() << "n"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() >= 1); - if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.stack.back().empty()); + if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() >= 1); + if (node.GetType() << "n"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() >= 1); + if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(!ret.sat.stack.back().empty()); // For 'd' nodes, a dissatisfaction must exist, and they must not need a signature. If it is non-malleable, // it must be canonical. - if (node.GetType() << "d"_mst) assert(ret.nsat.available != Availability::NO); - if (node.GetType() << "d"_mst) assert(!ret.nsat.has_sig); - if (node.GetType() << "d"_mst && !ret.nsat.malleable) assert(!ret.nsat.non_canon); + if (node.GetType() << "d"_mst) CHECK_NONFATAL(ret.nsat.available != Availability::NO); + if (node.GetType() << "d"_mst) CHECK_NONFATAL(!ret.nsat.has_sig); + if (node.GetType() << "d"_mst && !ret.nsat.malleable) CHECK_NONFATAL(!ret.nsat.non_canon); // For 'f'/'s' nodes, dissatisfactions/satisfactions must have a signature. - if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.has_sig); - if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) assert(ret.sat.has_sig); + if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.has_sig); + if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.has_sig); // For non-malleable 'e' nodes, a non-malleable dissatisfaction must exist. - if (node.GetType() << "me"_mst) assert(ret.nsat.available != Availability::NO); - if (node.GetType() << "me"_mst) assert(!ret.nsat.malleable); + if (node.GetType() << "me"_mst) CHECK_NONFATAL(ret.nsat.available != Availability::NO); + if (node.GetType() << "me"_mst) CHECK_NONFATAL(!ret.nsat.malleable); // For 'm' nodes, if a satisfaction exists, it must be non-malleable. - if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.malleable); + if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(!ret.sat.malleable); // If a non-malleable satisfaction exists, it must be canonical. - if (ret.sat.available != Availability::NO && !ret.sat.malleable) assert(!ret.sat.non_canon); + if (ret.sat.available != Availability::NO && !ret.sat.malleable) CHECK_NONFATAL(!ret.sat.non_canon); return ret; }; @@ -1604,7 +1605,8 @@ public: case Fragment::THRESH: return static_cast(std::count(subs.begin(), subs.end(), true)) >= node.k; default: // wrappers - assert(subs.size() == 1); + assert(subs.size() >= 1); + CHECK_NONFATAL(subs.size() == 1); return subs[0]; } }); @@ -2157,7 +2159,8 @@ inline NodeRef Parse(Span in, const Ctx& ctx) } // Sanity checks on the produced miniscript - assert(constructed.size() == 1); + assert(constructed.size() >= 1); + CHECK_NONFATAL(constructed.size() == 1); assert(constructed[0]->ScriptSize() == script_size); if (in.size() > 0) return {}; NodeRef tl_node = std::move(constructed.front());