refactor: Make HexStr take a span

laanwj commented at 4:53 pm on August 4, 2020: member

Make HexStr take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses.

laanwj added the label Refactoring on Aug 4, 2020

laanwj added the label Utils/log/libs on Aug 4, 2020

laanwj force-pushed on Aug 4, 2020

DrahtBot commented at 5:29 pm on August 4, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
#16546 (External signer support - Wallet Box edition by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake requested review from sipa on Aug 4, 2020

in src/uint256.cpp:23 in c0bf0d79a8 outdated

18@@ -19,7 +19,11 @@ base_blob<BITS>::base_blob(const std::vector<unsigned char>& vch)
19 template <unsigned int BITS>
20 std::string base_blob<BITS>::GetHex() const
21 {
22-    return HexStr(std::reverse_iterator<const uint8_t*>(m_data + sizeof(m_data)), std::reverse_iterator<const uint8_t*>(m_data));
23+    uint8_t m_data_rev[WIDTH];
24+    for (int i = 0; i < WIDTH; i++) {

jonatack commented at 6:53 am on August 5, 2020:

nit: Perhaps use the pre-increment (++i) operator.

laanwj commented at 9:42 am on August 5, 2020:

Sounds good to me

jonatack commented at 6:53 am on August 5, 2020: member

ACK c0bf0d79

in src/rpc/request.cpp:81 in c0bf0d79a8 outdated

77@@ -78,7 +78,7 @@ bool GenerateAuthCookie(std::string *cookie_out)
78     const size_t COOKIE_SIZE = 32;
79     unsigned char rand_pwd[COOKIE_SIZE];
80     GetRandBytes(rand_pwd, COOKIE_SIZE);
81-    std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd, rand_pwd+COOKIE_SIZE);
82+    std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);

promag commented at 9:36 am on August 5, 2020:

Nice.

in src/uint256.cpp:22 in c0bf0d79a8 outdated

18@@ -19,7 +19,11 @@ base_blob<BITS>::base_blob(const std::vector<unsigned char>& vch)
19 template <unsigned int BITS>
20 std::string base_blob<BITS>::GetHex() const
21 {
22-    return HexStr(std::reverse_iterator<const uint8_t*>(m_data + sizeof(m_data)), std::reverse_iterator<const uint8_t*>(m_data));
23+    uint8_t m_data_rev[WIDTH];

promag commented at 9:40 am on August 5, 2020:

Correct me if I’m wrong, but isn’t this duplicating memory usage?

laanwj commented at 10:41 am on August 5, 2020:

Not really: is is a local variable, all this does is create a small (32 bytes for uint256) temporary buffer on the stack. (I’ve opted to do this instead of creating a vector from the iterators, which would be a heap allocation)

hebasto commented at 6:27 am on August 6, 2020: member

Concept ACK.

in src/util/strencodings.h:127 in c0bf0d79a8 outdated

143-}
144+/**
145+ * Convert a span of bytes to a lower-case hexadecimal string.
146+ */
147+std::string HexStr(const Span<const uint8_t> s);
148+static inline std::string HexStr(const Span<const char> s) { return HexStr(MakeUCharSpan(s)); }

hebasto commented at 6:46 am on August 6, 2020:

Does static is really needed here?

laanwj commented at 9:22 am on August 6, 2020:

I don’t think it is

in src/util/strencodings.cpp:576 in c0bf0d79a8 outdated

568@@ -569,3 +569,16 @@ std::string Capitalize(std::string str)
569     str[0] = ToUpper(str.front());
570     return str;
571 }
572+
573+std::string HexStr(const Span<const uint8_t> s)
574+{
575+    std::string rv;
576+    static const char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',

hebasto commented at 6:47 am on August 6, 2020:

nit:

0    static constexpr char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',

laanwj commented at 9:23 am on August 6, 2020:

I’m not sure. Does this make a difference for an array?

Edit: well it works so I don’t think there any hurt in changing it…

hebasto approved

hebasto commented at 6:47 am on August 6, 2020: member

ACK c0bf0d79a8ce532ea1cf0a656acba2db2d8f3112, tested on Linux Mint 20 (x86_64).

promag commented at 8:39 am on August 6, 2020: member

Code review ACK c0bf0d79a8ce532ea1cf0a656acba2db2d8f3112.

hebasto approved

hebasto commented at 9:28 am on August 6, 2020: member

re-ACK c0c3262274a0767643b0e283aa5a5dcd30015f9b

theStack commented at 4:53 pm on August 6, 2020: member

Concept ACK

refactor: Make HexStr take a span

Make HexStr take a span of bytes, instead of an awkward pair of
templated iterators.

0a8aa626dd

laanwj force-pushed on Aug 6, 2020

laanwj commented at 5:42 pm on August 6, 2020: member

Squashed the fixme commits

hebasto approved

hebasto commented at 6:05 pm on August 6, 2020: member

re-ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3

in src/rpc/util.cpp:263 in 0a8aa626dd

259@@ -260,7 +260,7 @@ class DescribeAddressVisitor : public boost::static_visitor<UniValue>
260         UniValue obj(UniValue::VOBJ);
261         obj.pushKV("iswitness", true);
262         obj.pushKV("witness_version", (int)id.version);
263-        obj.pushKV("witness_program", HexStr(id.program, id.program + id.length));
264+        obj.pushKV("witness_program", HexStr(Span<const unsigned char>(id.program, id.length)));

laanwj commented at 7:39 am on August 7, 2020:

Maybe we want to give WitnessUnknown .data() and .size() fields to avoid having to do this explicitly. Don’t know. Probably not in thie PR though.

jonatack commented at 10:40 am on August 8, 2020: member

re-ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3

elichai commented at 11:20 am on August 8, 2020: contributor

Nice! Code review ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3

laanwj merged this on Aug 9, 2020

laanwj closed this on Aug 9, 2020

sidhujag referenced this in commit 0d802185f7 on Aug 9, 2020

laanwj referenced this in commit 44ddcd887d on Aug 19, 2020

sidhujag referenced this in commit d2b5dc4fbc on Aug 19, 2020

Fabcien referenced this in commit 3288ce1b22 on Feb 9, 2021

kittywhiskers referenced this in commit 478435dbd8 on May 18, 2021

kittywhiskers referenced this in commit 5a4e75cdc3 on May 20, 2021

kittywhiskers referenced this in commit dfd2a197d7 on May 20, 2021

kittywhiskers referenced this in commit bc2f11f230 on May 20, 2021

PastaPastaPasta referenced this in commit b76e7fec1f on May 21, 2021

random-zebra referenced this in commit 33c2ae83aa on Aug 16, 2021

christiancfifi referenced this in commit 452a33b410 on Aug 25, 2021

christiancfifi referenced this in commit a00dd8142d on Aug 25, 2021

christiancfifi referenced this in commit 32eeeec611 on Aug 25, 2021

christiancfifi referenced this in commit 9d942d72f3 on Aug 25, 2021

christiancfifi referenced this in commit a301e0a629 on Aug 25, 2021

vijaydasmp referenced this in commit f451701388 on Sep 28, 2021

DrahtBot locked this on Feb 15, 2022

refactor: Make HexStr take a span #19660

Conflicts