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

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:p2p-remove-unused-UnserializeV1Array changing 1 files +3 −11

jonatack commented at 12:58 PM on June 3, 2021: member

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

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

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

eaa6aaee4b

fanquake added the label P2P on Jun 3, 2021
fanquake added the label Refactoring on Jun 3, 2021
practicalswift commented at 1:15 PM on June 3, 2021: contributor

Concept ACK
klementtan approved
klementtan commented at 12:16 AM on June 5, 2021: contributor

ACK eaa6aaee4bbb16594a58c944c3beed78d5759484. Verified that UnserializeV1Stream that is not called anywhere else with rg -c UnserializeV1Array and built cleanly on macOs
sipa commented at 5:34 PM on June 5, 2021: member

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 commented at 2:01 PM on June 6, 2021: member

I would agree, but I don't see a V2 version of this function.
jonatack commented at 2:04 PM on June 6, 2021: member

There is a SerializeV1Array(), though, so that could be a case for not changing this. Either way is fine with me.
laanwj commented at 2:40 PM on June 7, 2021: member

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 eaa6aaee4bbb16594a58c944c3beed78d5759484
practicalswift commented at 7:43 PM on June 7, 2021: contributor

cr ACK eaa6aaee4bbb16594a58c944c3beed78d5759484

Agree with @laanwj
fanquake requested review from vasild on Jun 9, 2021
vasild commented at 2:26 PM on June 9, 2021: member

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 commented at 7:00 PM on July 27, 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:
- #22570 by MarcoFalke
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 added the label Needs rebase on Aug 2, 2021
DrahtBot commented at 11:32 AM on August 2, 2021: member



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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
laanwj commented at 11:40 AM on August 2, 2021: member

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 commented at 11:47 AM on August 2, 2021: member

SGTM
jonatack closed this on Aug 2, 2021
laanwj referenced this in commit 2ef186a140 on Nov 17, 2021
sidhujag referenced this in commit 675133c862 on Nov 17, 2021
DrahtBot locked this on Aug 18, 2022