Ignore banlist.dat by maflcko · Pull Request #22570 · bitcoin/bitcoin

maflcko · 2021-07-28T17:27:50Z

The code to read banlist.dat should be removed eventually. The major release (22.x) can be used to translate a banlist.dat into a banlist.json. Thus, it is now possible to remove the reading code.

src/addrdb.cpp

Zero-1729 · 2021-07-29T01:02:19Z

Concept ACK, but agree with @fanquake's comment regarding LogPrintf() and adding a terminal newline.

DrahtBot · 2021-07-29T03:39:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove unused CSubNet serialize code #22375 by MarcoFalke
Drop only invalid entries when reading banlist.json #22362 by MarcoFalke
p2p, refactor: remove unneeded CNetAddr::UnserializeV1Array() #22140 by jonatack
Implement BIP 370 PSBTv2 #21283 by achow101
addrman: Make consistency checks a runtime option #20233 by jnewbery

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.

practicalswift · 2021-07-29T08:41:46Z

Concept ACK

vasild

Approach ACK

Just to confirm that my understanding is correct: if this is merged now in master, it will be included in 23.0 but not in 22.1, right?

doc/files.md

src/addrdb.cpp

src/netaddress.h

maflcko · 2021-07-29T10:08:59Z

Just to confirm that my understanding is correct: if this is merged now in master, it will be included in 23.0 but not in 22.1, right?

Yes, the pull request is to the master branch. 22.x is a different branch: https://github.com/bitcoin/bitcoin/tree/22.x

vasild

ACK fac51e94ff2781de70ad4e8d3b4868a2d148d237

Zero-1729

ACK fac51e94ff2781de70ad4e8d3b4868a2d148d237

vasild · 2021-07-30T08:47:34Z

Further cleanup is possible:

bitcoin/src/test/fuzz/banman.cpp

Line 55 in 787296e

const std::string sfx{fuzzed_data_provider.ConsumeBool() ? ".dat" : ".json"};

bitcoin/src/test/fuzz/banman.cpp

Line 117 in 787296e

fs::remove(banlist_file.string() + ".dat");

This also allows to remove the "dirty" argument, which can now be deduced from the return value of Read().

Leaving the incorrect indentation would be frustrating because: * Some editor may fix up the whitespace when editing a file, so before commiting the whitespace changes need to be undone. * It makes it harder to use clang-format-diff on a change. Can be trivially reviewed with --word-diff-regex=. --ignore-all-space

Zero-1729 · 2021-07-30T09:30:36Z

re-ACK fa1eddb

vasild

ACK fa1eddb

jonatack

Light code review utACK fa1eddb

laanwj · 2021-08-02T10:43:03Z

Not a big fan of the huge whitespace change in the last commit, but otherwise
concept and code review ACK fa1eddb

maflcko · 2021-08-02T14:20:41Z

Release notes for 23.0 added in #22603

….dat) fa2c868 doc: Add release notes for 22570 (ignore banlist.dat) (MarcoFalke) Pull request description: Follow-up to bitcoin#22570 ACKs for top commit: tryphe: ACK fa2c868 vasild: ACK fa2c868 Zero-1729: ACK fa2c868 Tree-SHA512: 58eef340b6211bcdecf3826ac145afd476c397f110daa6783521c0c1e1be1ffbd2d24ad20c77921abbe5cdb8e644d3cd64e14e2819746cf0e5123fb7cc445d63

maflcko · 2021-08-07T08:23:57Z

Looks like we can re-enable the disabled assert again due to the removal of the code here: #22657

6919c82 MOVEONLY: Expose BanMapToJson / BanMapFromJson (Russell Yanofsky) Pull request description: This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). --- CSubNet serialization code that was removed in #22570 fa4e6af was needed by multiprocess code to share ban map between gui and node processes. Rather than adding it back, use suggestion from MarcoFalke #10102 (comment) to use JSON serialization. This requires making BanMapToJson / BanMapFromJson functions public. ACKs for top commit: promag: reACK 6919c82. Tree-SHA512: ce909a61b7869d16cf2e9f91b643dd9d2604efc5777703d3b77a4c40cb0ccdd20396ba87b1ec85aade142e12ff9ea4c95c7155840354873579565471779f5a33

maflcko added the P2P label Jul 28, 2021

maflcko mentioned this pull request Jul 28, 2021

Remove unused CSubNet serialize code #22375

Closed

fanquake reviewed Jul 29, 2021

View reviewed changes

src/addrdb.cpp Outdated Show resolved Hide resolved

This was referenced Jul 29, 2021

Drop only invalid entries when reading banlist.json #22362

Merged

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

Closed

maflcko force-pushed the 2107-noSerSubnet branch from fa08473 to fa718ea Compare July 29, 2021 06:33

vasild reviewed Jul 29, 2021

View reviewed changes

doc/files.md Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

src/netaddress.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2107-noSerSubnet branch 2 times, most recently from fa81dae to fac51e9 Compare July 29, 2021 10:47

vasild approved these changes Jul 29, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 29, 2021

Implement BIP 370 PSBTv2 #21283

Open

Zero-1729 approved these changes Jul 29, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 29, 2021

addrman: Make consistency checks a runtime option #20233

Closed

MarcoFalke added 3 commits July 30, 2021 11:21

Ignore banlist.dat

fa384fd

This also allows to remove the "dirty" argument, which can now be deduced from the return value of Read().

Remove unused CSubNet serialize code

fa4e6af

maflcko force-pushed the 2107-noSerSubnet branch from fac51e9 to fa1eddb Compare July 30, 2021 09:25

vasild approved these changes Jul 30, 2021

View reviewed changes

jonatack reviewed Jul 30, 2021

View reviewed changes

Zero-1729 approved these changes Jul 31, 2021

View reviewed changes

laanwj merged commit efd6f90 into bitcoin:master Aug 2, 2021

maflcko deleted the 2107-noSerSubnet branch August 2, 2021 14:11

maflcko mentioned this pull request Aug 2, 2021

doc: Add release notes for 22570 (ignore banlist.dat) #22603

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021

Merge bitcoin#22570: Ignore banlist.dat

1273a52

ryanofsky mentioned this pull request Aug 18, 2021

Multiprocess bitcoin #10102

Draft

ryanofsky mentioned this pull request Aug 31, 2021

MOVEONLY: Expose BanMapToJson / BanMapFromJson #22848

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

maflcko commented Jul 28, 2021

Uh oh!

Uh oh!

Zero-1729 commented Jul 29, 2021

Uh oh!

DrahtBot commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Jul 29, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jul 29, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Jul 30, 2021

Uh oh!

Zero-1729 commented Jul 30, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 2, 2021

Uh oh!

maflcko commented Aug 2, 2021

Uh oh!

maflcko commented Aug 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Jul 29, 2021 •

edited

Loading