refactor: Use spans of std::byte in serialize

MarcoFalke commented at 2:52 pm on November 4, 2021: member

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 commented at 2:57 pm on November 4, 2021: member

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

in src/span.h:247 in fab18b8b12 outdated

242+Span<const std::byte> AsBytes(Span<T> s) noexcept
243+{
244+    return {reinterpret_cast<const std::byte*>(s.data()), s.size_bytes()};
245+}
246+template <typename T>
247+Span<std::byte> AsWriteableBytes(Span<T> s) noexcept

sipa commented at 3:03 pm on November 4, 2021:

This probably needs std::enable_if_t<std::is_const_v<T>, Span<std::byte>> as return type, to prevent using this on const spans.

MarcoFalke commented at 3:25 pm on November 4, 2021:

reinterpret_cast can not be used to remove constness. This will fail with something like:

0./span.h:249:13: error: reinterpret_cast from 'const unsigned char *' to 'std::byte *' casts away qualifiers
1    return {reinterpret_cast<std::byte*>(s.data()), s.size_bytes()};
2            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3note: in instantiation of function template specialization 'AsWriteableBytes<const unsigned char>' requested here

sipa commented at 3:28 pm on November 4, 2021:

Interesting, I wasn’t aware of that.

Still, the C++20 std::as_writable_bytes definition (at least on cppreference) does state:

as_writable_bytes only participates in overload resolution if std::is_const_v<T> is false.

So I don’t think this really changes behavior in any way, but the error you’d get is different (the c++20 one will fail to match the function; the current code here will fail at substitution).

MarcoFalke commented at 9:18 am on November 22, 2021:

This has been pulled out into https://github.com/bitcoin/bitcoin/pull/23451/files#diff-f9b6e49de18c8395f9f521ead7ac2c688ed91826ebcecea0b7440b6d659c5abcR244 @sipa Let me know there, if you are against it.

in src/serialize.h:85 in fa441780ee outdated

