refactor: Make HexStr take a span #19660

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2020_06_hexstr_span changing 16 files +50 −70
  1. 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.

  2. laanwj added the label Refactoring on Aug 4, 2020
  3. laanwj added the label Utils/log/libs on Aug 4, 2020
  4. laanwj force-pushed on Aug 4, 2020
  5. laanwj force-pushed on Aug 4, 2020
  6. DrahtBot commented at 5:29 PM on August 4, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  7. DrahtBot cross-referenced this on Aug 4, 2020 from issue p2p: Move all header verification into the network layer, extend logging by troygiorshev
  8. DrahtBot cross-referenced this on Aug 4, 2020 from issue External signer support - Wallet Box edition by Sjors
  9. fanquake requested review from sipa on Aug 4, 2020
  10. 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

  11. jonatack commented at 6:53 AM on August 5, 2020: contributor

    ACK c0bf0d79

  12. 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.

  13. 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)

  14. hebasto commented at 6:27 AM on August 6, 2020: member

    Concept ACK.

  15. 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

  16. 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:

        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…

  17. hebasto approved
  18. hebasto commented at 6:47 AM on August 6, 2020: member

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

  19. promag commented at 8:39 AM on August 6, 2020: member

    Code review ACK c0bf0d79a8ce532ea1cf0a656acba2db2d8f3112.

  20. hebasto approved
  21. hebasto commented at 9:28 AM on August 6, 2020: member

    re-ACK c0c3262274a0767643b0e283aa5a5dcd30015f9b

  22. theStack commented at 4:53 PM on August 6, 2020: contributor

    Concept ACK

  23. refactor: Make HexStr take a span
    Make HexStr take a span of bytes, instead of an awkward pair of
templated iterators.
    0a8aa626dd
  24. laanwj force-pushed on Aug 6, 2020
  25. laanwj commented at 5:42 PM on August 6, 2020: member

    Squashed the fixme commits

  26. hebasto approved
  27. hebasto commented at 6:05 PM on August 6, 2020: member

    re-ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3

  28. 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.

  29. troygiorshev cross-referenced this on Aug 7, 2020 from issue Per-Peer Message Capture by troygiorshev
  30. jonatack commented at 10:40 AM on August 8, 2020: contributor

    re-ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3

  31. elichai commented at 11:20 AM on August 8, 2020: contributor

    Nice! Code review ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3

  32. laanwj merged this on Aug 9, 2020
  33. laanwj closed this on Aug 9, 2020
  34. sidhujag referenced this in commit 0d802185f7 on Aug 9, 2020
  35. theStack cross-referenced this on Aug 12, 2020 from issue refactor: make EncodeBase58{Check} consume Spans by theStack
  36. laanwj referenced this in commit 44ddcd887d on Aug 19, 2020
  37. sidhujag referenced this in commit d2b5dc4fbc on Aug 19, 2020
  38. Sjors cross-referenced this on Aug 31, 2020 from issue wallet: add parent_desc to getaddressinfo by achow101
  39. Fabcien referenced this in commit 3288ce1b22 on Feb 9, 2021
  40. kwvg cross-referenced this on Mar 8, 2021 from issue partial #8149 and merge #19845, #19954: Complete the BIP155 implementation and upgrade to TORv3 by kwvg
  41. kwvg referenced this in commit 478435dbd8 on May 18, 2021
  42. kwvg cross-referenced this on May 19, 2021 from issue Merge #19660, #19373, #19841, #13862, #13866, #17280, #17682 and partial #19326, #14978: Auxiliary Backports by kwvg
  43. kwvg referenced this in commit 5a4e75cdc3 on May 20, 2021
  44. kwvg referenced this in commit dfd2a197d7 on May 20, 2021
  45. kwvg referenced this in commit bc2f11f230 on May 20, 2021
  46. PastaPastaPasta referenced this in commit b76e7fec1f on May 21, 2021
  47. furszy cross-referenced this on Jul 20, 2021 from issue [Refactor] Make HexStr take a span by furszy
  48. random-zebra referenced this in commit 33c2ae83aa on Aug 16, 2021
  49. christiancfifi referenced this in commit 452a33b410 on Aug 25, 2021
  50. christiancfifi referenced this in commit a00dd8142d on Aug 25, 2021
  51. christiancfifi referenced this in commit 32eeeec611 on Aug 25, 2021
  52. christiancfifi referenced this in commit 9d942d72f3 on Aug 25, 2021
  53. christiancfifi referenced this in commit a301e0a629 on Aug 25, 2021
  54. vijaydasmp cross-referenced this on Sep 28, 2021 from issue Merge #14150: Add key origin support to descriptors by vijaydasmp
  55. vijaydasmp referenced this in commit f451701388 on Sep 28, 2021
  56. bitcoin locked this on Feb 15, 2022
Contributors
laanwjDrahtBotjonatackpromaghebastotheStackelichai


sipa

Labels
RefactoringUtils/log/libs
Linked (view graph)
#19136 wallet: add parent_desc to getaddressinfo#19509 Per-Peer Message Capture#19706 refactor: make EncodeBase58{Check} consume Spans
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC