highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-04-10 23:58:17 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34499: miniscript: Use valid script in test, etc (#31713 follow-ups)

Browse Source 
39e3295c71 test(miniscript): Check for depth rather than script size (Hodlinator)
5af5e87646 test(miniscript): Make tested script valid (Hodlinator)
fd7c494c6b doc(miniscript): Explain why we operate on vectors (Hodlinator)
da51b5e4d2 refactor(miniscript): Move keys to avoid copy (Hodlinator)

Pull request description:

  * Add missing move https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2722771821
  * Document reason behind moving entire `vector`s https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2722932391
  * Make miniscript valid and improve test name https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2722121348
  * Check for miniscript node depth rather than script size https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2722062298

ACKs for top commit:
  l0rinc:
    ACK 39e3295c71
  sedited:
    ACK 39e3295c71
  darosior:
    Github light ACK 39e3295c71. Code looks correct to me. I don't understand why i'm co-author of the last commit, since i did not author any code in there. 🤷

Tree-SHA512: 88c240183d7ebe93e3a1d3d65969b435775190f15d2845b58dbd16938553bb6490ab57400544f90a5a3f9a73245dce769ffc4868ae6fb7513f7db46743bfb9e1
This commit is contained in:
merge-script
2026-03-19 18:48:24 +01:00
parent 3c9e421942 39e3295c71
commit c53021b0ec
2 changed files with 25 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
src/script/miniscript.h
View File

@@ -552,6 +552,9 @@ public:
// Destroy the subexpressions iteratively after moving out their
// subexpressions to avoid a stack-overflow due to recursive calls to
// the subs' destructors.
// We move vectors in order to only update array-pointers inside them
// rather than moving individual Node instances which would involve
// moving/copying each Node field.
std::vector<std::vector<Node>> queue;
queue.push_back(std::move(subs));
do {
@@ -607,7 +610,7 @@ private:
// This is kept private as no valid fragment has all of these arguments.
// Only used by Clone()
Node(internal::NoDupCheck, MiniscriptContext script_ctx, enum Fragment nt, std::vector<Node> sub, std::vector<Key> key, std::vector<unsigned char> arg, uint32_t val)
: fragment(nt), k(val), keys(key), data(std::move(arg)), subs(std::move(sub)), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {}
: fragment(nt), k(val), keys(std::move(key)), data(std::move(arg)), subs(std::move(sub)), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {}
//! Compute the length of the script for this miniscript (including children).
size_t CalcScriptLen() const

27
src/test/miniscript_tests.cpp
View File

@@ -727,24 +727,39 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
}
// Confirm that ~Node(), Node::Clone() and operator=(Node&&) are stack-safe.
BOOST_AUTO_TEST_CASE(node_deep_destruct)
BOOST_AUTO_TEST_CASE(node_stress_stack)
{
using miniscript::internal::NoDupCheck;
using miniscript::Fragment;
using NodeU32 = miniscript::Node<uint32_t>;
constexpr auto ctx{miniscript::MiniscriptContext::P2WSH};
const auto compute_depth{[] (const NodeU32& node) -> size_t {
size_t depth{0};
for (const auto* n{&node}; !n->Subs().empty(); n = &n->Subs().front()) {
++depth;
}
return depth;
}};
constexpr auto ctx{miniscript::MiniscriptContext::TAPSCRIPT};
NodeU32 root{NoDupCheck{}, ctx, Fragment::JUST_1};
for (uint32_t i{0}; i < 200'000; ++i) {
root = NodeU32{NoDupCheck{}, ctx, Fragment::WRAP_S, Vector(std::move(root))};
// Some CI jobs run with CI_LIMIT_STACK_SIZE which reduces the stack size
// via ulimit to 512 kbytes. When tested with ~Node()=default (stack-unsafe)
// implementations the test has been shown to fail for the below depth.
// The test may pass locally despite stack-unsafe implementations unless the
// stack is reduced in a similar way or the depth is temporarily increased.
constexpr size_t depth{200'000};
for (size_t i{0}; i < depth; ++i) {
root = NodeU32{NoDupCheck{}, ctx, Fragment::WRAP_N, Vector(std::move(root))};
}
BOOST_CHECK_EQUAL(root.ScriptSize(), 200'001);
BOOST_CHECK(root.IsValid());
BOOST_CHECK_EQUAL(compute_depth(root), depth);
auto clone{root.Clone()};
BOOST_CHECK_EQUAL(clone.ScriptSize(), root.ScriptSize());
BOOST_CHECK_EQUAL(compute_depth(clone), depth);
clone = std::move(root);
BOOST_CHECK_EQUAL(compute_depth(clone), depth);
}
BOOST_AUTO_TEST_SUITE_END()