Skip to content

Mark Miniscript with duplicate keys as insane #116

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
merged 2 commits into from
Apr 21, 2022
Merged

Mark Miniscript with duplicate keys as insane #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
033238f
Modernize code: use std::optional instead of bool/outarg
sipa Apr 5, 2022
13edbef
miniscript: mark nodes with duplicate keys as insane
darosior Apr 14, 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
25 changes: 11 additions & 14 deletions bitcoin/script/miniscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,16 +351,15 @@ InputStack operator|(InputStack a, InputStack b) {
}
}

bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, std::vector<unsigned char>>>& out)
std::optional<std::vector<std::pair<opcodetype, std::vector<unsigned char>>>> DecomposeScript(const CScript& script)
{
out.clear();
std::vector<std::pair<opcodetype, std::vector<unsigned char>>> out;
CScript::const_iterator it = script.begin(), itend = script.end();
while (it != itend) {
std::vector<unsigned char> push_data;
opcodetype opcode;
if (!script.GetOp(it, opcode, push_data)) {
out.clear();
return false;
return {};
} else if (opcode >= OP_1 && opcode <= OP_16) {
// Deal with OP_n (GetOp does not turn them into pushes).
push_data.assign(1, CScript::DecodeOP_N(opcode));
Expand All @@ -377,30 +376,28 @@ bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, st
out.emplace_back(OP_EQUAL, std::vector<unsigned char>());
opcode = OP_VERIFY;
} else if (IsPushdataOp(opcode)) {
if (!CheckMinimalPush(push_data, opcode)) return false;
if (!CheckMinimalPush(push_data, opcode)) return {};
} else if (it != itend && (opcode == OP_CHECKSIG || opcode == OP_CHECKMULTISIG || opcode == OP_EQUAL) && (*it == OP_VERIFY)) {
// Rule out non minimal VERIFY sequences
return false;
return {};
}
out.emplace_back(opcode, std::move(push_data));
}
std::reverse(out.begin(), out.end());
return true;
return out;
}

bool ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in, int64_t& k) {
std::optional<int64_t> ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in) {
if (in.first == OP_0) {
k = 0;
return true;
return 0;
}
if (!in.second.empty()) {
if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return false;
if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return {};
try {
k = CScriptNum(in.second, true).GetInt64();
return true;
return CScriptNum(in.second, true).GetInt64();
} catch(const scriptnum_error&) {}
}
return false;
return {};
}

int FindNextChar(Span<const char> sp, const char m)
Expand Down
178 changes: 112 additions & 66 deletions bitcoin/script/miniscript.h
View file
Open in desktop

Large diffs are not rendered by default.

