refactor: Use spans of std::byte in serialize by maflcko · Pull Request #23438 · bitcoin/bitcoin

maflcko · 2021-11-04T14:52:17Z

This changes the serialize code (.read() and .write() functions) to take a Span instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to std::byte (instead of using char).

The benefits of using Span:

Less verbose and less fragile code when passing an already existing Span(-like) object to or from serialization

The benefits of using std::byte:

std::byte can't accidentally be mistaken for an integer

The goal here is to only change serialize to use spans of std::byte. If needed, AsBytes, MakeUCharSpan, ... can be used (temporarily) to pass spans of the right type.

Other changes that are included here:

#22167 (refactor: Remove char serialize by MarcoFalke)
#21906 (Preserve const in cast on CTransactionSignatureSerializer by promag)

sipa · 2021-11-04T14:57:58Z

Oh, std::byte is already in C++17!

src/span.h

src/serialize.h

maflcko · 2021-11-04T16:06:27Z

There seems to be a bug on msvc, because it doesn't eat the code I prepared. There is also no error. Any ideas @sipsorcery ?

DrahtBot · 2021-11-05T06:19:41Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24143 (WIP: net/p2p:rename command*/Command/* to message*/Message* by RandyMcMillan)
#23829 (refactor: use braced init for integer literals instead of c style casts by PastaPastaPasta)
#23810 (refactor: destroy all C-style casts; use modern C++ casts, enforce via -Wold-style-cast by PastaPastaPasta)
#19690 (util: improve FindByte() performance by LarryRuane)
#16981 (Improve runtime performance of --reindex by LarryRuane)

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.

src/pubkey.h

src/hash.h

sipsorcery · 2021-11-06T20:44:55Z

There seems to be a bug on msvc, because it doesn't eat the code I prepared. There is also no error. Any ideas @sipsorcery ?

Seems to be a const change for a bitcoinconsensus_verify_script_with_amount parameter.

Changing this line to the below fixes it (I added the const cast):

auto op0Result = bitcoinconsensus_verify_script_with_amount(pubKeyScript.data(), pubKeyScript.size(), amount, (const unsigned char*)stream.data(), stream.size(), 0, bitcoinconsensus_SCRIPT_FLAGS_VERIFY_ALL, &err);

But perhaps a better thing to do would be to remove the build_msvc/testconsensus directory completely. It's a test app that, in hindsight, would have been better put into a unit test somewhere. I suspect no one ever uses it. I haven't looked at it in the 4 years since I first wrote it... I'll submit a PR to remove it.

The testconsensus project is not integral to the Bitcoin Core code. It was originally added as a quick and dirty demo of how to do a consensus check with the msvc build. There are better examples. PR bitcoin#23438 made a change that caused a compiler error with the buildmsvc/testconsensus code. Rather than leave it hanging around to incur potential bitrot, or furhter PR build failures, it should be removed.

bb1c840 Remove the build_msvc/testconsensus project (Aaron Clauson) Pull request description: The testconsensus project is not integral to the Bitcoin Core code. It was originally added as a quick and dirty demo of how to do a consensus check with the msvc build. There are better examples. PR #23438 made a change that caused a compiler error with the buildmsvc/testconsensus code. Rather than leave it hanging around to incur potential bitrot, or further PR build failures, it should be removed. ACKs for top commit: hebasto: ACK bb1c840, tested on Windows 10 Pro (20H2). Tree-SHA512: d81b99eb09171b66c179961b15f0b2e2e97e5ee7f011f18667e890c90e3d169593ad9aedd05a8616e962212952048827b7159d3c2a2ecb7ac378136b80bf6b23

sipa

utACK face063b2bf0bea00f014cf0fbaa01fbb6bdeb9f

src/streams.h

maflcko · 2021-12-06T14:00:12Z

Thanks, force pushed to replace data-end with begin-end.

sipa · 2021-12-21T21:36:07Z

re-utACK fade79421730079481fc8837d7b86bbcdaeb6131

This switches .read() and .write() to take spans of bytes.

maflcko · 2022-01-02T14:33:43Z

Rebased due to silent conflict. Trivial to re-review via git range-diff bitcoin-core/master fade794217 fa5d2e678c.

PastaPastaPasta

utACK

I have reviewed the code, and didn't find any issues.

I am very happen to see std::byte being used, as well as spans :) All good refactoring as far as I can tell

sipa

re-utACK fa5d2e6

PastaPastaPasta · 2022-01-27T08:32:57Z

IMO, this PR could be merged. It's been 17 days w/o any review, and two acks. And it's been in high-priority for review for 7 days. I'm not sure what exactly the procedure is for bitcoin-core, but I think this relatively simple refactoring PR can be merged at this point.

maflcko · 2022-01-27T13:18:00Z

it's been in high-priority for review for 7 days. I'm not sure what exactly the procedure is for bitcoin-core, but I think this relatively simple refactoring PR can be merged at this point.

