Generalize/simplify VectorReader into SpanReader by sipa · Pull Request #23653 · bitcoin/bitcoin

sipa · 2021-12-02T16:54:08Z

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 · 2021-12-02T16:54:55Z

I'm happy to let #23438 (std::byte Serialize) go first and rebase this on top.

maflcko

ACK 06a1adf88d51e2771802c6c0f646e3ff16a4baa7 💽

Show signature

Signature:

src/blockfilter.cpp

src/streams.h

sipa

Addressed @MarcoFalke's comments.

src/streams.h

src/blockfilter.cpp

DrahtBot · 2021-12-03T04:01:45Z

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

ACK 2c35a93

maflcko · 2021-12-03T09:05:48Z

re-ACK 2c35a93 🖨

Show signature

Signature:

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 · 2021-12-06T20:35:58Z

src/streams.h

+        if (pos > m_data.size()) {
+            throw std::ios_base::failure("SpanReader(...): end of data (pos > m_data.size())");
        }
+        data = data.subspan(pos);


Shouldn't this be

Suggested change

data = data.subspan(pos);

m_data = m_data.subspan(pos);

Ugh, yes! Will create a PR.

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.

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.

… 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

…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

Post-merge ACK 2c35a93

…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

… 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

laanwj added the Utils/log/libs label Dec 2, 2021

maflcko approved these changes Dec 2, 2021

View reviewed changes

src/blockfilter.cpp Outdated Show resolved Hide resolved

src/streams.h Outdated Show resolved Hide resolved

Generalize/simplify VectorReader into SpanReader

2c35a93

sipa force-pushed the 202112_spanreader branch from 06a1adf to 2c35a93 Compare December 2, 2021 19:47

sipa commented Dec 2, 2021

View reviewed changes

src/streams.h Outdated Show resolved Hide resolved

src/blockfilter.cpp Outdated Show resolved Hide resolved

shaavan approved these changes Dec 3, 2021

View reviewed changes

DrahtBot mentioned this pull request Dec 3, 2021

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

Merged

maflcko added the Refactoring label Dec 3, 2021

maflcko merged commit fd1c9e2 into bitcoin:master Dec 3, 2021

achow101 reviewed Dec 6, 2021

View reviewed changes

sipa mentioned this pull request Dec 6, 2021

Remove unused (and broken) functionality in SpanReader #23687

Merged

jonatack reviewed Dec 7, 2021

View reviewed changes

ryanofsky mentioned this pull request Dec 17, 2021

Multiprocess bitcoin #10102

Draft

bitcoin locked and limited conversation to collaborators Dec 7, 2022

Conversation

sipa commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 2, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Dec 3, 2021

Conflicts

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 3, 2021

Uh oh!

achow101 Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

achow101 Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sipa commented Dec 2, 2021 •

edited

Loading

achow101 Dec 6, 2021 •

edited

Loading