Skip to content

p2p, refactor: remove unneeded CNetAddr::UnserializeV1Array() #22140

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
Closed
jonatack wants to merge 1 commit into from
Closed

p2p, refactor: remove unneeded CNetAddr::UnserializeV1Array() #22140

jonatack wants to merge 1 commit into from
+3 −11

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Jun 3, 2021

which is only called by CNetAddr::UnserializeV1Stream() and is a wrapper for CNetAddr::SetLegacyIPv6().

@jonatack
p2p, refactor: remove unneeded CNetAddr::UnserializeV1Array()
eaa6aae 
which is only called by CNetAddr::UnserializeV1Stream()
and is a wrapper for CNetAddr::SetLegacyIPv6().
@practicalswift
Copy link
Contributor

practicalswift commented Jun 3, 2021

Concept ACK

klementtan
klementtan approved these changes Jun 5, 2021
View reviewed changes
Copy link
Contributor

@klementtan klementtan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK eaa6aae. Verified that UnserializeV1Stream that is not called anywhere else with rg -c UnserializeV1Array and built cleanly on macOs

@sipa
Copy link
Member

sipa commented Jun 5, 2021 •
edited
Loading

I'm not convinced this is an improvement. Sure, it's a trivial function, but by having it this way there is symmetry with the V2 version of this function (I assume that was the reason for writing it this way).

@jonatack
Copy link
Member Author

jonatack commented Jun 6, 2021

I would agree, but I don't see a V2 version of this function.

@jonatack
Copy link
Member Author

jonatack commented Jun 6, 2021

There is a SerializeV1Array(), though, so that could be a case for not changing this. Either way is fine with me.

@laanwj
Copy link
Member

laanwj commented Jun 7, 2021

I think it's okay to get rid of this intermediate function. The signature uint8_t (&arr)[V1_SERIALIZATION_SIZE] looks really strange to me.

Code review ACK eaa6aae

@practicalswift
Copy link
Contributor

practicalswift commented Jun 7, 2021

cr ACK eaa6aae

Agree with @laanwj

@fanquake fanquake requested a review from vasild June 9, 2021 13:59
@vasild
Copy link
Contributor

vasild commented Jun 9, 2021

The reasoning is to have pairs of ser/unser functions, e.g. SerializeFoo() / UnserializeFoo() and have UnserializeFoo(SerializeFoo(x)) == x. This makes the code easier to verify visually and easier to modify - should one of those be altered, it is clear that a corresponding reverse mod should be applied to the other.

We currently have SerializeV1Array() which is not going away. Thus it would be nice to have UnserializeV1Array() (ie not to remove it in this PR).

There is no ser/unser for V2 to/from arrays because V2 length is variable.

@DrahtBot DrahtBot mentioned this pull request Jul 27, 2021
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 27, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@DrahtBot DrahtBot mentioned this pull request Jul 29, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 2, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@laanwj
Copy link
Member

laanwj commented Aug 2, 2021 •
edited
Loading

We currently have SerializeV1Array() which is not going away. Thus it would be nice to have UnserializeV1Array() (ie not to remove it in this PR).

OK makes sense, let's close this then. I think API symmetry can be important but hadn't noticed it's an issue here.

@jonatack
Copy link
Member Author

jonatack commented Aug 2, 2021

SGTM

@jonatack jonatack closed this Aug 2, 2021
@jonatack jonatack mentioned this pull request Sep 5, 2021
Merged
laanwj added a commit that referenced this pull request Nov 17, 2021
@laanwj
Merge #22881: doc: provide context for CNetAddr::UnserializeV1Array()…
2ef186a 
… and span.h with lifetimebound

33c6a20 span, doc: provide span.h context and explain lifetimebound definition (Jon Atack)
d14395b net, doc: provide context for UnserializeV1Array() (Jon Atack)

Pull request description:

  Add contextual documentation for developers and future readers of the code regarding
  - CNetAddr::UnserializeV1Array (see #22140)
  - Span and why it defines Clang lifetimebound locally rather than using the one in attributes.h

ACKs for top commit:
  laanwj:
    Documentation review ACK 33c6a20

Tree-SHA512: cb8e6a6c23b36c9ef7499257e97c5378ec895bb9122b79b63b572d9721a1ae6ce6c0be7ad61bdf976c255527ae750fc9ff4b3e03c07c6c797d14dbc82ea9fb3a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 17, 2021
@laanwj
Merge bitcoin#22881: doc: provide context for CNetAddr::UnserializeV1…
675133c 
…Array() and span.h with lifetimebound

33c6a20 span, doc: provide span.h context and explain lifetimebound definition (Jon Atack)
d14395b net, doc: provide context for UnserializeV1Array() (Jon Atack)

Pull request description:

  Add contextual documentation for developers and future readers of the code regarding
  - CNetAddr::UnserializeV1Array (see bitcoin#22140)
  - Span and why it defines Clang lifetimebound locally rather than using the one in attributes.h

ACKs for top commit:
  laanwj:
    Documentation review ACK 33c6a20

Tree-SHA512: cb8e6a6c23b36c9ef7499257e97c5378ec895bb9122b79b63b572d9721a1ae6ce6c0be7ad61bdf976c255527ae750fc9ff4b3e03c07c6c797d14dbc82ea9fb3a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vasild vasild Awaiting requested review from vasild

1 more reviewer

@klementtan klementtan klementtan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Needs rebase P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jonatack @practicalswift @sipa @laanwj @vasild @DrahtBot @klementtan @fanquake