Skip to content

Commit 132d5f8

Browse files
laanwjlaanwj
committed
Merge bitcoin/bitcoin#25001: Modernize util/strencodings and util/string: string_view and optional
fa7078d scripted-diff: Rename ValidAsCString to ContainsNoNUL (MacroFake) e7d2fbd Use std::string_view throughout util strencodings/string (Pieter Wuille) 8ffbd14 Make DecodeBase{32,64} take string_view arguments (Pieter Wuille) 1a72d62 Generalize ConvertBits to permit transforming the input (Pieter Wuille) 78f3ac5 Make DecodeBase{32,64} return optional instead of taking bool* (Pieter Wuille) a65931e Make DecodeBase{32,64} always return vector, not string (Pieter Wuille) a4377a0 Reject incorrect base64 in HTTP auth (Pieter Wuille) d648b51 Make SanitizeString use string_view (Pieter Wuille) 963bc9b Make IsHexNumber use string_view (Pieter Wuille) 4006299 Make IsHex use string_view (Pieter Wuille) c1d165a Make ParseHex use string_view (Pieter Wuille) Pull request description: Make use of `std::string_view` and `std::optional` in the util/{strencodings, string} files. This avoids many temporary string/vector objects being created, while making the interface easier to read. Changes include: * Make all input arguments in functions in util/strencodings and util/string take `std::string_view` instead of `std::string`. * Add `RemovePrefixView` and `TrimStringView` which also *return* `std::string_view` objects (the corresponding `RemovePrefix` and `TrimString` keep returning an `std::string`, as that's needed in many call sites still). * Stop returning `std::string` from `DecodeBase32` and `DecodeBase64`, but return vectors. Base32/64 are fundamentally algorithms for encoding bytes as strings; returning `std::string` from those (especially doing it conditionally based on the input arguments/types) is just bizarre. * Stop taking a `bool* pf_invalid` output argument pointer in `DecodeBase32` and `DecodeBase64`; return an `std::optional` instead. * Make `DecodeBase32` and `DecodeBase64` more efficient by doing the conversion from characters to integer symbols on-the-fly rather than through a temporary vector. ACKs for top commit: MarcoFalke: re-ACK fa7078d only change is rebase and adding a scripted-diff 🍲 martinus: Code review ACK fa7078d, found no issue laanwj: Code review ACK fa7078d sipa: utACK fa7078d (as far as the commit that isn't mine goes) Tree-SHA512: 5cf02e541caef0bcd100466747664bdb828a68a05dae568cbcd0632a53dd3a4c4e85cd8c48ebbd168d4247d5c9666689c16005f1c8ad75b0f057d8683931f664
2 parents 0b8e286 + fa7078d commit 132d5f8

22 files changed

+209
-253
lines changed

‎src/base58.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ std::string EncodeBase58(Span<const unsigned char> input)
126126

