span: Add std::byte helpers #23451

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2111-refSpan changing 8 files +67 −24

MarcoFalke commented at 6:05 PM on November 6, 2021: member

This adds (currently unused) span std::byte helpers, so that they can be used in new code.

The refactors are also required for #23438, but they are split up because the other pull doesn't compile with msvc right now.

The third commit is not needed for the other pull, but still nice.
MarcoFalke added the label Refactoring on Nov 6, 2021
DrahtBot commented at 12:30 AM on November 7, 2021: 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:
- #23438 (refactor: Use spans of std::byte in serialize by MarcoFalke)
- #23413 (Replace MakeSpan helper with Span deduction guide by sipa)
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.
laanwj commented at 11:18 AM on November 8, 2021: member

This adds (currently unused) span std::byte helpers, so that they can be used in new code.

Let's make sure they are at least all used in tests in some way (I haven't checked if this is the case).
klementtan approved
klementtan commented at 7:13 PM on November 8, 2021: contributor

Code review and tested ACK fae93c533d7364e7c0daa8f6646d1ea5bd02192e

Tested by writing an Adhoc unit test https://github.com/klementtan/bitcoin/commit/129b1e036d192e70bf2677b4521fc4de7b21589c (feel free to copy the test into this PR)

Use value_type in CDataStream where possible

Also, simplify unit tests with the CDataStream::str method.

fabe18d0b3

refactor: Use ignore helper when unserializing an invalid pubkey fa18038f51

span: Add std::byte helpers

Also, add Span<std::byte> interface to strencondings.

faa3ec2304

MarcoFalke force-pushed on Nov 9, 2021
MarcoFalke commented at 4:47 PM on November 9, 2021: member
Thanks for the tests. However, I think the line
```
   BOOST_CHECK_EQUAL(std::to_integer<int>(byte_span_char[6]), '\0');
```
is undefined behavior. The size of the span should be 6, so at most it is allowed to read [5]. Also, the uint32_t tests will fail on different endian hardware?

I've pushed a commit with my own tests for all newly added functions.
klementtan commented at 5:43 PM on November 9, 2021: contributor

reACK faa3ec2. Verified that all the new std::byte helper functions are tested. @MarcoFalke Yup you're right, [6] would lead UB (it would have thrown assertion error if I compiled with--enable-debug) and the uint32_t tests would only work on little-endian machine. My bad! 😅
in src/streams.h:281 in faa3ec2304
```
 277 | @@ -278,7 +278,7 @@ class CDataStream
 278 |              vch.insert(it, first, last);
 279 |      }
 280 |  
 281 | -    void insert(iterator it, const char* first, const char* last)
 282 | +    void insert(iterator it, const value_type* first, const value_type* last)
```
laanwj commented at 11:32 AM on November 10, 2021:

I'm slightly surprised that this doesn't give an error anywhere. value_type was uint8_t before, not char.

MarcoFalke commented at 11:51 AM on November 10, 2021:

This interface is not used in our code. It was added to have full compat with the std::vector interface. No opinion on whether that is a good goal.

laanwj commented at 6:31 PM on November 10, 2021:

I think that's a reasonably valid goal. Though we should at least test the function. Not in this PR though.
in src/pubkey.h:152 in fa18038f51 outdated
```
 151 |          } else {
 152 |              // invalid pubkey, skip available data
 153 | -            char dummy;
 154 | -            while (len--)
 155 | -                s.read(&dummy, 1);
 156 | +            s.ignore(len);
```
laanwj commented at 11:38 AM on November 10, 2021:

This is the most tricky change to review I think; whether the behavior here stays the same in all cases, also in edge cases at the end of the stream.
MarcoFalke commented at 11:56 AM on November 10, 2021:

The only change should be in the error message and a missing call to memcpy.

-CDataStream::read(): end of data +CDataStream::ignore(): end of data

The commit is unrelated and not needed for the changes here, so I am happy to remove. If it will be removed, I'd later on need to replace char dummy; with std::byte dummy;.
laanwj commented at 6:32 PM on November 10, 2021:

I'm fine with keeping it. I personally think the change is correct, just mentioned it as a point of focus for reviewers.
laanwj commented at 12:52 PM on November 16, 2021: member

Code review ACK faa3ec2304051be7cfbe301cfbfbda3faf7514fc
MarcoFalke merged this on Nov 24, 2021
MarcoFalke closed this on Nov 24, 2021
MarcoFalke deleted the branch on Nov 24, 2021
sidhujag referenced this in commit b8e78e1f0b on Nov 24, 2021
sidhujag referenced this in commit 1e01bd1d92 on Nov 24, 2021
Bigboy1982 commented at 8:43 AM on May 12, 2022: none

Good
MarcoFalke locked this on May 12, 2022

Contributors

Labels

Linked (view graph)

#23438 refactor: Use spans of std::byte in serialize

span: Add std::byte helpers #23451

Conflicts