Remove unused CSubNet serialize code #22375

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2106-serSubNo changing 2 files +19 −17

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.
fanquake added the label P2P on Jun 30, 2021
Remove unused CSubNet serialize code fa0dff59cd
MarcoFalke force-pushed on Jun 30, 2021
MarcoFalke added the label Refactoring on Jun 30, 2021

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

$ FUZZ=sub_net_deserialize src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/sub_net_deserialize
.../...
[#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-
[#131072](/bitcoin-bitcoin/131072/)	pulse  cov: 352 ft: 544 corp: 51/1064b lim: 1201 exec/s: 7710 rss: 224Mb
[#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-
[#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"-
[#262144](/bitcoin-bitcoin/262144/)	pulse  cov: 353 ft: 546 corp: 52/1085b lim: 2491 exec/s: 7710 rss: 313Mb
[#524288](/bitcoin-bitcoin/524288/)	pulse  cov: 353 ft: 546 corp: 52/1085b lim: 5105 exec/s: 8322 rss: 489Mb
[#1048576](/bitcoin-bitcoin/1048576/) pulse  cov: 353 ft: 546 corp: 52/1085b lim: 10320 exec/s: 8192 rss: 522Mb

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

practicalswift commented at 7:44 PM on July 24, 2021: contributor

Concept ACK

Unused code should be removed.
theStack approved
theStack commented at 9:06 AM on July 25, 2021: member

Code-review ACK fa0dff59cd9d363222f5c6825a3850f1a3b532d3
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.
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.
vasild approved
vasild commented at 7:33 AM on July 28, 2021: member

ACK fa0dff59cd9d363222f5c6825a3850f1a3b532d3
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?
MarcoFalke commented at 5:29 PM on July 28, 2021: member

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

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)
MarcoFalke commented at 2:32 PM on August 1, 2021: member

Indeed.

Please review (ACK/NACK) #22570 first.
DrahtBot added the label Needs rebase on Aug 2, 2021
DrahtBot commented at 11:31 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>
MarcoFalke closed this on Aug 2, 2021
MarcoFalke deleted the branch on Aug 2, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Refactoring P2P Needs rebase

Remove unused CSubNet serialize code #22375

Conflicts