91 }
92 template<typename Stream> inline void ser_writedata64(Stream &s, uint64_t obj)
93 {
94     obj = htole64(obj);
95-    s.write((char*)&obj, 8);
96+    s.write({BytePtr(&obj), 8});

sipa commented at 3:13 pm on November 4, 2021:

Post-#23413, this could also be written as s.write(AsBytes(Span{&obj,1}));. Before it’s a bit uglier.

I’m not sure it’s better, but it does avoid the need to explicitly list the size of the type.

MarcoFalke commented at 3:29 pm on November 4, 2021:

Yeah, the current version is closer to a refactor (only touching the type of the pointer). Happy to adjust though, if #23413 makes it in in the meantime. I think both versions are equally acceptable.

MarcoFalke force-pushed on Nov 4, 2021

DrahtBot added the label Block storage on Nov 4, 2021

DrahtBot added the label Consensus on Nov 4, 2021

DrahtBot added the label P2P on Nov 4, 2021

DrahtBot added the label Utils/log/libs on Nov 4, 2021

DrahtBot added the label UTXO Db and Indexes on Nov 4, 2021

DrahtBot added the label Wallet on Nov 4, 2021

MarcoFalke commented at 4:06 pm on November 4, 2021: member

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 ?

MarcoFalke removed the label Wallet on Nov 4, 2021

MarcoFalke removed the label UTXO Db and Indexes on Nov 4, 2021

MarcoFalke removed the label P2P on Nov 4, 2021

MarcoFalke removed the label Consensus on Nov 4, 2021

MarcoFalke removed the label Block storage on Nov 4, 2021

MarcoFalke added the label Refactoring on Nov 4, 2021

MarcoFalke renamed this:
~~Use spans of std::byte in serialize~~
refactor: Use spans of std::byte in serialize
on Nov 4, 2021

DrahtBot commented at 6:19 am on November 5, 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:

#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.

in src/pubkey.h:157 in fa6bde9649 outdated

148+            s.read({BytePtr(vch), len});
149             if (len != size()) {
150                 Invalidate();
151             }
152         } else {
153             // invalid pubkey, skip available data

sipa commented at 4:09 am on November 6, 2021:

I think this could just be s.ignore(len);.

MarcoFalke commented at 6:05 pm on November 6, 2021:

Good idea. Done in https://github.com/bitcoin/bitcoin/pull/23451

in src/hash.h:116 in fa6bde9649 outdated

110@@ -111,8 +111,9 @@ class CHashWriter
111     int GetType() const { return nType; }
112     int GetVersion() const { return nVersion; }
113 
114-    void write(const char *pch, size_t size) {
115-        ctx.Write((const unsigned char*)pch, size);
116+    void write(Span<const std::byte> src)
117+    {
118+        ctx.Write(UCharCast(src.data()), src.size());

sipa commented at 4:09 am on November 6, 2021:

Any plan to make hash interfaces also use Span<std::byte> based arguments?

MarcoFalke commented at 8:59 am on November 6, 2021:

Maybe if I am bored over the holiday season, but not now and not in this pull :sweat_smile:

sipsorcery commented at 8:44 pm on November 6, 2021: member

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.

sipsorcery referenced this in commit bb1c84082c on Nov 6, 2021

MarcoFalke marked this as a draft on Nov 7, 2021

fanquake referenced this in commit 12f2b6ac01 on Nov 8, 2021

MarcoFalke referenced this in commit 9394964f6b on Nov 24, 2021

MarcoFalke marked this as ready for review on Nov 24, 2021

MarcoFalke force-pushed on Nov 24, 2021

sidhujag referenced this in commit b8e78e1f0b on Nov 24, 2021

sidhujag referenced this in commit 1e01bd1d92 on Nov 24, 2021

DrahtBot added the label Needs rebase on Dec 3, 2021

MarcoFalke force-pushed on Dec 3, 2021

DrahtBot removed the label Needs rebase on Dec 3, 2021

in src/streams.h:405 in fa568864d5 outdated

398@@ -398,18 +399,18 @@ class CDataStream
399         nReadPos = nReadPosNext;
400     }
401 
402-    void write(const char* pch, size_t nSize)
403+    void write(Span<const value_type> src)
404     {
405         // Write to the end of the buffer
406-        vch.insert(vch.end(), pch, pch + nSize);
407+        vch.insert(vch.end(), src.data(), src.end());

sipa commented at 7:38 pm on December 3, 2021:

For consistency (and compatibility with std::span) probably better use src.begin() rather than src.data() here.

sipa commented at 7:43 pm on December 3, 2021: member

utACK face063b2bf0bea00f014cf0fbaa01fbb6bdeb9f

MarcoFalke force-pushed on Dec 6, 2021

MarcoFalke commented at 2:00 pm on December 6, 2021: member

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

sipa commented at 9:36 pm on December 21, 2021: member

re-utACK fade79421730079481fc8837d7b86bbcdaeb6131

fanquake requested review from laanwj on Dec 23, 2021

span: Add BytePtr helper fa65bbf217

Use spans of std::byte in serialize

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

fa24493d63

Remove unused char serialize fa5d2e678c

MarcoFalke force-pushed on Jan 2, 2022

MarcoFalke commented at 2:33 pm on January 2, 2022: member

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

PastaPastaPasta approved

PastaPastaPasta commented at 2:46 am on January 10, 2022: contributor

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

fanquake requested review from sipa on Jan 10, 2022

sipa approved

sipa commented at 4:00 am on January 10, 2022: member

re-utACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903

MarcoFalke requested review from ryanofsky on Jan 10, 2022

laanwj added this to the "Blockers" column in a project

PastaPastaPasta commented at 8:32 am on January 27, 2022: contributor

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.

MarcoFalke commented at 1:18 pm on January 27, 2022: member

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 commented at 6:04 pm on January 27, 2022: member

Concept and code review ACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903 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.

in src/streams.h:425 in fa24493d63 outdated

421@@ -421,7 +422,7 @@ class CDataStream
422         }
423 
424         for (size_type i = 0, j = 0; i != size(); i++) {
425-            vch[i] ^= key[j++];
426+            vch[i] ^= std::byte{key[j++]};

laanwj commented at 6:11 pm on January 27, 2022:

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>&.

laanwj merged this on Jan 27, 2022

laanwj closed this on Jan 27, 2022

laanwj removed this from the "Blockers" column in a project

MarcoFalke deleted the branch on Jan 27, 2022

sidhujag referenced this in commit cc616fea0d on Jan 28, 2022

sidhujag referenced this in commit b14bff8e55 on Jan 28, 2022

sidhujag referenced this in commit 622e655728 on Feb 12, 2022

DrahtBot locked this on Jan 28, 2023

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

Conflicts