41 changes: 22 additions & 19 deletions bitcoin/test/fuzz/miniscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,12 @@ struct TestData {
struct ParserContext {
typedef CPubKey Key;

bool ToString(const Key& key, std::string& ret) const
std::optional<std::string> ToString(const Key& key) const
{
auto it = TEST_DATA.dummy_key_idx_map.find(key);
if (it == TEST_DATA.dummy_key_idx_map.end()) return false;
if (it == TEST_DATA.dummy_key_idx_map.end()) return {};
uint8_t idx = it->second;
ret = HexStr(Span{&idx, 1});
return true;
return HexStr(Span{&idx, 1});
}

const std::vector<unsigned char> ToPKBytes(const Key& key) const
Expand All @@ -97,29 +96,29 @@ struct ParserContext {
}

template<typename I>
bool FromString(I first, I last, Key& key) const {
if (last - first != 2) return false;
std::optional<Key> FromString(I first, I last) const {
if (last - first != 2) return {};
auto idx = ParseHex(std::string(first, last));
if (idx.size() != 1) return false;
key = TEST_DATA.dummy_keys[idx[0]];
return true;
if (idx.size() != 1) return {};
return TEST_DATA.dummy_keys[idx[0]];
}

template<typename I>
bool FromPKBytes(I first, I last, CPubKey& key) const {
std::optional<CPubKey> FromPKBytes(I first, I last) const {
CPubKey key;
key.Set(first, last);
return key.IsValid();
if (!key.IsValid()) return {};
return key;
}

template<typename I>
bool FromPKHBytes(I first, I last, CPubKey& key) const {
std::optional<CPubKey> FromPKHBytes(I first, I last) const {
assert(last - first == 20);
CKeyID keyid;
std::copy(first, last, keyid.begin());
const auto it = TEST_DATA.dummy_keys_map.find(keyid);
if (it == TEST_DATA.dummy_keys_map.end()) return false;
key = it->second;
return true;
if (it == TEST_DATA.dummy_keys_map.end()) return {};
return it->second;
}
} PARSER_CTX;

Expand All @@ -128,6 +127,8 @@ struct ScriptParserContext {
struct Key {
bool is_hash;
std::vector<unsigned char> data;

bool operator<(Key k) const { return data < k.data; }
};

const std::vector<unsigned char>& ToPKBytes(const Key& key) const
Expand All @@ -144,19 +145,21 @@ struct ScriptParserContext {
}

template<typename I>
bool FromPKBytes(I first, I last, Key& key) const
std::optional<Key> FromPKBytes(I first, I last) const
{
Key key;
key.data.assign(first, last);
key.is_hash = false;
return true;
return key;
}

template<typename I>
bool FromPKHBytes(I first, I last, Key& key) const
std::optional<Key> FromPKHBytes(I first, I last) const
{
Key key;
key.data.assign(first, last);
key.is_hash = true;
return true;
return key;
}
} SCRIPT_PARSER_CONTEXT;

Expand Down
33 changes: 26 additions & 7 deletions bitcoin/test/miniscript_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,27 +126,33 @@ struct KeyConverter {

//! Parse a public key from a range of hex characters.
template<typename I>
bool FromString(I first, I last, CPubKey& key) const {
std::optional<CPubKey> FromString(I first, I last) const
{
auto bytes = ParseHex(std::string(first, last));
CPubKey key;
key.Set(bytes.begin(), bytes.end());
return key.IsValid();
if (!key.IsValid()) return {};
return key;
}

template<typename I>
bool FromPKBytes(I first, I last, CPubKey& key) const {
std::optional<CPubKey> bool FromPKBytes(I first, I last) const
{
CPubKey key;
key.Set(first, last);
return key.IsValid();
if (!key.IsValid()) return {};
return key;
}

template<typename I>
bool FromPKHBytes(I first, I last, CPubKey& key) const {
std::optional<CPubKey> FromPKHBytes(I first, I last) const
{
assert(last - first == 20);
CKeyID keyid;
std::copy(first, last, keyid.begin());
auto it = g_testdata->pkmap.find(keyid);
assert(it != g_testdata->pkmap.end());
key = it->second;
return true;
return it->second;
}
};

Expand Down Expand Up @@ -499,6 +505,19 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
// its subs to all be 'u' (taken from https://github.com/rust-bitcoin/rust-miniscript/discussions/341).
const auto ms_minimalif = miniscript::FromString("thresh(3,c:pk_k(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),sc:pk_k(03fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),sc:pk_k(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798),sdv:older(32))", CONVERTER);
BOOST_CHECK(!ms_minimalif);
// A Miniscript with duplicate keys is not sane
const auto ms_dup1 = miniscript::FromString("and_v(v:pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65))", CONVERTER);
BOOST_CHECK(ms_dup1);
BOOST_CHECK(!ms_dup1->IsSane());
// Same with a disjunction, and different key nodes (pk and pkh)
const auto ms_dup2 = miniscript::FromString("or_b(c:pk_k(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),ac:pk_h(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65))", CONVERTER);
BOOST_CHECK(ms_dup2 && !ms_dup2->IsSane());
// Same when the duplicates are leaves or a larger tree
const auto ms_dup3 = miniscript::FromString("or_i(and_b(pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),s:pk(03fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556)),and_b(older(1),s:pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65)))", CONVERTER);
BOOST_CHECK(ms_dup3 && !ms_dup3->IsSane());
// Same when the duplicates are on different levels in the tree
const auto ms_dup4 = miniscript::FromString("thresh(2,pkh(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),s:pk(03fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),a:and_b(dv:older(1),s:pk(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65)))", CONVERTER);
BOOST_CHECK(ms_dup4 && !ms_dup4->IsSane());

// Timelock tests
Test("after(100)", "?", TESTMODE_VALID | TESTMODE_NONMAL); // only heightlock
Expand Down
85 changes: 42 additions & 43 deletions compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,16 @@ struct Policy {
Policy& operator=(Policy&& x) = default;
Policy(Policy&& x) = default;

explicit Policy(Type nt) : node_type(nt) {}
explicit Policy(Type nt, uint32_t kv) : node_type(nt), k(kv) {}
explicit Policy(Type nt, std::vector<unsigned char>&& dat) : node_type(nt), data(std::move(dat)) {}
explicit Policy(Type nt, std::vector<unsigned char>&& dat, uint32_t kv) : node_type(nt), data(std::move(dat)), k(kv) {}
explicit Policy(Type nt, std::vector<Policy>&& subs) : node_type(nt), sub(std::move(subs)) {}
explicit Policy(Type nt, std::vector<CompilerContext::Key>&& key) : node_type(nt), keys(std::move(key)) {}
explicit Policy(Type nt, std::vector<Policy>&& subs, std::vector<uint32_t>&& probs) : node_type(nt), sub(std::move(subs)), prob(std::move(probs)) {}
explicit Policy(Type nt, std::vector<Policy>&& subs, uint32_t kv) : node_type(nt), sub(std::move(subs)), k(kv) {}
explicit Policy(Type nt, std::vector<CompilerContext::Key>&& key, uint32_t kv) : node_type(nt), keys(std::move(key)), k(kv) {}
Policy() {}
Policy(Type nt) : node_type(nt) {}
Policy(Type nt, uint32_t kv) : node_type(nt), k(kv) {}
Policy(Type nt, std::vector<unsigned char>&& dat) : node_type(nt), data(std::move(dat)) {}
Policy(Type nt, std::vector<unsigned char>&& dat, uint32_t kv) : node_type(nt), data(std::move(dat)), k(kv) {}
Policy(Type nt, std::vector<Policy>&& subs) : node_type(nt), sub(std::move(subs)) {}
Policy(Type nt, std::vector<CompilerContext::Key>&& key) : node_type(nt), keys(std::move(key)) {}
Policy(Type nt, std::vector<Policy>&& subs, std::vector<uint32_t>&& probs) : node_type(nt), sub(std::move(subs)), prob(std::move(probs)) {}
Policy(Type nt, std::vector<Policy>&& subs, uint32_t kv) : node_type(nt), sub(std::move(subs)), k(kv) {}
Policy(Type nt, std::vector<CompilerContext::Key>&& key, uint32_t kv) : node_type(nt), keys(std::move(key)), k(kv) {}

bool operator()() const { return node_type != Type::NONE; }
};
Expand Down Expand Up @@ -96,87 +97,85 @@ Policy Parse(Span<const char>& in) {
using namespace spanparsing;
auto expr = Expr(in);
if (Func("pk", expr)) {
CompilerContext::Key key;
if (COMPILER_CTX.FromString(expr.begin(), expr.end(), key)) {
return Policy(Policy::Type::PK_K, Vector(std::move(key)));
}
return Policy(Policy::Type::NONE);
auto key = COMPILER_CTX.FromString(expr.begin(), expr.end());
if (key) return {Policy::Type::PK_K, Vector(std::move(*key))};
return {};
} else if (Func("after", expr)) {
uint64_t num;
if (!ParseUInt64(std::string(expr.begin(), expr.end()), &num)) {
return Policy(Policy::Type::NONE);
return Policy::Type::NONE;
}
if (num >= 1 && num < 0x80000000UL) {
return Policy(Policy::Type::AFTER, num);
return {Policy::Type::AFTER, uint32_t(num)};
}
return Policy(Policy::Type::NONE);
return {};
} else if (Func("older", expr)) {
uint64_t num;
if (!ParseUInt64(std::string(expr.begin(), expr.end()), &num)) {
return Policy(Policy::Type::NONE);
return Policy::Type::NONE;
}
if (num >= 1 && num < 0x80000000UL) {
return Policy(Policy::Type::OLDER, num);
return {Policy::Type::OLDER, uint32_t(num)};
}
return Policy(Policy::Type::NONE);
return {};
} else if (Func("sha256", expr)) {
auto hash = Hash(expr, 32);
if (hash.size()) return Policy(Policy::Type::SHA256, std::move(hash));
return Policy(Policy::Type::NONE);
if (hash.size()) return {Policy::Type::SHA256, std::move(hash)};
return {};
} else if (Func("ripemd160", expr)) {
auto hash = Hash(expr, 20);
if (hash.size()) return Policy(Policy::Type::RIPEMD160, std::move(hash));
return Policy(Policy::Type::NONE);
if (hash.size()) return {Policy::Type::RIPEMD160, std::move(hash)};
return {};
} else if (Func("hash256", expr)) {
auto hash = Hash(expr, 32);
if (hash.size()) return Policy(Policy::Type::HASH256, std::move(hash));
return Policy(Policy::Type::NONE);
if (hash.size()) return {Policy::Type::HASH256, std::move(hash)};
return {};
} else if (Func("hash160", expr)) {
auto hash = Hash(expr, 20);
if (hash.size()) return Policy(Policy::Type::HASH160, std::move(hash));
return Policy(Policy::Type::NONE);
if (hash.size()) return {Policy::Type::HASH160, std::move(hash)};
return {};
} else if (Func("or", expr)) {
std::vector<Policy> sub;
std::vector<uint32_t> prob;
uint32_t p;
sub.emplace_back(ParseProb(expr, p));
if (!sub.back()()) return Policy(Policy::Type::NONE);
if (!sub.back()()) return {};
prob.push_back(p);
while (expr.size()) {
if (!Const(",", expr)) return Policy(Policy::Type::NONE);
if (!Const(",", expr)) return {};
sub.emplace_back(ParseProb(expr, p));
if (!sub.back()()) return Policy(Policy::Type::NONE);
if (!sub.back()()) return {};
prob.push_back(p);
}
return Policy(Policy::Type::OR, std::move(sub), std::move(prob));
return {Policy::Type::OR, std::move(sub), std::move(prob)};
} else if (Func("and", expr)) {
std::vector<Policy> sub;
sub.emplace_back(Parse(expr));
if (!sub.back()()) return Policy(Policy::Type::NONE);
if (!sub.back()()) return {};
while (expr.size()) {
if (!Const(",", expr)) return Policy(Policy::Type::NONE);
if (!Const(",", expr)) return {};
sub.emplace_back(Parse(expr));
if (!sub.back()()) return Policy(Policy::Type::NONE);
if (!sub.back()()) return {};
}
return Policy(Policy::Type::AND, std::move(sub));
return {Policy::Type::AND, std::move(sub)};
} else if (Func("thresh", expr)) {
auto arg = Expr(expr);
uint32_t count;
if (!ParseUInt32(std::string(arg.begin(), arg.end()), &count)) {
return Policy(Policy::Type::NONE);
return {};
}
if (count < 1) return Policy(Policy::Type::NONE);
if (count < 1) return {};
std::vector<Policy> sub;
while (expr.size()) {
if (!Const(",", expr)) return Policy(Policy::Type::NONE);
if (!Const(",", expr)) return {};
sub.emplace_back(Parse(expr));
if (!sub.back()()) return Policy(Policy::Type::NONE);
if (!sub.back()()) return {};
}
if (sub.size() > 100 || count > sub.size()) return Policy(Policy::Type::NONE);
return Policy(Policy::Type::THRESH, std::move(sub), count);
if (sub.size() > 100 || count > sub.size()) return {};
return {Policy::Type::THRESH, std::move(sub), count};
}

return Policy(Policy::Type::NONE);
return {};
}

Policy Parse(const std::string& in) {
Expand Down
9 changes: 4 additions & 5 deletions compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@
struct CompilerContext {
typedef std::string Key;

bool ToString(const Key& key, std::string& str) const { str = key; return true; }
std::optional<std::string> ToString(const Key& key) const { return key; }

template<typename I>
bool FromString(I first, I last, Key& key) const {
if (std::distance(first, last) == 0 || std::distance(first, last) > 17) return false;
key = std::string(first, last);
return true;
std::optional<Key> FromString(I first, I last) const {
if (std::distance(first, last) == 0 || std::distance(first, last) > 17) return {};
return std::string(first, last);
}

std::vector<unsigned char> ToPKBytes(const Key& key) const {
Expand Down
12 changes: 6 additions & 6 deletions main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ static bool run(std::string&& line, int64_t count) {
miniscript::NodeRef<std::string> ret;
double avgcost = 0;
if (Compile(Expand(line), ret, avgcost)) {
std::string str;
ret->ToString(COMPILER_CTX, str);
printf("X %17.10f %5i %s %s\n", ret->ScriptSize() + avgcost, (int)ret->ScriptSize(), Abbreviate(std::move(str)).c_str(), line.c_str());
auto str = ret->ToString(COMPILER_CTX);
assert(str);
printf("X %17.10f %5i %s %s\n", ret->ScriptSize() + avgcost, (int)ret->ScriptSize(), Abbreviate(std::move(*str)).c_str(), line.c_str());
} else if ((ret = miniscript::FromString(Expand(line), COMPILER_CTX))) {
std::string ms;
ret->ToString(COMPILER_CTX, ms);
printf("%7li scriptlen=%i maxops=%i type=%s safe=%s nonmal=%s dissat=%s input=%s output=%s timelock_mix=%s miniscript=%s\n", (long)count, (int)ret->ScriptSize(), (int)ret->GetOps(), ret->GetType() << "B"_mst ? "B" : ret->GetType() << "V"_mst ? "V" : ret->GetType() << "W"_mst ? "W" : ret->GetType() << "K"_mst ? "K" : "(invalid)", ret->GetType() << "s"_mst ? "yes" : "no", ret->GetType() << "m"_mst ? "yes" : "no", ret->GetType() << "f"_mst ? "no" : ret->GetType() << "e"_mst ? "unique" : ret->GetType() << "d"_mst ? "yes" : "unknown", ret->GetType() << "z"_mst ? "0" : ret->GetType() << "o"_mst ? (ret->GetType() << "n"_mst ? "1n" : "1") : ret->GetType() << "n"_mst ? "n" : "-", ret->GetType() << "u"_mst ? "1" : "nonzero", ret->GetType() << "k"_mst ? "no": "yes", Abbreviate(ms).c_str());
auto ms = ret->ToString(COMPILER_CTX);
assert(ms);
printf("%7li scriptlen=%i maxops=%i type=%s safe=%s nonmal=%s dissat=%s input=%s output=%s timelock_mix=%s miniscript=%s\n", (long)count, (int)ret->ScriptSize(), (int)ret->GetOps(), ret->GetType() << "B"_mst ? "B" : ret->GetType() << "V"_mst ? "V" : ret->GetType() << "W"_mst ? "W" : ret->GetType() << "K"_mst ? "K" : "(invalid)", ret->GetType() << "s"_mst ? "yes" : "no", ret->GetType() << "m"_mst ? "yes" : "no", ret->GetType() << "f"_mst ? "no" : ret->GetType() << "e"_mst ? "unique" : ret->GetType() << "d"_mst ? "yes" : "unknown", ret->GetType() << "z"_mst ? "0" : ret->GetType() << "o"_mst ? (ret->GetType() << "n"_mst ? "1n" : "1") : ret->GetType() << "n"_mst ? "n" : "-", ret->GetType() << "u"_mst ? "1" : "nonzero", ret->GetType() << "k"_mst ? "no": "yes", Abbreviate(*ms).c_str());
} else {
printf("Failed to parse as policy or miniscript '%s'\n", line.c_str());
}
Expand Down