127127
bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret_len)
128128
{
129-
if (!ValidAsCString(str)) {
129+
if (!ContainsNoNUL(str)) {
130130
return false;
131131
}
132132
return DecodeBase58(str.c_str(), vchRet, max_ret_len);
@@ -160,7 +160,7 @@ std::string EncodeBase58Check(Span<const unsigned char> input)
160160

161161
bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret)
162162
{
163-
if (!ValidAsCString(str)) {
163+
if (!ContainsNoNUL(str)) {
164164
return false;
165165
}
166166
return DecodeBase58Check(str.c_str(), vchRet, max_ret);

‎src/bitcoin-tx.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInId
240240
template <typename T>
241241
static T TrimAndParse(const std::string& int_str, const std::string& err)
242242
{
243-
const auto parsed{ToIntegral<T>(TrimString(int_str))};
243+
const auto parsed{ToIntegral<T>(TrimStringView(int_str))};
244244
if (!parsed.has_value()) {
245245
throw std::runtime_error(err + " '" + int_str + "'");
246246
}

‎src/httprpc.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,11 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
131131
return false;
132132
if (strAuth.substr(0, 6) != "Basic ")
133133
return false;
134-
std::string strUserPass64 = TrimString(strAuth.substr(6));
135-
std::string strUserPass = DecodeBase64(strUserPass64);
134+
std::string_view strUserPass64 = TrimStringView(std::string_view{strAuth}.substr(6));
135+
auto userpass_data = DecodeBase64(strUserPass64);
136+
std::string strUserPass;
137+
if (!userpass_data) return false;
138+
strUserPass.assign(userpass_data->begin(), userpass_data->end());
136139

137140
if (strUserPass.find(':') != std::string::npos)
138141
strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':'));

‎src/i2p.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,11 @@ static std::string SwapBase64(const std::string& from)
6969
static Binary DecodeI2PBase64(const std::string& i2p_b64)
7070
{
7171
const std::string& std_b64 = SwapBase64(i2p_b64);
72-
bool invalid;
73-
Binary decoded = DecodeBase64(std_b64.c_str(), &invalid);
74-
if (invalid) {
72+
auto decoded = DecodeBase64(std_b64);
73+
if (!decoded) {
7574
throw std::runtime_error(strprintf("Cannot decode Base64: \"%s\"", i2p_b64));
7675
}
77-
return decoded;
76+
return std::move(*decoded);
7877
}
7978

8079
/**

‎src/netaddress.cpp

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ static void Checksum(Span<const uint8_t> addr_pubkey, uint8_t (&checksum)[CHECKS
210210

211211
bool CNetAddr::SetSpecial(const std::string& addr)
212212
{
213-
if (!ValidAsCString(addr)) {
213+
if (!ContainsNoNUL(addr)) {
214214
return false;
215215
}
216216

@@ -234,17 +234,16 @@ bool CNetAddr::SetTor(const std::string& addr)
234234
return false;
235235
}
236236

237-
bool invalid;
238-
const auto& input = DecodeBase32(addr.substr(0, addr.size() - suffix_len).c_str(), &invalid);
237+
auto input = DecodeBase32(std::string_view{addr}.substr(0, addr.size() - suffix_len));
239238

240-
if (invalid) {
239+
if (!input) {
241240
return false;
242241
}
243242

244-
if (input.size() == torv3::TOTAL_LEN) {
245-
Span<const uint8_t> input_pubkey{input.data(), ADDR_TORV3_SIZE};
246-
Span<const uint8_t> input_checksum{input.data() + ADDR_TORV3_SIZE, torv3::CHECKSUM_LEN};
247-
Span<const uint8_t> input_version{input.data() + ADDR_TORV3_SIZE + torv3::CHECKSUM_LEN, sizeof(torv3::VERSION)};
243+
if (input->size() == torv3::TOTAL_LEN) {
244+
Span<const uint8_t> input_pubkey{input->data(), ADDR_TORV3_SIZE};
245+
Span<const uint8_t> input_checksum{input->data() + ADDR_TORV3_SIZE, torv3::CHECKSUM_LEN};
246+
Span<const uint8_t> input_version{input->data() + ADDR_TORV3_SIZE + torv3::CHECKSUM_LEN, sizeof(torv3::VERSION)};
248247

249248
if (input_version != torv3::VERSION) {
250249
return false;
@@ -280,15 +279,14 @@ bool CNetAddr::SetI2P(const std::string& addr)
280279
// can decode it.
281280
const std::string b32_padded = addr.substr(0, b32_len) + "====";
282281

283-
bool invalid;
284-
const auto& address_bytes = DecodeBase32(b32_padded.c_str(), &invalid);
282+
auto address_bytes = DecodeBase32(b32_padded);
285283

286-
if (invalid || address_bytes.size() != ADDR_I2P_SIZE) {
284+
if (!address_bytes || address_bytes->size() != ADDR_I2P_SIZE) {
287285
return false;
288286
}
289287

290288
m_net = NET_I2P;
291-
m_addr.assign(address_bytes.begin(), address_bytes.end());
289+
m_addr.assign(address_bytes->begin(), address_bytes->end());
292290

293291
return true;
294292
}

‎src/netbase.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ static bool LookupIntern(const std::string& name, std::vector<CNetAddr>& vIP, un
136136
{
137137
vIP.clear();
138138

139-
if (!ValidAsCString(name)) {
139+
if (!ContainsNoNUL(name)) {
140140
return false;
141141
}
142142

@@ -169,7 +169,7 @@ static bool LookupIntern(const std::string& name, std::vector<CNetAddr>& vIP, un
169169

170170
bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup, DNSLookupFn dns_lookup_function)
171171
{
172-
if (!ValidAsCString(name)) {
172+
if (!ContainsNoNUL(name)) {
173173
return false;
174174
}
175175
std::string strHost = name;
@@ -184,7 +184,7 @@ bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned in
184184

185185
bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup, DNSLookupFn dns_lookup_function)
186186
{
187-
if (!ValidAsCString(name)) {
187+
if (!ContainsNoNUL(name)) {
188188
return false;
189189
}
190190
std::vector<CNetAddr> vIP;
@@ -197,7 +197,7 @@ bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup, DNSL
197197

198198
bool Lookup(const std::string& name, std::vector<CService>& vAddr, uint16_t portDefault, bool fAllowLookup, unsigned int nMaxSolutions, DNSLookupFn dns_lookup_function)
199199
{
200-
if (name.empty() || !ValidAsCString(name)) {
200+
if (name.empty() || !ContainsNoNUL(name)) {
201201
return false;
202202
}
203203
uint16_t port{portDefault};
@@ -216,7 +216,7 @@ bool Lookup(const std::string& name, std::vector<CService>& vAddr, uint16_t port
216216

217217
bool Lookup(const std::string& name, CService& addr, uint16_t portDefault, bool fAllowLookup, DNSLookupFn dns_lookup_function)
218218
{
219-
if (!ValidAsCString(name)) {
219+
if (!ContainsNoNUL(name)) {
220220
return false;
221221
}
222222
std::vector<CService> vService;
@@ -229,7 +229,7 @@ bool Lookup(const std::string& name, CService& addr, uint16_t portDefault, bool
229229

230230
CService LookupNumeric(const std::string& name, uint16_t portDefault, DNSLookupFn dns_lookup_function)
231231
{
232-
if (!ValidAsCString(name)) {
232+
if (!ContainsNoNUL(name)) {
233233
return {};
234234
}
235235
CService addr;
@@ -684,7 +684,7 @@ bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_
684684

685685
bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
686686
{
687-
if (!ValidAsCString(subnet_str)) {
687+
if (!ContainsNoNUL(subnet_str)) {
688688
return false;
689689
}
690690

‎src/psbt.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -388,18 +388,17 @@ std::string PSBTRoleName(PSBTRole role) {
388388

389389
bool DecodeBase64PSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error)
390390
{
391-
bool invalid;
392-
std::string tx_data = DecodeBase64(base64_tx, &invalid);
393-
if (invalid) {
391+
auto tx_data = DecodeBase64(base64_tx);
392+
if (!tx_data) {
394393
error = "invalid base64";
395394
return false;
396395
}
397-
return DecodeRawPSBT(psbt, tx_data, error);
396+
return DecodeRawPSBT(psbt, MakeByteSpan(*tx_data), error);
398397
}
399398

400-
bool DecodeRawPSBT(PartiallySignedTransaction& psbt, const std::string& tx_data, std::string& error)
399+
bool DecodeRawPSBT(PartiallySignedTransaction& psbt, Span<const std::byte> tx_data, std::string& error)
401400
{
402-
CDataStream ss_data(MakeByteSpan(tx_data), SER_NETWORK, PROTOCOL_VERSION);
401+
CDataStream ss_data(tx_data, SER_NETWORK, PROTOCOL_VERSION);
403402
try {
404403
ss_data >> psbt;
405404
if (!ss_data.empty()) {

‎src/psbt.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -988,6 +988,6 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti
988988
//! Decode a base64ed PSBT into a PartiallySignedTransaction
989989
[[nodiscard]] bool DecodeBase64PSBT(PartiallySignedTransaction& decoded_psbt, const std::string& base64_psbt, std::string& error);
990990
//! Decode a raw (binary blob) PSBT into a PartiallySignedTransaction
991-
[[nodiscard]] bool DecodeRawPSBT(PartiallySignedTransaction& decoded_psbt, const std::string& raw_psbt, std::string& error);
991+
[[nodiscard]] bool DecodeRawPSBT(PartiallySignedTransaction& decoded_psbt, Span<const std::byte> raw_psbt, std::string& error);
992992

993993
#endif // BITCOIN_PSBT_H

‎src/qt/walletframe.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -194,16 +194,16 @@ void WalletFrame::gotoVerifyMessageTab(QString addr)
194194

195195
void WalletFrame::gotoLoadPSBT(bool from_clipboard)
196196
{
197-
std::string data;
197+
std::vector<unsigned char> data;
198198

199199
if (from_clipboard) {
200200
std::string raw = QApplication::clipboard()->text().toStdString();
201-
bool invalid;
202-
data = DecodeBase64(raw, &invalid);
203-
if (invalid) {
201+
auto result = DecodeBase64(raw);
202+
if (!result) {
204203
Q_EMIT message(tr("Error"), tr("Unable to decode PSBT from clipboard (invalid base64)"), CClientUIInterface::MSG_ERROR);
205204
return;
206205
}
206+
data = std::move(*result);
207207
} else {
208208
QString filename = GUIUtil::getOpenFileName(this,
209209
tr("Load Transaction Data"), QString(),
@@ -214,12 +214,12 @@ void WalletFrame::gotoLoadPSBT(bool from_clipboard)
214214
return;
215215
}
216216
std::ifstream in{filename.toLocal8Bit().data(), std::ios::binary};
217-
data = std::string(std::istreambuf_iterator<char>{in}, {});
217+
data.assign(std::istream_iterator<unsigned char>{in}, {});
218218
}
219219

220220
std::string error;
221221
PartiallySignedTransaction psbtx;
222-
if (!DecodeRawPSBT(psbtx, data, error)) {
222+
if (!DecodeRawPSBT(psbtx, MakeByteSpan(data), error)) {
223223
Q_EMIT message(tr("Error"), tr("Unable to decode PSBT") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR);
224224
return;
225225
}

‎src/test/base32_tests.cpp

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,20 +22,16 @@ BOOST_AUTO_TEST_CASE(base32_testvectors)
2222
BOOST_CHECK_EQUAL(strEnc, vstrOut[i]);
2323
strEnc = EncodeBase32(vstrIn[i], false);
2424
BOOST_CHECK_EQUAL(strEnc, vstrOutNoPadding[i]);
25-
std::string strDec = DecodeBase32(vstrOut[i]);
26-
BOOST_CHECK_EQUAL(strDec, vstrIn[i]);
25+
auto dec = DecodeBase32(vstrOut[i]);
26+
BOOST_REQUIRE(dec);
27+
BOOST_CHECK_MESSAGE(MakeByteSpan(*dec) == MakeByteSpan(vstrIn[i]), vstrOut[i]);
2728
}
2829

2930
// Decoding strings with embedded NUL characters should fail
30-
bool failure;
31-
(void)DecodeBase32("invalid\0"s, &failure); // correct size, invalid due to \0
32-
BOOST_CHECK(failure);
33-
(void)DecodeBase32("AWSX3VPP"s, &failure); // valid
34-
BOOST_CHECK(!failure);
35-
(void)DecodeBase32("AWSX3VPP\0invalid"s, &failure); // correct size, invalid due to \0
36-
BOOST_CHECK(failure);
37-
(void)DecodeBase32("AWSX3VPPinvalid"s, &failure); // invalid size
38-
BOOST_CHECK(failure);
31+
BOOST_CHECK(!DecodeBase32("invalid\0"s)); // correct size, invalid due to \0
32+
BOOST_CHECK(DecodeBase32("AWSX3VPP"s)); // valid
33+
BOOST_CHECK(!DecodeBase32("AWSX3VPP\0invalid"s)); // correct size, invalid due to \0
34+
BOOST_CHECK(!DecodeBase32("AWSX3VPPinvalid"s)); // invalid size
3935
}
4036

4137
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)