Skip to content

Commit 8a81eeb

Browse files
sipasipa
committed
Modernize code: use std::optional instead of bool/outarg
1 parent b431733 commit 8a81eeb

File tree

6 files changed

+130
-135
lines changed

6 files changed

+130
-135
lines changed

‎bitcoin/script/miniscript.cpp‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -352,16 +352,15 @@ InputStack operator|(InputStack a, InputStack b) {
352352
}
353353
}
354354

355-
bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, std::vector<unsigned char>>>& out)
355+
std::optional<std::vector<std::pair<opcodetype, std::vector<unsigned char>>>> DecomposeScript(const CScript& script)
356356
{
357-
out.clear();
357+
std::vector<std::pair<opcodetype, std::vector<unsigned char>>> out;
358358
CScript::const_iterator it = script.begin(), itend = script.end();
359359
while (it != itend) {
360360
std::vector<unsigned char> push_data;
361361
opcodetype opcode;
362362
if (!script.GetOp(it, opcode, push_data)) {
363-
out.clear();
364-
return false;
363+
return {};
365364
} else if (opcode >= OP_1 && opcode <= OP_16) {
366365
// Deal with OP_n (GetOp does not turn them into pushes).
367366
push_data.assign(1, CScript::DecodeOP_N(opcode));
@@ -378,30 +377,28 @@ bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, st
378377
out.emplace_back(OP_EQUAL, std::vector<unsigned char>());
379378
opcode = OP_VERIFY;
380379
} else if (IsPushdataOp(opcode)) {
381-
if (!CheckMinimalPush(push_data, opcode)) return false;
380+
if (!CheckMinimalPush(push_data, opcode)) return {};
382381
} else if (it != itend && (opcode == OP_CHECKSIG || opcode == OP_CHECKMULTISIG || opcode == OP_EQUAL) && (*it == OP_VERIFY)) {
383382
// Rule out non minimal VERIFY sequences
384-
return false;
383+
return {};
385384
}
386385
out.emplace_back(opcode, std::move(push_data));
387386
}
388387
std::reverse(out.begin(), out.end());
389-
return true;
388+
return out;
390389
}
391390

392-
bool ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in, int64_t& k) {
391+
std::optional<int64_t> ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in) {
393392
if (in.first == OP_0) {
394-
k = 0;
395-
return true;
393+
return 0;
396394
}
397395
if (!in.second.empty()) {
398-
if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return false;
396+
if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return {};
399397
try {
400-
k = CScriptNum(in.second, true).GetInt64();
401-
return true;
398+
return CScriptNum(in.second, true).GetInt64();
402399
} catch(const scriptnum_error&) {}
403400
}
404-
return false;
401+
return {};
405402
}
406403

407404
int FindNextChar(Span<const char> sp, const char m)

‎bitcoin/script/miniscript.h‎

Lines changed: 47 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -588,7 +588,7 @@ struct Node {
588588
}
589589

