Generalize/simplify VectorReader into SpanReader #23653
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm happy to let #23438 ( |
maflcko
approved these changes
Dec 2, 2021
There was a problem hiding this comment.
ACK 06a1adf 💽
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 06a1adf88d51e2771802c6c0f646e3ff16a4baa7 💽
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUibXQwAg72RLZWg263rc7eMLWvExHZ+JJj91IV1IcqQqOd+BlGWIhtwih2/wy5J
+fjp98EEgwYPy18TivkjGddaQqL+HCou9PWJNyUV0GPqgd2HDT+KNU9e1T6IDZsS
9+Ug+m/wsiY8tNLDOnZ6vJK+xFzu2oGQnNLOFxG4VFwh7fR20qvSitLU8Uhaz7et
+9KBO2XAtHUAeYusDeTc6ZZHPErw/Pvd6p/qt3cBPtGSRMjNCSCUSGaDzXar415L
AswOh/7NxXs3gETjlGkGIITAZSxHbdGVbiHYA7VTKn69OoYezlOxcgd2RTx+nLEv
DkjguFkA6oacFVCOhRZGqblUMnxfDm5r7SG9nG1bHpt5XaUJ5354EIS2M1moca48
gRku2NfErtayNR0xh3ZnkkyzrSvNKVlA3f9SehpuXjwUK7YY0Rd0ynxBcIx/Ii5D
rp2dhsT+uAH/9lRWVBzmUUqfOwfwpRxAQLhx50JcCap0fpRx6bLrakhMxhbf2XWv
hJQfoMcI
=k3Yq
-----END PGP SIGNATURE-----
06a1adf to
2c35a93
Compare
sipa
commented
Dec 2, 2021
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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 these changes
Dec 3, 2021
|
re-ACK 2c35a93 🖨 Show signatureSignature: |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 3, 2021
2c35a93 Generalize/simplify VectorReader into SpanReader (Pieter Wuille) Pull request description: Originally written for bitcoin#21590 (safegcd-based MuHash inverses), but then found a better way that removed the need for it, so I'm submitting it independently. ACKs for top commit: MarcoFalke: re-ACK 2c35a93 🖨 shaavan: ACK 2c35a93 Tree-SHA512: 959e3251e0cfe20e13a50639b617c9dc2a561d613a0884d983c93d15dacb6d2305d760aa933d18ba055cef8a1651a344bcb6b3f93051ecf26d3f2efc5779efa4
achow101
reviewed
Dec 6, 2021
| } | ||
| data = data.subspan(pos); |
There was a problem hiding this comment.
Shouldn't this be
Suggested change
| data = data.subspan(pos); | |
| m_data = m_data.subspan(pos); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Dec 7, 2021
… in SpanReader 31ba1af Remove unused (and broken) functionality in SpanReader (Pieter Wuille) Pull request description: This removes the ability to set an offset in the `SpanReader::SpanReader` constructor, as the current code is broken since #23653. All call sites use `pos=0`, so it is actually unused. If future call sites need it, `SpanReader{a, b, c, d}` is equivalent to `SpanReader{a, b, c.subspan(d)}`. It also removes the ability to deserialize from `SpanReader` directly from the constructor. This too is unused, and can be more idiomatically simulated using `(SpanReader{a, b, c} >> x >> y >> z)` instead of `SpanReader{a, b, c, x, y, z}`. This was pointed out by achow101 in bitcoin/bitcoin#23653 (comment). ACKs for top commit: jb55: crACK 31ba1af achow101: ACK 31ba1af Tree-SHA512: 700ebcd74147628488c39168dbf3a00f8ed41709a26711695f4bf036250a9b115574923bbf96040ec7b7fee4132d6dbbcb5c6e5a2977c4beb521dc1500e6ed53
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 7, 2021
…Reader 31ba1af Remove unused (and broken) functionality in SpanReader (Pieter Wuille) Pull request description: This removes the ability to set an offset in the `SpanReader::SpanReader` constructor, as the current code is broken since bitcoin#23653. All call sites use `pos=0`, so it is actually unused. If future call sites need it, `SpanReader{a, b, c, d}` is equivalent to `SpanReader{a, b, c.subspan(d)}`. It also removes the ability to deserialize from `SpanReader` directly from the constructor. This too is unused, and can be more idiomatically simulated using `(SpanReader{a, b, c} >> x >> y >> z)` instead of `SpanReader{a, b, c, x, y, z}`. This was pointed out by achow101 in bitcoin#23653 (comment). ACKs for top commit: jb55: crACK 31ba1af achow101: ACK 31ba1af Tree-SHA512: 700ebcd74147628488c39168dbf3a00f8ed41709a26711695f4bf036250a9b115574923bbf96040ec7b7fee4132d6dbbcb5c6e5a2977c4beb521dc1500e6ed53
jonatack
reviewed
Dec 7, 2021
RandyMcMillan
pushed a commit
to RandyMcMillan/mempool-tab
that referenced
this pull request
Dec 23, 2021
…anReader c6d9984 Generalize/simplify VectorReader into SpanReader (Pieter Wuille) Pull request description: 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. ACKs for top commit: MarcoFalke: re-ACK c6d9984 🖨 shaavan: ACK c6d9984 Tree-SHA512: 959e3251e0cfe20e13a50639b617c9dc2a561d613a0884d983c93d15dacb6d2305d760aa933d18ba055cef8a1651a344bcb6b3f93051ecf26d3f2efc5779efa4
RandyMcMillan
pushed a commit
to RandyMcMillan/mempool-tab
that referenced
this pull request
Dec 23, 2021
… in SpanReader 9f98adf Remove unused (and broken) functionality in SpanReader (Pieter Wuille) Pull request description: This removes the ability to set an offset in the `SpanReader::SpanReader` constructor, as the current code is broken since #23653. All call sites use `pos=0`, so it is actually unused. If future call sites need it, `SpanReader{a, b, c, d}` is equivalent to `SpanReader{a, b, c.subspan(d)}`. It also removes the ability to deserialize from `SpanReader` directly from the constructor. This too is unused, and can be more idiomatically simulated using `(SpanReader{a, b, c} >> x >> y >> z)` instead of `SpanReader{a, b, c, x, y, z}`. This was pointed out by achow101 in bitcoin/bitcoin#23653 (comment). ACKs for top commit: jb55: crACK 9f98adf achow101: ACK 9f98adf Tree-SHA512: 700ebcd74147628488c39168dbf3a00f8ed41709a26711695f4bf036250a9b115574923bbf96040ec7b7fee4132d6dbbcb5c6e5a2977c4beb521dc1500e6ed53
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.