Skip to content
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

Modernize code: use std::optional instead of bool/outarg #114

Merged
merged 1 commit into from
Apr 21, 2022
Merged

Modernize code: use std::optional instead of bool/outarg #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed some call sites, such as:

miniscript/bitcoin/script/miniscript.h

Line 1693 in 8a81eeb

if (!ParseScriptNumber(in[1], n)) return {};

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
123 changes: 64 additions & 59 deletions bitcoin/script/miniscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ struct Node {
}

template<typename CTx>
bool ToString(const CTx& ctx, std::string& ret) const {
std::optional<std::string> ToString(const CTx& ctx) const {
// To construct the std::string representation for a Miniscript object, we use
// the TreeEvalMaybe algorithm. The State is a boolean: whether the parent node is a
// wrapper. If so, non-wrapper expressions must be prefixed with a ":".
Expand All @@ -612,15 +612,15 @@ struct Node {
case Fragment::WRAP_C:
if (node.subs[0]->fragment == Fragment::PK_K) {
// pk(K) is syntactic sugar for c:pk_k(K)
std::string key_str;
if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {};
return std::move(ret) + "pk(" + std::move(key_str) + ")";
auto key_str = ctx.ToString(node.subs[0]->keys[0]);
if (!key_str) return {};
return std::move(ret) + "pk(" + std::move(*key_str) + ")";
}
if (node.subs[0]->fragment == Fragment::PK_H) {
// pkh(K) is syntactic sugar for c:pk_h(K)
std::string key_str;
if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {};
return std::move(ret) + "pkh(" + std::move(key_str) + ")";
auto key_str = ctx.ToString(node.subs[0]->keys[0]);
if (!key_str) return {};
return std::move(ret) + "pkh(" + std::move(*key_str) + ")";
}
return "c" + std::move(subs[0]);
case Fragment::WRAP_D: return "d" + std::move(subs[0]);
Expand All @@ -639,14 +639,14 @@ struct Node {
}
switch (node.fragment) {
case Fragment::PK_K: {
std::string key_str;
if (!ctx.ToString(node.keys[0], key_str)) return {};
return std::move(ret) + "pk_k(" + std::move(key_str) + ")";
auto key_str = ctx.ToString(node.keys[0]);
if (!key_str) return {};
return std::move(ret) + "pk_k(" + std::move(*key_str) + ")";
}
case Fragment::PK_H: {
std::string key_str;
if (!ctx.ToString(node.keys[0], key_str)) return {};
return std::move(ret) + "pk_h(" + std::move(key_str) + ")";
auto key_str = ctx.ToString(node.keys[0]);
if (!key_str) return {};
return std::move(ret) + "pk_h(" + std::move(*key_str) + ")";
}
case Fragment::AFTER: return std::move(ret) + "after(" + ::ToString(node.k) + ")";
case Fragment::OLDER: return std::move(ret) + "older(" + ::ToString(node.k) + ")";
Expand All @@ -669,9 +669,9 @@ struct Node {
case Fragment::MULTI: {
auto str = std::move(ret) + "multi(" + ::ToString(node.k);
for (const auto& key : node.keys) {
std::string key_str;
if (!ctx.ToString(key, key_str)) return {};
str += "," + std::move(key_str);
auto key_str = ctx.ToString(key);
if (!key_str) return {};
str += "," + std::move(*key_str);
}
return std::move(str) + ")";
}
Expand All @@ -687,9 +687,7 @@ struct Node {
return ""; // Should never be reached.
};

auto res = TreeEvalMaybe<std::string>(false, downfn, upfn);
if (res.has_value()) ret = std::move(*res);
return res.has_value();
return TreeEvalMaybe<std::string>(false, downfn, upfn);
}

internal::Ops CalcOps() const {
Expand Down Expand Up @@ -1181,11 +1179,11 @@ int FindNextChar(Span<const char> in, const char m);
template<typename Key, typename 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 {{std::move(key), key_size}};
auto key = ctx.FromString(in.begin(), in.begin() + key_size);
if (!key) return {};
return {{std::move(*key), key_size}};
}

/** Parse a hex string ending at the end of the fragment's text representation. */
Expand Down Expand Up @@ -1356,12 +1354,12 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
// Get keys
std::vector<Key> keys;
while (next_comma != -1) {
Key key;
next_comma = FindNextChar(in, ',');
int key_length = (next_comma == -1) ? FindNextChar(in, ')') : next_comma;
if (key_length < 1) return {};
if (!ctx.FromString(in.begin(), in.begin() + key_length, key)) return {};
keys.push_back(std::move(key));
auto key = ctx.FromString(in.begin(), in.begin() + key_length);
if (!key) return {};
keys.push_back(std::move(*key));
in = in.subspan(key_length + 1);
}
if (keys.size() < 1 || keys.size() > 20) return {};
Expand Down Expand Up @@ -1532,10 +1530,10 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
* and OP_EQUALVERIFY are decomposed into OP_CHECKSIG, OP_CHECKMULTISIG, OP_EQUAL
* respectively, plus OP_VERIFY.
*/
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);

/** Determine whether the passed pair (created by DecomposeScript) is pushing a number. */
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);

enum class DecodeContext {
/** A single expression of type B, K, or V. Specifically, this can't be an
Expand Down Expand Up @@ -1642,34 +1640,38 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
}
// Public keys
if (in[0].second.size() == 33) {
Key key;
if (!ctx.FromPKBytes(in[0].second.begin(), in[0].second.end(), key)) return {};
auto key = ctx.FromPKBytes(in[0].second.begin(), in[0].second.end());
if (!key) return {};
++in;
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_K, Vector(std::move(key))));
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_K, Vector(std::move(*key))));
break;
}
if (last - in >= 5 && in[0].first == OP_VERIFY && in[1].first == OP_EQUAL && in[3].first == OP_HASH160 && in[4].first == OP_DUP && in[2].second.size() == 20) {
Key key;
if (!ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end(), key)) return {};
auto key = ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end());
if (!key) return {};
in += 5;
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_H, Vector(std::move(key))));
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_H, Vector(std::move(*key))));
break;
}
// Time locks
if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && ParseScriptNumber(in[1], k)) {
if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY) {
auto num = ParseScriptNumber(in[1]);
in += 2;
if (k < 1 || k > 0x7FFFFFFFL) return {};
constructed.push_back(MakeNodeRef<Key>(Fragment::OLDER, k));
if (!num || *num < 1 || *num > 0x7FFFFFFFL) return {};
constructed.push_back(MakeNodeRef<Key>(Fragment::OLDER, *num));
break;
}
if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && ParseScriptNumber(in[1], k)) {
if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY) {
auto num = ParseScriptNumber(in[1]);
in += 2;
if (k < 1 || k > 0x7FFFFFFFL) return {};
constructed.push_back(MakeNodeRef<Key>(Fragment::AFTER, k));
if (!num || num < 1 || num > 0x7FFFFFFFL) return {};
constructed.push_back(MakeNodeRef<Key>(Fragment::AFTER, *num));
break;
}
// Hashes
if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && ParseScriptNumber(in[5], k) && k == 32 && in[6].first == OP_SIZE) {
if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && in[6].first == OP_SIZE) {
auto num = ParseScriptNumber(in[5]);
if (!num || num != 32) return {};
if (in[2].first == OP_SHA256 && in[1].second.size() == 32) {
constructed.push_back(MakeNodeRef<Key>(Fragment::SHA256, in[1].second));
in += 7;
Expand All @@ -1691,20 +1693,22 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
// Multi
if (last - in >= 3 && in[0].first == OP_CHECKMULTISIG) {
std::vector<Key> keys;
if (!ParseScriptNumber(in[1], n)) return {};
if (last - in < 3 + n) return {};
if (n < 1 || n > 20) return {};
for (int i = 0; i < n; ++i) {
Key key;
auto n = ParseScriptNumber(in[1]);
if (!n) return {};
if (last - in < 3 + *n) return {};
if (*n < 1 || *n > 20) return {};
for (int i = 0; i < *n; ++i) {
if (in[2 + i].second.size() != 33) return {};
if (!ctx.FromPKBytes(in[2 + i].second.begin(), in[2 + i].second.end(), key)) return {};
keys.push_back(std::move(key));
auto key = ctx.FromPKBytes(in[2 + i].second.begin(), in[2 + i].second.end());
if (!key) return {};
keys.push_back(std::move(*key));
}
if (!ParseScriptNumber(in[2 + n], k)) return {};
if (k < 1 || k > n) return {};
in += 3 + n;
auto k = ParseScriptNumber(in[2 + *n]);
if (!k) return {};
if (*k < 1 || *k > *n) return {};
in += 3 + *n;
std::reverse(keys.begin(), keys.end());
constructed.push_back(MakeNodeRef<Key>(Fragment::MULTI, std::move(keys), k));
constructed.push_back(MakeNodeRef<Key>(Fragment::MULTI, std::move(keys), *k));
break;
}
/** In the following wrappers, we only need to push SINGLE_BKV_EXPR rather
Expand Down Expand Up @@ -1732,10 +1736,11 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
break;
}
// Thresh
if (last - in >= 3 && in[0].first == OP_EQUAL && ParseScriptNumber(in[1], k)) {
if (k < 1) return {};
if (last - in >= 3 && in[0].first == OP_EQUAL) {
auto k = ParseScriptNumber(in[1]);
if (!k || *k < 1) return {};
in += 2;
to_parse.emplace_back(DecodeContext::THRESH_W, 0, k);
to_parse.emplace_back(DecodeContext::THRESH_W, 0, *k);
break;
}
// OP_ENDIF can be WRAP_J, WRAP_D, ANDOR, OR_C, OR_D, or OR_I
Expand Down Expand Up @@ -1970,12 +1975,12 @@ inline NodeRef<typename Ctx::Key> FromString(const std::string& str, const Ctx&
template<typename Ctx>
inline NodeRef<typename Ctx::Key> FromScript(const CScript& script, const Ctx& ctx) {
using namespace internal;
std::vector<std::pair<opcodetype, std::vector<unsigned char>>> decomposed;
if (!DecomposeScript(script, decomposed)) return {};
auto it = decomposed.begin();
auto ret = DecodeScript<typename Ctx::Key>(it, decomposed.end(), ctx);
auto decomposed = DecomposeScript(script);
if (!decomposed) return {};
auto it = decomposed->begin();
auto ret = DecodeScript<typename Ctx::Key>(it, decomposed->end(), ctx);
if (!ret) return {};
if (it != decomposed.end()) return {};
if (it != decomposed->end()) return {};
return ret;
}

Expand Down
39 changes: 20 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 @@ -144,19 +143,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
Loading