590590
template<typename CTx>
591-
bool ToString(const CTx& ctx, std::string& ret) const {
591+
std::optional<std::string> ToString(const CTx& ctx) const {
592592
// To construct the std::string representation for a Miniscript object, we use
593593
// the TreeEvalMaybe algorithm. The State is a boolean: whether the parent node is a
594594
// wrapper. If so, non-wrapper expressions must be prefixed with a ":".
@@ -612,15 +612,15 @@ struct Node {
612612
case Fragment::WRAP_C:
613613
if (node.subs[0]->fragment == Fragment::PK_K) {
614614
// pk(K) is syntactic sugar for c:pk_k(K)
615-
std::string key_str;
616-
if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {};
617-
return std::move(ret) + "pk(" + std::move(key_str) + ")";
615+
auto key_str = ctx.ToString(node.subs[0]->keys[0]);
616+
if (!key_str) return {};
617+
return std::move(ret) + "pk(" + std::move(*key_str) + ")";
618618
}
619619
if (node.subs[0]->fragment == Fragment::PK_H) {
620620
// pkh(K) is syntactic sugar for c:pk_h(K)
621-
std::string key_str;
622-
if (!ctx.ToString(node.subs[0]->keys[0], key_str)) return {};
623-
return std::move(ret) + "pkh(" + std::move(key_str) + ")";
621+
auto key_str = ctx.ToString(node.subs[0]->keys[0]);
622+
if (!key_str) return {};
623+
return std::move(ret) + "pkh(" + std::move(*key_str) + ")";
624624
}
625625
return "c" + std::move(subs[0]);
626626
case Fragment::WRAP_D: return "d" + std::move(subs[0]);
@@ -639,14 +639,14 @@ struct Node {
639639
}
640640
switch (node.fragment) {
641641
case Fragment::PK_K: {
642-
std::string key_str;
643-
if (!ctx.ToString(node.keys[0], key_str)) return {};
644-
return std::move(ret) + "pk_k(" + std::move(key_str) + ")";
642+
auto key_str = ctx.ToString(node.keys[0]);
643+
if (!key_str) return {};
644+
return std::move(ret) + "pk_k(" + std::move(*key_str) + ")";
645645
}
646646
case Fragment::PK_H: {
647-
std::string key_str;
648-
if (!ctx.ToString(node.keys[0], key_str)) return {};
649-
return std::move(ret) + "pk_h(" + std::move(key_str) + ")";
647+
auto key_str = ctx.ToString(node.keys[0]);
648+
if (!key_str) return {};
649+
return std::move(ret) + "pk_h(" + std::move(*key_str) + ")";
650650
}
651651
case Fragment::AFTER: return std::move(ret) + "after(" + ::ToString(node.k) + ")";
652652
case Fragment::OLDER: return std::move(ret) + "older(" + ::ToString(node.k) + ")";
@@ -669,9 +669,9 @@ struct Node {
669669
case Fragment::MULTI: {
670670
auto str = std::move(ret) + "multi(" + ::ToString(node.k);
671671
for (const auto& key : node.keys) {
672-
std::string key_str;
673-
if (!ctx.ToString(key, key_str)) return {};
674-
str += "," + std::move(key_str);
672+
auto key_str = ctx.ToString(key);
673+
if (!key_str) return {};
674+
str += "," + std::move(*key_str);
675675
}
676676
return std::move(str) + ")";
677677
}
@@ -687,9 +687,7 @@ struct Node {
687687
return ""; // Should never be reached.
688688
};
689689

690-
auto res = TreeEvalMaybe<std::string>(false, downfn, upfn);
691-
if (res.has_value()) ret = std::move(*res);
692-
return res.has_value();
690+
return TreeEvalMaybe<std::string>(false, downfn, upfn);
693691
}
694692

695693
internal::Ops CalcOps() const {
@@ -1181,11 +1179,11 @@ int FindNextChar(Span<const char> in, const char m);
11811179
template<typename Key, typename Ctx>
11821180
std::optional<std::pair<Key, int>> ParseKeyEnd(Span<const char> in, const Ctx& ctx)
11831181
{
1184-
Key key;
11851182
int key_size = FindNextChar(in, ')');
11861183
if (key_size < 1) return {};
1187-
if (!ctx.FromString(in.begin(), in.begin() + key_size, key)) return {};
1188-
return {{std::move(key), key_size}};
1184+
auto key = ctx.FromString(in.begin(), in.begin() + key_size);
1185+
if (!key) return {};
1186+
return {{std::move(*key), key_size}};
11891187
}
11901188

11911189
/** Parse a hex string ending at the end of the fragment's text representation. */
@@ -1356,12 +1354,12 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
13561354
// Get keys
13571355
std::vector<Key> keys;
13581356
while (next_comma != -1) {
1359-
Key key;
13601357
next_comma = FindNextChar(in, ',');
13611358
int key_length = (next_comma == -1) ? FindNextChar(in, ')') : next_comma;
13621359
if (key_length < 1) return {};
1363-
if (!ctx.FromString(in.begin(), in.begin() + key_length, key)) return {};
1364-
keys.push_back(std::move(key));
1360+
auto key = ctx.FromString(in.begin(), in.begin() + key_length);
1361+
if (!key) return {};
1362+
keys.push_back(std::move(*key));
13651363
in = in.subspan(key_length + 1);
13661364
}
13671365
if (keys.size() < 1 || keys.size() > 20) return {};
@@ -1532,10 +1530,10 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
15321530
* and OP_EQUALVERIFY are decomposed into OP_CHECKSIG, OP_CHECKMULTISIG, OP_EQUAL
15331531
* respectively, plus OP_VERIFY.
15341532
*/
1535-
bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, std::vector<unsigned char>>>& out);
1533+
std::optional<std::vector<std::pair<opcodetype, std::vector<unsigned char>>>> DecomposeScript(const CScript& script);
15361534

15371535
/** Determine whether the passed pair (created by DecomposeScript) is pushing a number. */
1538-
bool ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in, int64_t& k);
1536+
std::optional<int64_t> ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in);
15391537

15401538
enum class DecodeContext {
15411539
/** A single expression of type B, K, or V. Specifically, this can't be an
@@ -1642,34 +1640,35 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
16421640
}
16431641
// Public keys
16441642
if (in[0].second.size() == 33) {
1645-
Key key;
1646-
if (!ctx.FromPKBytes(in[0].second.begin(), in[0].second.end(), key)) return {};
1643+
auto key = ctx.FromPKBytes(in[0].second.begin(), in[0].second.end());
1644+
if (!key) return {};
16471645
++in;
1648-
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_K, Vector(std::move(key))));
1646+
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_K, Vector(std::move(*key))));
16491647
break;
16501648
}
16511649
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) {
1652-
Key key;
1653-
if (!ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end(), key)) return {};
1650+
auto key = ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end());
1651+
if (!key) return {};
16541652
in += 5;
1655-
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_H, Vector(std::move(key))));
1653+
constructed.push_back(MakeNodeRef<Key>(Fragment::PK_H, Vector(std::move(*key))));
16561654
break;
16571655
}
16581656
// Time locks
1659-
if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && ParseScriptNumber(in[1], k)) {
1657+
std::optional<int64_t> num;
1658+
if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && (num = ParseScriptNumber(in[1]))) {
16601659
in += 2;
1661-
if (k < 1 || k > 0x7FFFFFFFL) return {};
1662-
constructed.push_back(MakeNodeRef<Key>(Fragment::OLDER, k));
1660+
if (*num < 1 || *num > 0x7FFFFFFFL) return {};
1661+
constructed.push_back(MakeNodeRef<Key>(Fragment::OLDER, *num));
16631662
break;
16641663
}
1665-
if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && ParseScriptNumber(in[1], k)) {
1664+
if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) {
16661665
in += 2;
1667-
if (k < 1 || k > 0x7FFFFFFFL) return {};
1668-
constructed.push_back(MakeNodeRef<Key>(Fragment::AFTER, k));
1666+
if (num < 1 || num > 0x7FFFFFFFL) return {};
1667+
constructed.push_back(MakeNodeRef<Key>(Fragment::AFTER, *num));
16691668
break;
16701669
}
16711670
// Hashes
1672-
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) {
1671+
if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && (num = ParseScriptNumber(in[5])) && num == 32 && in[6].first == OP_SIZE) {
16731672
if (in[2].first == OP_SHA256 && in[1].second.size() == 32) {
16741673
constructed.push_back(MakeNodeRef<Key>(Fragment::SHA256, in[1].second));
16751674
in += 7;
@@ -1695,10 +1694,10 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
16951694
if (last - in < 3 + n) return {};
16961695
if (n < 1 || n > 20) return {};
16971696
for (int i = 0; i < n; ++i) {
1698-
Key key;
16991697
if (in[2 + i].second.size() != 33) return {};
1700-
if (!ctx.FromPKBytes(in[2 + i].second.begin(), in[2 + i].second.end(), key)) return {};
1701-
keys.push_back(std::move(key));
1698+
auto key = ctx.FromPKBytes(in[2 + i].second.begin(), in[2 + i].second.end());
1699+
if (!key) return {};
1700+
keys.push_back(std::move(*key));
17021701
}
17031702
if (!ParseScriptNumber(in[2 + n], k)) return {};
17041703
if (k < 1 || k > n) return {};
@@ -1970,12 +1969,12 @@ inline NodeRef<typename Ctx::Key> FromString(const std::string& str, const Ctx&
19701969
template<typename Ctx>
19711970
inline NodeRef<typename Ctx::Key> FromScript(const CScript& script, const Ctx& ctx) {
19721971
using namespace internal;
1973-
std::vector<std::pair<opcodetype, std::vector<unsigned char>>> decomposed;
1974-
if (!DecomposeScript(script, decomposed)) return {};
1975-
auto it = decomposed.begin();
1976-
auto ret = DecodeScript<typename Ctx::Key>(it, decomposed.end(), ctx);
1972+
auto decomposed = DecomposeScript(script);
1973+
if (!decomposed) return {};
1974+
auto it = decomposed->begin();
1975+
auto ret = DecodeScript<typename Ctx::Key>(it, decomposed->end(), ctx);
19771976
if (!ret) return {};
1978-
if (it != decomposed.end()) return {};
1977+
if (it != decomposed->end()) return {};
19791978
return ret;
19801979
}
19811980

‎bitcoin/test/fuzz/miniscript.cpp‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -77,13 +77,12 @@ struct TestData {
7777
struct ParserContext {
7878
typedef CPubKey Key;
7979

80-
bool ToString(const Key& key, std::string& ret) const
80+
std::optional<std::string> ToString(const Key& key, std::string& ret) const
8181
{
8282
auto it = TEST_DATA.dummy_key_idx_map.find(key);
83-
if (it == TEST_DATA.dummy_key_idx_map.end()) return false;
83+
if (it == TEST_DATA.dummy_key_idx_map.end()) return {};
8484
uint8_t idx = it->second;
85-
ret = HexStr(Span{&idx, 1});
86-
return true;
85+
return HexStr(Span{&idx, 1});
8786
}
8887

8988
const std::vector<unsigned char> ToPKBytes(const Key& key) const
@@ -98,29 +97,29 @@ struct ParserContext {
9897
}
9998

10099
template<typename I>
101-
bool FromString(I first, I last, Key& key) const {
102-
if (last - first != 2) return false;
100+
std::optional<Key> FromString(I first, I last) const {
101+
if (last - first != 2) return {};
103102
auto idx = ParseHex(std::string(first, last));
104-
if (idx.size() != 1) return false;
105-
key = TEST_DATA.dummy_keys[idx[0]];
106-
return true;
103+
if (idx.size() != 1) return {};
104+
return TEST_DATA.dummy_keys[idx[0]];
107105
}
108106

109107
template<typename I>
110-
bool FromPKBytes(I first, I last, CPubKey& key) const {
108+
std::optional<CPubKey> FromPKBytes(I first, I last) const {
109+
CPubKey key;
111110
key.Set(first, last);
112-
return key.IsValid();
111+
if (!key.IsValid()) return {};
112+
return key;
113113
}
114114

115115
template<typename I>
116-
bool FromPKHBytes(I first, I last, CPubKey& key) const {
116+
std::optional<CPubKey> FromPKHBytes(I first, I last) const {
117117
assert(last - first == 20);
118118
CKeyID keyid;
119119
std::copy(first, last, keyid.begin());
120120
const auto it = TEST_DATA.dummy_keys_map.find(keyid);
121-
if (it == TEST_DATA.dummy_keys_map.end()) return false;
122-
key = it->second;
123-
return true;
121+
if (it == TEST_DATA.dummy_keys_map.end()) return {};
122+
return it->second;
124123
}
125124
} PARSER_CTX;
126125

@@ -145,19 +144,21 @@ struct ScriptParserContext {
145144
}
146145

147146
template<typename I>
148-
bool FromPKBytes(I first, I last, Key& key) const
147+
std::optional<Key> FromPKBytes(I first, I last) const
149148
{
149+
Key key;
150150
key.data.assign(first, last);
151151
key.is_hash = false;
152-
return true;
152+
return key;
153153
}
154154

155155
template<typename I>
156-
bool FromPKHBytes(I first, I last, Key& key) const
156+
std::optional<Key> FromPKHBytes(I first, I last) const
157157
{
158+
Key key;
158159
key.data.assign(first, last);
159160
key.is_hash = true;
160-
return true;
161+
return key;
161162
}
162163
} SCRIPT_PARSER_CONTEXT;
163164

0 commit comments

Comments
 (0)