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
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
1return {reinterpret_cast<std::byte*>(s.data()), s.size_bytes()};
2^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~3note: in instantiation of function template specialization 'AsWriteableBytes<const unsigned char>' requested here
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:
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
fa6bde9649outdated
148+ s.read({BytePtr(vch), len});
149 if (len != size()) {
150 Invalidate();
151 }
152 } else {
153 // invalid pubkey, skip available data
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
fa568864d5outdated
398@@ -398,18 +399,18 @@ class CDataStream
399 nReadPos = nReadPosNext;
400 }
401402- 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());
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
utACKface063b2bf0bea00f014cf0fbaa01fbb6bdeb9f
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-utACKfade79421730079481fc8837d7b86bbcdaeb6131
fanquake requested review from laanwj
on Dec 23, 2021
span: Add BytePtr helperfa65bbf217
Use spans of std::byte in serialize
This switches .read() and .write() to take spans of bytes.
fa24493d63
Remove unused char serializefa5d2e678c
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-utACKfa5d2e678c809c26bd40d7e7c171529d3ffb5903
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 ACKfa5d2e678c809c26bd40d7e7c171529d3ffb5903
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
fa24493d63outdated
421@@ -421,7 +422,7 @@ class CDataStream
422 }
423424 for (size_type i = 0, j = 0; i != size(); i++) {
425- vch[i] ^= key[j++];
426+ 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>&.
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
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-02-23 18:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me