refactor: make EncodeBase58{Check} consume Spans #19706

pull theStack wants to merge 2 commits into bitcoin:master from theStack:20200810-util-make-base58encode-consume-spans changing 5 files +18 −30

theStack commented at 2:44 PM on August 12, 2020: member

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).
util: make EncodeBase58 consume Spans f0fce0675d
util: make EncodeBase58Check consume Spans 356988e200
fanquake added the label Refactoring on Aug 12, 2020
in src/base58.cpp:93 in 356988e200
```
  91 |      int zeroes = 0;
  92 |      int length = 0;
  93 | -    while (pbegin != pend && *pbegin == 0) {
  94 | -        pbegin++;
  95 | +    while (input.size() > 0 && input[0] == 0) {
  96 | +        input = input.subspan(1);
```
laanwj commented at 3:00 PM on August 12, 2020:

Taking a subspan is a cheap operation (akin to pointer arithmetic), right?

theStack commented at 4:02 PM on August 12, 2020:

Yes, taking a subspan only needs to update pointer and size, which I would still consider cheap. I guess often the size update can be optimized away so that in the end only pointer arithmetic is involved, but I wouldn't feel qualified enough to give any guarantees here. Maybe @sipa can shed a bit more light on this topic.

However, I felt keen to experiment... Here is a godbolt snippet where you can see the difference of "count leading zero-bytes" implementations, on one hand implemented with pointers and on the other hand with spans (using the Bitcoin Core substitute implementation, as I couldn't find a compiler that supported C++20 std::spans yet): https://godbolt.org/z/1oGGPP The generated codes for the loop bodies are basically identical.

laanwj commented at 4:53 PM on August 12, 2020:

Clear, I was just wondering, thank you.
jonatack commented at 4:39 PM on August 12, 2020: member

Concept ACK, light code review ACK
laanwj commented at 4:54 PM on August 12, 2020: member

Code review ACK 356988e200b1debaa80d210d502d2d085c72dc64
practicalswift commented at 3:15 PM on August 14, 2020: contributor

Concept ACK
laanwj merged this on Aug 19, 2020
laanwj closed this on Aug 19, 2020
sidhujag referenced this in commit d2b5dc4fbc on Aug 19, 2020
theStack deleted the branch on Dec 1, 2020
Fabcien referenced this in commit feada73b0a on Sep 15, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19660 refactor: Make HexStr take a span #19687 refactor: make EncodeBase{32,64} consume Spans