Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

merge #12752, #10785, #17896, #17957, #18021, #18112: serialization improvements #4037

Merged
merged 6 commits into from
May 14, 2021
Merged

merge #12752, #10785, #17896, #17957, #18021, #18112: serialization improvements #4037

merged 6 commits into from
May 14, 2021
+328 −352

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Mar 10, 2021 •
edited
Loading

Overview

As part of #4025, component pull requests that are required by TorV3 logic are being broken down into branches wherever feasable. This PR deals with serialization improvements taken from bitcoin#10785 (split into bitcoin#17850, bitcoin#17896, bitcoin#17957, bitcoin#18021, bitcoin#18112 and others)

Contents

@kwvg kwvg mentioned this pull request Mar 10, 2021
Closed
@UdjinM6 UdjinM6 added this to the 17 milestone Mar 10, 2021
UdjinM6
UdjinM6 previously approved these changes Mar 10, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I like skipping commits here, would be nice to finalize these backports while we are at it... But LGTM otherwise, so

utACK

xdustinface
xdustinface suggested changes Mar 12, 2021
View reviewed changes
Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit see below and i also feel like we shouldn't just randomly skip commits while backporting PRs only because they are "not relevant". There is imo no good reason to only partially backport PRs if its possible to completely merge them in.. skipping parts of them just makes reviewing the PR as a whole a bit weird/confusing here and there imo and this even two times, now and when backporting the remaining parts.

@kwvg kwvg changed the title partial merge #10785: serialization improvements merge #10785, #12752: serialization improvements Mar 16, 2021
@kwvg kwvg requested review from xdustinface and UdjinM6 March 16, 2021 09:38
UdjinM6
UdjinM6 requested changes Mar 16, 2021
View reviewed changes
@kwvg kwvg requested a review from UdjinM6 March 16, 2021 17:08
UdjinM6
UdjinM6 previously approved these changes Mar 17, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly tested ACK

xdustinface
xdustinface previously approved these changes Mar 23, 2021
View reviewed changes
Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@PastaPastaPasta PastaPastaPasta modified the milestones: 17, 17.1 Mar 23, 2021
@PastaPastaPasta
Copy link
Member

please resolve conflicts

@kwvg kwvg dismissed stale reviews from xdustinface and UdjinM6 via b3033f7 April 18, 2021 13:08
@PastaPastaPasta
Copy link
Member

@UdjinM6 is this merge okay, or does it need to be a rebase?

@UdjinM6
Copy link

UdjinM6 commented Apr 18, 2021

¯\_(ツ)_/¯

Personally, I'd prefer a rebase but technically this merge should be ok too I guess

kwvg added 6 commits April 18, 2021 17:02
@kwvg @PastaPastaPasta
Merge bitcoin#17850: Convert addrdb/addrman to new serialization
e483a68 
bitcoin@9250a08

Partial bitcoin#17850: Introduce new serialization macros without casts

bitcoin@ca33451
@kwvg @PastaPastaPasta
Merge bitcoin#17896: Convert chain to new serialization
46cb6e2 
bitcoin@9b66083

Partial bitcoin#17896: Convert VARINT to the formatter/Using approach

bitcoin@2f1b2f4

Partial bitcoin#17896: Add a generic approach for (de)serialization of objects

bitcoin@ca62563
@kwvg @PastaPastaPasta
Merge bitcoin#18021: Convert undo.h to new serialization framework
0802bdf 
bitcoin@3c94b00

Partial bitcoin#18021: Make std::vector and prevector reuse the VectorFormatter logic

bitcoin@3cd8ab9

Partial bitcoin#18021: Add custom vector-element formatter

bitcoin@abf8624

Partial bitcoin#18021: Add a constant for the maximum vector allocation (5 Mbyte)

37d800b
@kwvg @PastaPastaPasta
Merge bitcoin#12752: Move compressor utility functions out of class
2f542a9 
bitcoin@76a9aac
@kwvg @PastaPastaPasta
Merge bitcoin#17957: Convert compression.h to new serialization frame…
bfe5971 
…work

bitcoin@4de934b

Partial bitcoin#17957: Add FORMATTER_METHODS, similar to SERIALIZE_METHODS, but for formatters

bitcoin@ca34c5c
@kwvg @PastaPastaPasta
Merge bitcoin#18112: Convert blockencodings.h to new serialization fr…
97caca3 
…amework

bitcoin@353f376

Partial bitcoin#18112: Add CustomUintFormatter

bitcoin@e574fff

Partial bitcoin#18112: Add DifferenceFormatter

bitcoin@1063339

Partial bitcoin#18112: Make VectorFormatter support stateful formatters

bitcoin@56dd9f0

Partial bitcoin#18112: Convert CCompactSize to proper formatter

bitcoin@3ca574c
@PastaPastaPasta
Copy link
Member

Yeah, I prefer rebase I think. @kittywhiskers please see https://github.com/PastaPastaPasta/dash/commits/pr_4027 I rebased on develop and squashed your commits

@kwvg
Copy link
Collaborator Author

kwvg commented May 5, 2021

Branch updated with @PastaPastaPasta's fork as-is

PastaPastaPasta
PastaPastaPasta approved these changes May 6, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, although should probably adjust title

@UdjinM6 UdjinM6 changed the title merge #10785, #12752: serialization improvements merge #12752, #10785, #17896, #17957, #18021, #18112: serialization improvements May 6, 2021
UdjinM6
UdjinM6 approved these changes May 6, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@UdjinM6 UdjinM6 requested a review from xdustinface May 6, 2021 16:29
@UdjinM6 UdjinM6 merged commit 20b7170 into dashpay:develop May 14, 2021
@kwvg kwvg deleted the serialize branch July 18, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6

@PastaPastaPasta PastaPastaPasta

@xdustinface xdustinface

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
18
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kwvg @PastaPastaPasta @UdjinM6 @xdustinface