[Backport] Serialization framework updates by furszy · Pull Request #2412 · PIVX-Project/PIVX

furszy · 2021-06-08T19:32:42Z

Decoupled from #2411, built on top of #2359.

Focused on creating the Span class and updating the serialization framework and every object using it up to latest upstream structure (3-4 years ahead of what we currently are in master). We will be up-to-date with them in the area after finishing with #2411 entirely (there are few more updates to the serialization code that comes down #2411 commits line that cannot cherry-pick here).

Adapted the following PRs:

furszy · 2021-06-09T20:33:49Z

rebased on master, ready to go.

random-zebra · 2021-06-18T14:45:23Z

This is a great job, really loved it.
Although I think that 068facd contains an error, which causes the warning for which f767d26 was introduced (which, in turn, fyi, results in a bunch of -Wunknown-pragmas warnings on GCC).

The issue here is a mis-use of SER_READ.
This macro is not meant to be used to actually read a value from the stream and save it to the variable passed as argument (like READWRITE). The second argument to SER_READ is a function, to be applied bypassing the const-ness of the object inside SERIALIZE_METHODS.

So, for example, we have something like:

std::vector<unsigned char> bytes;
READWRITE(bytes);
SER_READ(obj, obj.vBits = BytesToBits(bytes));

where, when ser_action.ForRead, READWRITE stores the vector contents inside the bytes local variable, and then SER_READ is used to update the object's member vBits.

When, instead, we do things like:

SER_READ(obj, nMoneySupply);

we are essentially passing this lambda to ::SerRead

[&](Stream& s, typename std::remove_const<Type>::type& obj) { nMoneySupply; }

which is a statement that has no effect on obj.
This is what the -Wunused-value is warning about (not because those variables are dummy containers inside SERIALIZE_METHODS).

And IMO this actually signals an error: as we are not reading anything with those SER_READ calls, we are not skipping the dummy fields, and so the serialization will fail with old DB versions.

random-zebra@3e20242

furszy · 2021-06-18T15:19:14Z

yeah great, nice catch ☕ . Updating.

furszy · 2021-06-18T15:30:45Z

Updated per feedback, cherry-picked your commit zebra and removed the warning suppress.

furszy · 2021-06-19T15:10:02Z

Rebased on master, migrating the recently merged ProUpServPL class to the new serialization form. Preventing a post-merge "fix missing" PR.

random-zebra

There is a change in the serialization of CBudgetVote, which makes it impossible to sync proposal votes from nodes that are not running this PR.

furszy · 2021-06-26T13:14:10Z

Updated per feedback, squashed change in 8a12b0c.
Fast diff check with git range-diff master 6cfd257 d361548.

random-zebra

Code review ACK d36154890a8b20d8aae9f6b344dbd00b12ce10dc

Will run it live for some time.

Based on btc@da74db0940720407fafaf3582bbaf9c81a4d3b4d

Now that `GetType()` is not propagated, the benefits are not worth the code.

This new approach uses a static method which takes the object as a argument. This has the advantage that its constness can be a template parameter, allowing a single implementation that sees the object as const for serialization and non-const for deserialization, without casts. More boilerplate is included in the new macro as well.

… other classes This adds the (internal) Wrapper class, and the Using function that uses it. Given a class F that implements Ser(stream, const object&) and Unser(stream, object&) functions, this permits writing e.g. READWRITE(Using<F>(object)).

This removes the need for the GNU C++ extension of variadic macros.

Extracted by Pieter Wuille from a comment by Russ Yanofsky, see bitcoin#18317 (comment).