There is no strict procedure in this project, but if a pull request hasn't been merged, it can either mean that all maintainers that looked at it preferred more review, or it can mean that it went unnoticed from maintainers.
High-priority doesn't have any meaning for the project. It is a way for individual contributors to share their favourite pet (their most important pull among all of their pulls) with others.
While it isn't incredibly hard to review this, it also isn't trivial. Integral types in C++ aren't type safe. While this changes some stuff to span<byte>, which is type safe, there are conversions at the "boundaries", which might cause issues even if the code looks right and compiles and passes all tests.

laanwj · 2022-01-27T18:04:21Z

Concept and code review ACK fa5d2e6
Removing the bare char serializer makes a lot of sense, it's always been kind of a wildcard type resulting on annoying and potentially dangerous platform differences.

laanwj · 2022-01-27T18:11:51Z

src/streams.h


        for (size_type i = 0, j = 0; i != size(); i++) {
-            vch[i] ^= key[j++];
+            vch[i] ^= std::byte{key[j++]};


I think it would make sense to make ObfuscateKey a std::byte throughout, avoiding this cast. Though no need to do so in this PR. Could also pass in a span here then instead of const std::vector<unsigned char>&.

…alize" This reverts commit cc616fe.

maflcko mentioned this pull request Nov 4, 2021

refactor: Preserve const in cast on CTransactionSignatureSerializer #21906

Closed

sipa reviewed Nov 4, 2021

View reviewed changes

src/span.h Outdated Show resolved Hide resolved

sipa reviewed Nov 4, 2021

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2105-uin8t branch from fa44178 to fafbcfb Compare November 4, 2021 15:33

DrahtBot added Block storage Consensus P2P Utils/log/libs UTXO Db and Indexes Wallet labels Nov 4, 2021

maflcko added Refactoring and removed Wallet UTXO Db and Indexes P2P Consensus Block storage labels Nov 4, 2021

maflcko changed the title ~~Use spans of std::byte in serialize~~ refactor: Use spans of std::byte in serialize Nov 4, 2021

This was referenced Nov 5, 2021

util: improve FindByte() performance #19690

Merged

Improve runtime performance of --reindex #16981

Merged

sipa reviewed Nov 6, 2021

View reviewed changes

src/pubkey.h Outdated Show resolved Hide resolved

sipa reviewed Nov 6, 2021

View reviewed changes

src/hash.h Outdated Show resolved Hide resolved

maflcko mentioned this pull request Nov 6, 2021

span: Add std::byte helpers #23451

Merged

sipsorcery mentioned this pull request Nov 6, 2021

Remove the build_msvc/testconsensus project #23453

Merged

maflcko marked this pull request as draft November 7, 2021 09:43

maflcko force-pushed the 2105-uin8t branch from facf8c3 to face063 Compare December 3, 2021 11:56

DrahtBot removed the Needs rebase label Dec 3, 2021

sipa reviewed Dec 3, 2021

View reviewed changes

src/streams.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2105-uin8t branch from face063 to fade794 Compare December 6, 2021 11:00

maflcko mentioned this pull request Dec 18, 2021

docs: avoid C-style casts; use modern C++ casts #23810

Merged

DrahtBot mentioned this pull request Dec 21, 2021

refactor: use braced init for integer literals instead of c style casts #23829

Merged

fanquake requested a review from laanwj December 23, 2021 06:58

MarcoFalke added 3 commits January 2, 2022 11:13

span: Add BytePtr helper

fa65bbf

Use spans of std::byte in serialize

fa24493

This switches .read() and .write() to take spans of bytes.

Remove unused char serialize

fa5d2e6

maflcko force-pushed the 2105-uin8t branch from fade794 to fa5d2e6 Compare January 2, 2022 14:32

PastaPastaPasta approved these changes Jan 10, 2022

View reviewed changes

fanquake requested a review from sipa January 10, 2022 02:57

sipa approved these changes Jan 10, 2022

View reviewed changes

maflcko requested a review from ryanofsky January 10, 2022 07:50

DrahtBot mentioned this pull request Jan 26, 2022

WIP: net/p2p:rename command*/Command/* to message*/Message* #24143

Closed

laanwj reviewed Jan 27, 2022

View reviewed changes

laanwj merged commit 196b459 into bitcoin:master Jan 27, 2022

maflcko deleted the 2105-uin8t branch January 27, 2022 18:42

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022

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

cc616fe

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022

Revert "Merge bitcoin#23438: refactor: Use spans of std::byte in seri…

b14bff8

…alize" This reverts commit cc616fe.

PastaPastaPasta mentioned this pull request Jan 31, 2022

refactor: only use explicit reinterpret/const casts, not implicit #24185

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 12, 2022

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

622e655

bitcoin locked and limited conversation to collaborators Jan 28, 2023

Conversation

maflcko commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 4, 2021

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 4, 2021

Uh oh!

DrahtBot commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

sipsorcery commented Nov 6, 2021

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 6, 2021

Uh oh!

sipa commented Dec 21, 2021

Uh oh!

maflcko commented Jan 2, 2022

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Jan 27, 2022

Uh oh!

maflcko commented Jan 27, 2022

Uh oh!

laanwj commented Jan 27, 2022

Uh oh!

laanwj Jan 27, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maflcko commented Nov 4, 2021 •

edited

Loading

DrahtBot commented Nov 5, 2021 •

edited

Loading

laanwj Jan 27, 2022 •

edited

Loading