Skip to content

Address some nits following #99 and #100 #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sipa merged 6 commits into from
Feb 21, 2022
Merged

Address some nits following #99 and #100 #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a516e1a
Improvements for ParseKey / ParseHexStr
sipa Feb 18, 2022
5fc20e9
Rename s/NodeType/Fragment/ in compiler
sipa Feb 18, 2022
492fced
Simplify FindInsaneSub
sipa Feb 18, 2022
328fabd
Remove redundent ComputeScriptLen expression
sipa Feb 18, 2022
b18669a
More auto types
sipa Feb 18, 2022
827ed1b
Remove leftover types set
sipa Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bitcoin/script/miniscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ size_t ComputeScriptLen(Fragment nodetype, Type sub0typ, size_t subsize, uint32_
case Fragment::JUST_0: return 1;
case Fragment::PK_K: return 34;
case Fragment::PK_H: return 3 + 21;
case Fragment::OLDER: return 1 + BuildScript(k).size();
case Fragment::OLDER:
case Fragment::AFTER: return 1 + BuildScript(k).size();
case Fragment::HASH256:
case Fragment::SHA256: return 4 + 2 + 33;
Expand Down
105 changes: 49 additions & 56 deletions bitcoin/script/miniscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,38 +683,38 @@ struct Node {
case Fragment::HASH160: return {4, 0, {}};
case Fragment::AND_V: return {subs[0]->ops.count + subs[1]->ops.count, subs[0]->ops.sat + subs[1]->ops.sat, {}};
case Fragment::AND_B: {
const uint32_t count{1 + subs[0]->ops.count + subs[1]->ops.count};
const internal::MaxInt<uint32_t> sat{subs[0]->ops.sat + subs[1]->ops.sat};
const internal::MaxInt<uint32_t> dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
const auto count{1 + subs[0]->ops.count + subs[1]->ops.count};
const auto sat{subs[0]->ops.sat + subs[1]->ops.sat};
const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
return {count, sat, dsat};
}
case Fragment::OR_B: {
const uint32_t count{1 + subs[0]->ops.count + subs[1]->ops.count};
const internal::MaxInt<uint32_t> sat{Choose(subs[0]->ops.sat + subs[1]->ops.dsat, subs[1]->ops.sat + subs[0]->ops.dsat)};
const internal::MaxInt<uint32_t> dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
const auto count{1 + subs[0]->ops.count + subs[1]->ops.count};
const auto sat{Choose(subs[0]->ops.sat + subs[1]->ops.dsat, subs[1]->ops.sat + subs[0]->ops.dsat)};
const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
return {count, sat, dsat};
}
case Fragment::OR_D: {
const uint32_t count{3 + subs[0]->ops.count + subs[1]->ops.count};
const internal::MaxInt<uint32_t> sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat + subs[0]->ops.dsat)};
const internal::MaxInt<uint32_t> dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
const auto count{3 + subs[0]->ops.count + subs[1]->ops.count};
const auto sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat + subs[0]->ops.dsat)};
const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
return {count, sat, dsat};
}
case Fragment::OR_C: {
const uint32_t count{2 + subs[0]->ops.count + subs[1]->ops.count};
const internal::MaxInt<uint32_t> sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat + subs[0]->ops.dsat)};
const auto count{2 + subs[0]->ops.count + subs[1]->ops.count};
const auto sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat + subs[0]->ops.dsat)};
return {count, sat, {}};
}
case Fragment::OR_I: {
const uint32_t count{3 + subs[0]->ops.count + subs[1]->ops.count};
const internal::MaxInt<uint32_t> sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat)};
const internal::MaxInt<uint32_t> dsat{Choose(subs[0]->ops.dsat, subs[1]->ops.dsat)};
const auto count{3 + subs[0]->ops.count + subs[1]->ops.count};
const auto sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat)};
const auto dsat{Choose(subs[0]->ops.dsat, subs[1]->ops.dsat)};
return {count, sat, dsat};
}
case Fragment::ANDOR: {
const uint32_t count{3 + subs[0]->ops.count + subs[1]->ops.count + subs[2]->ops.count};
const internal::MaxInt<uint32_t> sat{Choose(subs[1]->ops.sat + subs[0]->ops.sat, subs[0]->ops.dsat + subs[2]->ops.sat)};
const internal::MaxInt<uint32_t> dsat{subs[0]->ops.dsat + subs[2]->ops.dsat};
const auto count{3 + subs[0]->ops.count + subs[1]->ops.count + subs[2]->ops.count};
const auto sat{Choose(subs[1]->ops.sat + subs[0]->ops.sat, subs[0]->ops.dsat + subs[2]->ops.sat)};
const auto dsat{subs[0]->ops.dsat + subs[2]->ops.dsat};
return {count, sat, dsat};
}
case Fragment::MULTI: return {1, (uint32_t)keys.size(), (uint32_t)keys.size()};
Expand Down Expand Up @@ -756,15 +756,15 @@ struct Node {
case Fragment::HASH256:
case Fragment::HASH160: return {1, {}};
case Fragment::ANDOR: {
const internal::MaxInt<uint32_t> sat{Choose(subs[0]->ss.sat + subs[1]->ss.sat, subs[0]->ss.dsat + subs[2]->ss.sat)};
const internal::MaxInt<uint32_t> dsat{subs[0]->ss.dsat + subs[2]->ss.dsat};
const auto sat{Choose(subs[0]->ss.sat + subs[1]->ss.sat, subs[0]->ss.dsat + subs[2]->ss.sat)};
const auto dsat{subs[0]->ss.dsat + subs[2]->ss.dsat};
return {sat, dsat};
}
case Fragment::AND_V: return {subs[0]->ss.sat + subs[1]->ss.sat, {}};
case Fragment::AND_B: return {subs[0]->ss.sat + subs[1]->ss.sat, subs[0]->ss.dsat + subs[1]->ss.dsat};
case Fragment::OR_B: {
const internal::MaxInt<uint32_t> sat{Choose(subs[0]->ss.dsat + subs[1]->ss.sat, subs[0]->ss.sat + subs[1]->ss.dsat)};
const internal::MaxInt<uint32_t> dsat{subs[0]->ss.dsat + subs[1]->ss.dsat};
const auto sat{Choose(subs[0]->ss.dsat + subs[1]->ss.sat, subs[0]->ss.sat + subs[1]->ss.dsat)};
const auto dsat{subs[0]->ss.dsat + subs[1]->ss.dsat};
return {sat, dsat};
}
case Fragment::OR_C: return {Choose(subs[0]->ss.sat, subs[0]->ss.dsat + subs[1]->ss.sat), {}};
Expand Down Expand Up @@ -974,20 +974,13 @@ struct Node {
//! Return the expression type.
Type GetType() const { return typ; }

//! Find the deepest insane sub. Null if there is none.
NodeRef<Key> FindInsaneSub() const {
auto downfn = [](NodeRef<Key> curr_insane, const Node& node, size_t i) {
if (!node.subs[i]->IsSane()) return node.subs[i];
return curr_insane;
};

auto upfn = [](NodeRef<Key> curr_insane, const Node& node, Span<NodeRef<Key>> subs) {
for (const auto& sub: subs) if (sub) return sub;
return curr_insane;
};

NodeRef<Key> null;
return TreeEvalMaybe<NodeRef<Key>>(null, downfn, upfn).value_or(null);
//! Find an insane subnode which has no insane children. Nullptr if there is none.
const Node* FindInsaneSub() const {
return TreeEval<const Node*>([](const Node& node, Span<const Node*> subs) -> const Node* {
for (auto& sub: subs) if (sub) return sub;
if (!node.IsSane()) return &node;
return nullptr;
});
Comment on lines +978 to +983
Copy link
Copy Markdown
Contributor

@darosior darosior Feb 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed much nicer. I successfully tested against the Bitcoin Core PR which had test vectors for it.

}

//! Check whether this node is valid at all.
Expand Down Expand Up @@ -1105,27 +1098,27 @@ int FindNextChar(Span<const char> in, const char m);

/** Parse a key string ending at the end of the fragment's text representation. */
template<typename Key, typename Ctx>
std::optional<std::pair<Key, int>> ParseKey(Span<const char> in, const Ctx& ctx)
std::optional<std::pair<Key, int>> ParseKeyEnd(Span<const char> in, const Ctx& ctx)
{
Key key;
int key_size = FindNextChar(in, ')');
if (key_size < 1) return {};
if (!ctx.FromString(in.begin(), in.begin() + key_size, key)) return {};
return {{key, key_size}};
return {{std::move(key), key_size}};
}

/** Parse a hex string ending at the end of the fragment's text representation. */
template<typename Ctx>
std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStr(Span<const char> in, const size_t expected_size,
const Ctx& ctx)
std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStrEnd(Span<const char> in, const size_t expected_size,
const Ctx& ctx)
{
int hash_size = FindNextChar(in, ')');
if (hash_size < 1) return {};
std::string val = std::string(in.begin(), in.begin() + hash_size);
if (!IsHex(val)) return {};
auto hash = ParseHex(val);
if (hash.size() != expected_size) return {};
return {{hash, hash_size}};
return {{std::move(hash), hash_size}};
}

/** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
Expand Down Expand Up @@ -1210,51 +1203,51 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
} else if (Const("1", in)) {
constructed.push_back(MakeNodeRef<Key>(Fragment::JUST_1));
} else if (Const("pk(", in)) {
auto res = ParseKey<Key, Ctx>(in, ctx);
auto res = ParseKeyEnd<Key, Ctx>(in, ctx);
if (!res) return {};
auto [key, key_size] = *res;
auto& [key, key_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::WRAP_C, Vector(MakeNodeRef<Key>(Fragment::PK_K, Vector(std::move(key))))));
in = in.subspan(key_size + 1);
} else if (Const("pkh(", in)) {
auto res = ParseKey<Key>(in, ctx);
auto res = ParseKeyEnd<Key>(in, ctx);
if (!res) return {};
auto [key, key_size] = *res;
auto& [key, key_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::WRAP_C, Vector(MakeNodeRef<Key>(Fragment::PK_H, Vector(std::move(key))))));
in = in.subspan(key_size + 1);
} else if (Const("pk_k(", in)) {
auto res = ParseKey<Key>(in, ctx);
auto res = ParseKeyEnd<Key>(in, ctx);
if (!res) return {};
auto [key, key_size] = *res;
auto& [key, key_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_K, Vector(std::move(key))));
in = in.subspan(key_size + 1);
} else if (Const("pk_h(", in)) {
auto res = ParseKey<Key>(in, ctx);
auto res = ParseKeyEnd<Key>(in, ctx);
if (!res) return {};
auto [key, key_size] = *res;
auto& [key, key_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_H, Vector(std::move(key))));
in = in.subspan(key_size + 1);
} else if (Const("sha256(", in)) {
auto res = ParseHexStr(in, 32, ctx);
auto res = ParseHexStrEnd(in, 32, ctx);
if (!res) return {};
auto [hash, hash_size] = *res;
auto& [hash, hash_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::SHA256, std::move(hash)));
in = in.subspan(hash_size + 1);
} else if (Const("ripemd160(", in)) {
auto res = ParseHexStr(in, 20, ctx);
auto res = ParseHexStrEnd(in, 20, ctx);
if (!res) return {};
auto [hash, hash_size] = *res;
auto& [hash, hash_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::RIPEMD160, std::move(hash)));
in = in.subspan(hash_size + 1);
} else if (Const("hash256(", in)) {
auto res = ParseHexStr(in, 32, ctx);
auto res = ParseHexStrEnd(in, 32, ctx);
if (!res) return {};
auto [hash, hash_size] = *res;
auto& [hash, hash_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::HASH256, std::move(hash)));
in = in.subspan(hash_size + 1);
} else if (Const("hash160(", in)) {
auto res = ParseHexStr(in, 20, ctx);
auto res = ParseHexStrEnd(in, 20, ctx);
if (!res) return {};
auto [hash, hash_size] = *res;
auto& [hash, hash_size] = *res;
constructed.push_back(MakeNodeRef<Key>(Fragment::HASH160, std::move(hash)));
in = in.subspan(hash_size + 1);
} else if (Const("after(", in)) {
Expand Down
2 changes: 0 additions & 2 deletions bitcoin/test/fuzz/miniscript_random.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,6 @@ struct NodeInfo {
NodeInfo(Fragment frag, uint32_t _k, std::vector<CPubKey> _keys): fragment(frag), n_subs(0), k(_k), keys(std::move(_keys)) {}
};

std::set<std::pair<Fragment, miniscript::Type>> types;

/** Pick an index in a collection from a single byte in the fuzzer's output. */
template<typename T, typename A>
T ConsumeIndex(FuzzedDataProvider& provider, A& col) {
Expand Down
Loading