refactor: Make HexStr take a span #19660

laanwj · 2020-08-04T16:53:27Z

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

DrahtBot · 2020-08-04T17:29:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: Move all header verification into the network layer, extend logging #19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
External signer support - Wallet Box edition #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.

jonatack

ACK c0bf0d7

jonatack · 2020-08-05T06:53:07Z

src/uint256.cpp

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


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

Sounds good to me

promag · 2020-08-05T09:36:10Z

src/rpc/request.cpp

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


promag · 2020-08-05T09:40:02Z

src/uint256.cpp

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


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

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 · 2020-08-06T06:27:13Z

Concept ACK.

hebasto

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

hebasto · 2020-08-06T06:46:39Z

src/util/strencodings.h

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


Does static is really needed here?

I don't think it is

hebasto · 2020-08-06T06:47:28Z

src/util/strencodings.cpp

+std::string HexStr(const Span<const uint8_t> s)
+{
+    std::string rv;
+    static const char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',


nit:

Suggested change

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

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

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…

promag

Code review ACK c0bf0d7.

hebasto

re-ACK c0c3262

theStack · 2020-08-06T16:53:31Z

Concept ACK

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

laanwj · 2020-08-06T17:42:20Z

Squashed the fixme commits

hebasto

re-ACK 0a8aa62

laanwj · 2020-08-07T07:39:49Z

src/rpc/util.cpp

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


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

re-ACK 0a8aa62

elichai · 2020-08-08T11:20:13Z

Nice!
Code review ACK 0a8aa62

0a8aa62 refactor: Make HexStr take a span (Wladimir J. van der Laan) Pull request description: Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses. ACKs for top commit: elichai: Code review ACK 0a8aa62 hebasto: re-ACK 0a8aa62 jonatack: re-ACK 0a8aa62 Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992

356988e util: make EncodeBase58Check consume Spans (Sebastian Falbesoner) f0fce06 util: make EncodeBase58 consume Spans (Sebastian Falbesoner) Pull request description: This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs #19660, #19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks). ACKs for top commit: laanwj: Code review ACK 356988e Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512

356988e util: make EncodeBase58Check consume Spans (Sebastian Falbesoner) f0fce06 util: make EncodeBase58 consume Spans (Sebastian Falbesoner) Pull request description: This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs bitcoin#19660, bitcoin#19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks). ACKs for top commit: laanwj: Code review ACK 356988e Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512

Summary: ``` Make HexStr take a span of bytes, instead of an awkward pair of templated iterators. ``` Backport of core [[bitcoin/bitcoin#19660 | PR19660]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9183

Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"

Merge bitcoin#19660, bitcoin#19373, bitcoin#19841, bitcoin#13862, bitcoin#13866, bitcoin#17280, bitcoin#17682 and partial bitcoin#19326, bitcoin#14978: Auxiliary Backports

d4408cd Make HexStr() take a Span (furszy) 4c755ec dead code: Remove dead option in HexStr conversion. (Lenny Maiorani) Pull request description: Follow up to #2470. Only the last two commits are from this work. Make `HexStr` take a span of bytes, instead of an awkward pair of templated iterators. Simplifying most of the uses. Adaptation of bitcoin#19660 and bitcoin#15573. ACKs for top commit: random-zebra: utACK d4408cd Tree-SHA512: 541f80e1af9abea2d662e5fc31fc4d5b4057ff93eefeba5632c6ddca391f3b148c8d819a56802720c3932c3c2afb69ddd1efb4890a59ecf1bf5afd6686cd5a67

Modified some code not backported yet 0a8aa62 refactor: Make HexStr take a span (Wladimir J. van der Laan) Pull request description: Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses. ACKs for top commit: elichai: Code review ACK 0a8aa62 hebasto: re-ACK 0a8aa62 jonatack: re-ACK 0a8aa62 Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992

Partial because it modified some code not backported yet 0a8aa62 refactor: Make HexStr take a span (Wladimir J. van der Laan) Pull request description: Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses. ACKs for top commit: elichai: Code review ACK 0a8aa62 hebasto: re-ACK 0a8aa62 jonatack: re-ACK 0a8aa62 Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992

0a8aa62 refactor: Make HexStr take a span (Wladimir J. van der Laan) Pull request description: Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses. ACKs for top commit: elichai: Code review ACK 0a8aa62 hebasto: re-ACK 0a8aa62 jonatack: re-ACK 0a8aa62 Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992

laanwj added Refactoring Utils/log/libs labels Aug 4, 2020

laanwj force-pushed the 2020_06_hexstr_span branch 2 times, most recently from 55e2632 to c0bf0d7 Compare August 4, 2020 17:25

This was referenced Aug 4, 2020

p2p: Move all header verification into the network layer, extend logging #19107

Merged

External signer support - Wallet Box edition #16546

Merged

fanquake requested a review from sipa August 4, 2020 23:21

jonatack reviewed Aug 5, 2020

View reviewed changes

promag reviewed Aug 5, 2020

View reviewed changes

hebasto approved these changes Aug 6, 2020

View reviewed changes

promag reviewed Aug 6, 2020

View reviewed changes

hebasto approved these changes Aug 6, 2020

View reviewed changes

refactor: Make HexStr take a span

0a8aa62

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

laanwj force-pushed the 2020_06_hexstr_span branch from f0dd45e to 0a8aa62 Compare August 6, 2020 17:42

hebasto approved these changes Aug 6, 2020

View reviewed changes

laanwj commented Aug 7, 2020

View reviewed changes

troygiorshev mentioned this pull request Aug 7, 2020

Per-Peer Message Capture #19509

Merged

jonatack reviewed Aug 8, 2020

View reviewed changes

laanwj merged commit b75f2ad into bitcoin:master Aug 9, 2020

theStack mentioned this pull request Aug 12, 2020

refactor: make EncodeBase58{Check} consume Spans #19706

Merged

Sjors mentioned this pull request Aug 31, 2020

wallet: add parent_desc to getaddressinfo #19136

Merged

kwvg mentioned this pull request Mar 8, 2021

partial #8149 and merge #19845, #19954: Complete the BIP155 implementation and upgrade to TORv3 dashpay/dash#4025

Closed

kwvg added a commit to kwvg/dash that referenced this pull request May 18, 2021

Merge bitcoin#19660: Make HexStr take a span

478435d

kwvg mentioned this pull request May 19, 2021

Merge #19660, #19373, #19841, #13862, #13866, #17280, #17682 and partial #19326, #14978: Auxiliary Backports dashpay/dash#4164

Merged

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Partial bitcoin#19660: Make HexStr take a span

5a4e75c

Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Partial bitcoin#19660: Make HexStr take a span

dfd2a19

Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Partial bitcoin#19660: Make HexStr take a span

bc2f11f

Comment from 6f7b52a: "The fix for CPubKey is a part of `bitcoin#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"

furszy mentioned this pull request Jul 20, 2021

[Refactor] Make HexStr take a span PIVX-Project/PIVX#2482

Merged

vijaydasmp mentioned this pull request Sep 28, 2021

Merge #14150: Add key origin support to descriptors dashpay/dash#4457

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

refactor: Make HexStr take a span #19660

refactor: Make HexStr take a span #19660

Uh oh!

Conversation

laanwj commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 4, 2020

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 6, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Aug 6, 2020

Uh oh!

laanwj commented Aug 6, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

elichai commented Aug 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj commented Aug 4, 2020 •

edited

Loading

laanwj Aug 5, 2020 •

edited

Loading

laanwj Aug 6, 2020 •

edited

Loading

elichai commented Aug 8, 2020 •

edited

Loading