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

1. 
   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().
2. p2p, refactor: remove unneeded CNetAddr::UnserializeV1Array()

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

   eaa6aaee4b
3. fanquake added the label P2P on Jun 3, 2021
4. fanquake added the label Refactoring on Jun 3, 2021
5. 
   practicalswift commented at 1:15 pm on June 3, 2021: contributor

   Concept ACK
6. klementtan approved
7. 
   klementtan commented at 0: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
8. 
   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).
9. 
   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.
10. 
    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.
11. 
    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

12. 
    practicalswift commented at 7:43 pm on June 7, 2021: contributor

    cr ACK eaa6aaee4bbb16594a58c944c3beed78d5759484

    Agree with @laanwj

13. fanquake requested review from vasild on Jun 9, 2021
14. 
    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.

15. 
    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.

16. DrahtBot added the label Needs rebase on Aug 2, 2021
17. 
    DrahtBot commented at 11:32 am on August 2, 2021: member

    🐙 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”.

18. 
    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.

19. 
    jonatack commented at 11:47 am on August 2, 2021: member

    SGTM
20. jonatack closed this on Aug 2, 2021

    ---

21. laanwj referenced this in commit 2ef186a140 on Nov 17, 2021
22. sidhujag referenced this in commit 675133c862 on Nov 17, 2021
23. DrahtBot locked this on Aug 18, 2022

Contributors
jonatack practicalswift klementtan sipa laanwj vasild DrahtBot

Review Requested
vasild

Labels
Refactoring P2P Needs rebase