std::vector<std::byte> for BlockManager::ReadRawBlock()
#32743
Following this comment, this PR changes BlockManager::ReadRawBlock() to accept a std::vector<std::byte> instead of std::vector<uint8_t>, in order to avoid casts during its invocations.
It also adds a new SpanReader constructor to allow reading from a span of std::bytes (in addition to span of uint8_t).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32743.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | l0rinc, TheCharlatan, maflcko |
| Stale ACK | hebasto, BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
lgtm ACK ddaa27e83dfa13e961a4be3293fb647ed0fe3a1d
This changes the underlying type to std::byte to get rid of casts in calling code and also allows SpanReader to deal with std::byte now.
1036@@ -1037,7 +1037,7 @@ bool BlockManager::ReadBlock(CBlock& block, const CBlockIndex& index) const
1037 return ReadBlock(block, block_pos, index.GetBlockHash());
1038 }
1039
1040-bool BlockManager::ReadRawBlock(std::vector<uint8_t>& block, const FlatFilePos& pos) const
1041+bool BlockManager::ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos& pos) const
It seems to me we don’t need filein.read(MakeWritableByteSpan(block)); after this change anymore:
0 filein.read(block);
106@@ -107,6 +107,7 @@ class SpanReader
107 * @param[in] data Referenced byte vector to overwrite/append
108 */
109 explicit SpanReader(std::span<const unsigned char> data) : m_data{data} {}
110+ explicit SpanReader(std::span<const std::byte> data) : m_data{MakeUCharSpan(data)} {}
👍 for adding an alternative constructor - this could be a good opportunity to migrate the internals of SpanReader to std::byte as well, given that read already copies std::byte into m_data:
0 std::span<const std::byte> m_data;
1
2public:
3 /**
4 * [@param](/bitcoin-bitcoin/contributor/param/)[in] data Referenced byte vector to overwrite/append
5 */
6 explicit SpanReader(const std::span<const unsigned char> data) : m_data{std::as_bytes(data)} {}
7 explicit SpanReader(const std::span<const std::byte> data) : m_data{data} {}
Concept ACK, good to get rid of a few more unsigned-char-to-byte conversions! Could you please provide a more descriptive PR description?
I also have a few non-blocking suggestions I’d like you to consider:
0diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
1index 3fc1e0fb16..5816baa20e 100644
2--- a/src/node/blockstorage.cpp
3+++ b/src/node/blockstorage.cpp
4@@ -1071,7 +1071,7 @@ bool BlockManager::ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos
5 }
6
7 block.resize(blk_size); // Zeroing of memory is intentional here
8- filein.read(MakeWritableByteSpan(block));
9+ filein.read(block);
10 } catch (const std::exception& e) {
11 LogError("Read from block file failed: %s for %s while reading raw block", e.what(), pos.ToString());
12 return false;
13diff --git a/src/streams.h b/src/streams.h
14index 682f4d4b33..8dde5a9af9 100644
15--- a/src/streams.h
16+++ b/src/streams.h
17@@ -99,15 +99,14 @@ private:
18 */
19 class SpanReader
20 {
21-private:
22- std::span<const unsigned char> m_data;
23+ std::span<const std::byte> m_data;
24
25 public:
26 /**
27 * [@param](/bitcoin-bitcoin/contributor/param/)[in] data Referenced byte vector to overwrite/append
28 */
29- explicit SpanReader(std::span<const unsigned char> data) : m_data{data} {}
30- explicit SpanReader(std::span<const std::byte> data) : m_data{MakeUCharSpan(data)} {}
31+ explicit SpanReader(const std::span<const unsigned char> data) : m_data{std::as_bytes(data)} {}
32+ explicit SpanReader(const std::span<const std::byte> data) : m_data{data} {}
33
34 template<typename T>
35 SpanReader& operator>>(T&& obj)
std::vector<std::byte> instead of std::vector<uint8_t> for BlockManager::ReadRawBlock()