Generalize/simplify VectorReader into SpanReader

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:

0    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:

0    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 💽

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 06a1adf88d51e2771802c6c0f646e3ff16a4baa7 💽
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUibXQwAg72RLZWg263rc7eMLWvExHZ+JJj91IV1IcqQqOd+BlGWIhtwih2/wy5J
 8+fjp98EEgwYPy18TivkjGddaQqL+HCou9PWJNyUV0GPqgd2HDT+KNU9e1T6IDZsS
 99+Ug+m/wsiY8tNLDOnZ6vJK+xFzu2oGQnNLOFxG4VFwh7fR20qvSitLU8Uhaz7et
10+9KBO2XAtHUAeYusDeTc6ZZHPErw/Pvd6p/qt3cBPtGSRMjNCSCUSGaDzXar415L
11AswOh/7NxXs3gETjlGkGIITAZSxHbdGVbiHYA7VTKn69OoYezlOxcgd2RTx+nLEv
12DkjguFkA6oacFVCOhRZGqblUMnxfDm5r7SG9nG1bHpt5XaUJ5354EIS2M1moca48
13gRku2NfErtayNR0xh3ZnkkyzrSvNKVlA3f9SehpuXjwUK7YY0Rd0ynxBcIx/Ii5D
14rp2dhsT+uAH/9lRWVBzmUUqfOwfwpRxAQLhx50JcCap0fpRx6bLrakhMxhbf2XWv
15hJQfoMcI
16=k3Yq
17-----END PGP SIGNATURE-----

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 🖨

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35 🖨
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgcPgv+KnKiX/J5RN0CHNr/DpP/v+1fhK27eioJvsEcH9DXAdY0Y/mntZ0mMOSZ
 8rYAqSHTmDn9uFwwEu9QCffS5VRiyPdi3tb6X/v34uEiAQhOwVPA3o/b0D6fF3t17
 9AkEIM2uSX03yj1PSbpeeTkR8clP2B3Tc5SVcg+fCduDSeQCjAcBP94bIzqQBI18M
1047GpVMBG6ZWLE3yvf/z0mdJq4/wmTRLUonpCQtHFHhtaTHm3FxxmsN7QOX9LwHEi
11oLMGM7qM/Ag09i+AC9mhuSBBi/tlyKGgtKNpmZITqZyT7Kc+bWodCcufbYhxOFl7
12ylFE58QIyPpUbbLTxybu+2anvL/m9ABnXDxxqbiNG0DvqEhtSjZRzcq0nPsIYlMH
13860ewOu0SU4XsMjMJzptEyfz/E5VQQbmIgGGMTg6iBezFuliYruQl09SsxcHJeFx
14eROywgR4zpvLtt0yONmyAU9tfq/2zbjlRJe7IeyMlXP8jOneqavZj7sa0zKJmk5c
15Tvx9RDcp
16=JgBF
17-----END PGP SIGNATURE-----

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

0       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

Generalize/simplify VectorReader into SpanReader #23653

Conflicts