Generalize/simplify VectorReader into SpanReader #23653

pull sipa wants to merge 1 commits into bitcoin:master from sipa:202112_spanreader changing 8 files +35 −36

sipa commented at 4:54 PM on December 2, 2021: member

Originally written for #21590 (safegcd-based MuHash inverses), but then found a better way that removed the need for it, so I'm submitting it independently.
sipa commented at 4:54 PM on December 2, 2021: member

I'm happy to let #23438 (std::byte Serialize) go first and rebase this on top.
laanwj added the label Utils/log/libs on Dec 2, 2021

in src/blockfilter.cpp:84 in 06a1adf88d outdated

  80 | @@ -81,7 +81,7 @@ GCSFilter::GCSFilter(const Params& params)
  81 |  GCSFilter::GCSFilter(const Params& params, std::vector<unsigned char> encoded_filter)
  82 |      : m_params(params), m_encoded(std::move(encoded_filter))
  83 |  {
  84 | -    VectorReader stream(GCS_SER_TYPE, GCS_SER_VERSION, m_encoded, 0);
  85 | +    SpanReader stream(GCS_SER_TYPE, GCS_SER_VERSION, m_encoded, 0);

MarcoFalke commented at 6:36 PM on December 2, 2021:

    SpanReader stream{GCS_SER_TYPE, GCS_SER_VERSION, m_encoded, 0};

nit: Haven't checked if it compiles.

sipa commented at 7:48 PM on December 2, 2021:

Done, everywhere.

in src/streams.h:181 in 06a1adf88d outdated

 176 | @@ -177,8 +177,8 @@ class VectorReader
 177 |      int GetVersion() const { return m_version; }
 178 |      int GetType() const { return m_type; }
 179 |  
 180 | -    size_t size() const { return m_data.size() - m_pos; }
 181 | -    bool empty() const { return m_data.size() == m_pos; }
 182 | +    size_t size() const { return m_data.size(); }
 183 | +    bool empty() const { return m_data.size() == 0; }

MarcoFalke commented at 6:38 PM on December 2, 2021:

    bool empty() const { return m_data.empty; }

sipa commented at 7:47 PM on December 2, 2021:

Done.

MarcoFalke approved

MarcoFalke commented at 6:40 PM on December 2, 2021: member

ACK 06a1adf88d51e2771802c6c0f646e3ff16a4baa7 💽

<details><summary>Show signature</summary>

Signature:

</details>

Generalize/simplify VectorReader into SpanReader 2c35a93b3c
sipa force-pushed on Dec 2, 2021
sipa commented at 7:48 PM on December 2, 2021: member

Addressed @MarcoFalke's comments.
DrahtBot commented at 4:01 AM on December 3, 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)
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.
shaavan approved
shaavan commented at 8:33 AM on December 3, 2021: contributor

ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35

MarcoFalke commented at 9:05 AM on December 3, 2021: member

re-ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35 🖨

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke added the label Refactoring on Dec 3, 2021
MarcoFalke merged this on Dec 3, 2021
MarcoFalke closed this on Dec 3, 2021
sidhujag referenced this in commit 25e1d70230 on Dec 3, 2021
in src/streams.h:154 in 2c35a93b3c
```
 153 | -        if (m_pos > m_data.size()) {
 154 | -            throw std::ios_base::failure("VectorReader(...): end of data (m_pos > m_data.size())");
 155 | +        if (pos > m_data.size()) {
 156 | +            throw std::ios_base::failure("SpanReader(...): end of data (pos > m_data.size())");
 157 |          }
 158 | +        data = data.subspan(pos);
```
achow101 commented at 8:35 PM on December 6, 2021:

Shouldn't this be

m_data = m_data.subspan(pos);
sipa commented at 8:37 PM on December 6, 2021:

Ugh, yes! Will create a PR.

sipa commented at 8:38 PM on December 6, 2021:

Actually, it looks like all call sites use pos=0, which makes this harmless. With spans, having a separate pos is also pointless as it can be easily done by the caller using subspan. I'm just going to drop it.

achow101 commented at 8:39 PM on December 6, 2021:

I need it for PSBT things (which is how I found this bug). It would also be nice to have a test for other pos too.

sipa commented at 8:51 PM on December 6, 2021:

https://github.com/bitcoin/bitcoin/pull/23687
MarcoFalke referenced this in commit b7e63306e8 on Dec 7, 2021
sidhujag referenced this in commit 82cf06c9e2 on Dec 7, 2021
jonatack commented at 5:46 PM on December 7, 2021: member

Post-merge ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35
RandyMcMillan referenced this in commit 6c4c642027 on Dec 23, 2021
RandyMcMillan referenced this in commit 3aa0c2ac7b on Dec 23, 2021
DrahtBot locked this on Dec 7, 2022

Contributors

Labels

Refactoring Utils/log/libs

Linked (view graph)

#21590 Safegcd-based modular inverses in MuHash3072 #23687 Remove unused (and broken) functionality in SpanReader

Generalize/simplify VectorReader into SpanReader #23653

Conflicts