Remove unused CSubNet serialize code #22375

1. 
   MarcoFalke commented at 9:43 am on June 30, 2021: member

   The code is complicated and confusing. Now that it is unused, remove it to avoid the risk of using it later.
2. fanquake added the label P2P on Jun 30, 2021
3. Remove unused CSubNet serialize code fa0dff59cd
4. MarcoFalke force-pushed on Jun 30, 2021
5. MarcoFalke added the label Refactoring on Jun 30, 2021
6. 
   jonatack commented at 1:34 pm on July 15, 2021: member

   ACK fa0dff59cd9d363222f5c6825a3850f1a3b532d3 code review, debug-built and tested after rebasing to master, ran unit tests and sub_net_deserialize fuzzer briefly

   0$ FUZZ=sub_net_deserialize src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/sub_net_deserialize
   1.../...
   2[#103509](/bitcoin-bitcoin/103509/)	REDUCE cov: 352 ft: 544 corp: 51/1064b lim: 932 exec/s: 7393 rss: 205Mb L: 20/129 MS: 2 ShuffleBytes-EraseBytes-
   3[#131072](/bitcoin-bitcoin/131072/)	pulse  cov: 352 ft: 544 corp: 51/1064b lim: 1201 exec/s: 7710 rss: 224Mb
   4[#150330](/bitcoin-bitcoin/150330/)	REDUCE cov: 352 ft: 544 corp: 51/1063b lim: 1391 exec/s: 7516 rss: 237Mb L: 6/129 MS: 1 EraseBytes-
   5[#185891](/bitcoin-bitcoin/185891/)	REDUCE cov: 353 ft: 546 corp: 52/1085b lim: 1741 exec/s: 7745 rss: 261Mb L: 22/129 MS: 1 CMP- DE: "\xfd\x87\xd8~\xebC"-
   6[#262144](/bitcoin-bitcoin/262144/)	pulse  cov: 353 ft: 546 corp: 52/1085b lim: 2491 exec/s: 7710 rss: 313Mb
   7[#524288](/bitcoin-bitcoin/524288/)	pulse  cov: 353 ft: 546 corp: 52/1085b lim: 5105 exec/s: 8322 rss: 489Mb
   8[#1048576](/bitcoin-bitcoin/1048576/) pulse  cov: 353 ft: 546 corp: 52/1085b lim: 10320 exec/s: 8192 rss: 522Mb
   

7. laanwj added this to the "Blockers" column in a project

   ---

8. 
   practicalswift commented at 7:44 pm on July 24, 2021: contributor

   Concept ACK

   Unused code should be removed.

9. theStack approved
10. 
    theStack commented at 9:06 am on July 25, 2021: member

    Code-review ACK fa0dff59cd9d363222f5c6825a3850f1a3b532d3
11. 
    DrahtBot commented at 4:25 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.

12. in src/test/fuzz/deserialize.cpp:154 in fa0dff59cd

    158+            int version;
    159+            ds >> version;
    160+            ds.SetVersion(version);
    161+        }
    162+        ds >> CSubNet{};
    163+    } catch (const std::ios_base::failure&) {
    

    ---

    ---

    

    vasild commented at 7:25 am on July 28, 2021:

    nit: why the extra {}?

    ---

    

    MarcoFalke commented at 7:53 am on July 28, 2021:

    to limit the scope of version and to put it in its own section. An alternative would be add a newline after ds.SetVersion and not care about the scope.
13. vasild approved
14. 
    vasild commented at 7:33 am on July 28, 2021: member

    ACK fa0dff59cd9d363222f5c6825a3850f1a3b532d3
15. 
    vasild commented at 7:37 am on July 28, 2021: member

    The Unserialize() method can be removed once we stop reading banlist.dat, in the next version, I guess… Actually, given that 22.0 is branched of and this change, after being merged to master, will only be released in 23.0, wouldn’t it be easier to drop Serialize(), Unserialize() and banlist.dat reading support now?
16. 
    MarcoFalke commented at 5:29 pm on July 28, 2021: member

    Done in #22570
17. MarcoFalke removed this from the "Blockers" column in a project

    ---

18. 
    laanwj commented at 2:25 pm on August 1, 2021: member

    Concept ACK (as it doesn’t get rid of a lot of code like this, I have a slight preference for removing the serialization and deserialization code at the same time, but no opposition to doing it like this either)
19. 
    MarcoFalke commented at 2:32 pm on August 1, 2021: member

    Indeed.

    Please review (ACK/NACK) #22570 first.

20. DrahtBot added the label Needs rebase on Aug 2, 2021
21. 
    DrahtBot commented at 11:31 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”.

22. MarcoFalke closed this on Aug 2, 2021

    ---

23. MarcoFalke deleted the branch on Aug 2, 2021
24. DrahtBot locked this on Aug 18, 2022

Contributors
MarcoFalke jonatack practicalswift theStack DrahtBot vasild laanwj

Labels
Refactoring P2P Needs rebase