Extracted and extended by Pieter Wuille from a comment by Russ Yanofsky (see bitcoin#18317 (comment)).

…ions that doesn't require an special treatment.

Adaptation of btc@ef17c03e074b6c3f185afa4eff572ba687c2a171

furszy · 2021-07-03T12:12:12Z

Rebased on master, no code changes. So this branch does not reject blocks v10 on mainnet due the recently merged #2434 for v5.2 (requirement signaled by zebra that has been testing it ☕ ☕).

Can easily verify that nothing has changed with git range-diff master d361548 bd4b846d

random-zebra

ACK bd4b846

Fuzzbawls

ACK bd4b846

furszy self-assigned this Jun 8, 2021

furszy added this to the 6.0.0 milestone Jun 8, 2021

furszy added the Upstream label Jun 8, 2021

furszy force-pushed the 2021_serialization_updates branch from 475c54d to adba8de Compare June 9, 2021 20:25

furszy force-pushed the 2021_serialization_updates branch from adba8de to 4f80853 Compare June 18, 2021 15:29

furszy force-pushed the 2021_serialization_updates branch from 4f80853 to 6cfd257 Compare June 19, 2021 15:06

random-zebra suggested changes Jun 26, 2021

View reviewed changes

Comment thread src/budget/budgetvote.h Outdated

furszy force-pushed the 2021_serialization_updates branch from 6cfd257 to d361548 Compare June 26, 2021 13:07

random-zebra reviewed Jun 26, 2021

View reviewed changes

random-zebra added Refactoring Util / log / libs UTXO DBs and Indexes labels Jun 28, 2021

furszy requested a review from Fuzzbawls June 30, 2021 14:57

furszy mentioned this pull request Jul 1, 2021

Road to Tor v3 support (BIP155) #2411

Merged

sipa and others added 10 commits July 3, 2021 09:02

Add Slice: a (pointer, size) array view that acts like a container

8fef544

Support serializing Span<unsigned char> and use that instead of FLATDATA

1ef2d90

Migrate last FLATDATA calls to use Span.

fb3c646

Introduce BigEndian wrapper and use it for netaddress ports

5c36b3d

Drop unused GetType() from CSizeComputer

f05e692

Based on btc@da74db0940720407fafaf3582bbaf9c81a4d3b4d

Drop minor GetSerializeSize template

ace7d14

Now that `GetType()` is not propagated, the benefits are not worth the code.

Convert addrdb/addrman to new serialization

ace3895

Convert VARINT to the formatter/Using approach

bbfc55c

sipa and others added 24 commits July 3, 2021 09:02

Make std::vector and prevector reuse the VectorFormatter logic

a926ba3

Convert undo.h to new serialization framework

e107a0c

Get rid of VARINT default argument

bb99030

This removes the need for the GNU C++ extension of variadic macros.

Convert CCompactSize to proper formatter

4e2afad

Make VectorFormatter support stateful formatters

fd29a50

Add CustomUintFormatter

d6380c4

Fix a violation of C++ standard rules that unions cannot be switched.

806213a

Add static_asserts to ser_X_to_Y() methods

3765d6c

Merge BigEndian functionality into CustomUintFormatter

c4d6228

Extend CustomUintFormatter to support enums

7344c1a

Extracted by Pieter Wuille from a comment by Russ Yanofsky, see bitcoin#18317 (comment).

Add SER_READ and SER_WRITE for read/write-dependent statements

13577fb

Extracted and extended by Pieter Wuille from a comment by Russ Yanofsky (see bitcoin#18317 (comment)).

Convert merkleblock to new serialization

3d3ee64

Convert everything except wallet/qt to new serialization (step 1)

0f15784

Convert to new serialization (step 2). Focused on object's serializat…

eccd473

…ions that doesn't require an special treatment.

Add comments to CustomUintFormatter

3f7826e

Convert Qt to new serialization

35fca11

Remove old MESS_VER_STRMESS message version try-catch.

dc0fc95

Convert to new serialization (step 3)

cf06950

Convert wallet to new serialization

221bf49

Adaptation of btc@ef17c03e074b6c3f185afa4eff572ba687c2a171

Convert CDiskBlockIndex to new serialization.

1ee0cb2

Fix CDiskBlockIndex serialization of dummy fields for old DB versions

f021897

Convert LimitedString to formatter

8c74c09

Convert the last, non-trivial, serialization functions to the new form

060d62b

Remove old serialization primitives

bd4b846

furszy force-pushed the 2021_serialization_updates branch from d361548 to bd4b846 Compare July 3, 2021 12:04

random-zebra approved these changes Jul 4, 2021

View reviewed changes

Fuzzbawls approved these changes Jul 5, 2021

View reviewed changes

furszy merged commit 5c93f15 into PIVX-Project:master Jul 5, 2021

furszy deleted the 2021_serialization_updates branch November 29, 2022 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backport] Serialization framework updates#2412

[Backport] Serialization framework updates#2412
furszy merged 40 commits into
PIVX-Project:masterfrom
furszy:2021_serialization_updates

furszy commented Jun 8, 2021

Uh oh!

furszy commented Jun 9, 2021

Uh oh!

random-zebra commented Jun 18, 2021 •

edited

Loading

Uh oh!

furszy commented Jun 18, 2021

Uh oh!

furszy commented Jun 18, 2021

Uh oh!

furszy commented Jun 19, 2021

Uh oh!

random-zebra left a comment

Uh oh!

Uh oh!

furszy commented Jun 26, 2021

Uh oh!

random-zebra left a comment

Uh oh!

furszy commented Jul 3, 2021

Uh oh!

random-zebra left a comment

Uh oh!

Fuzzbawls left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

furszy commented Jun 8, 2021

Uh oh!

furszy commented Jun 9, 2021

Uh oh!

random-zebra commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jun 18, 2021

Uh oh!

furszy commented Jun 18, 2021

Uh oh!

furszy commented Jun 19, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Jun 26, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

furszy commented Jul 3, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

random-zebra commented Jun 18, 2021 •

edited

Loading