Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore banlist.dat #22570

Merged
merged 3 commits into from
Aug 2, 2021
Merged

Ignore banlist.dat #22570

merged 3 commits into from
Aug 2, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 28, 2021

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 Outdated Show resolved Hide resolved
@Zero-1729
Copy link
Contributor

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 29, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor

Concept ACK

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/addrdb.cpp Outdated Show resolved Hide resolved
src/addrdb.cpp Show resolved Hide resolved
src/netaddress.h Show resolved Hide resolved
@maflcko
Copy link
Member Author

maflcko commented Jul 29, 2021

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

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

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fac51e9

Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fac51e9

@vasild
Copy link
Contributor

vasild commented Jul 30, 2021

Further cleanup is possible:

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

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

MarcoFalke added 3 commits July 30, 2021 11:21
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
Copy link
Contributor

re-ACK fa1eddb

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa1eddb

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light code review utACK fa1eddb

@laanwj
Copy link
Member

laanwj commented Aug 2, 2021

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

@laanwj laanwj merged commit efd6f90 into bitcoin:master Aug 2, 2021
@maflcko maflcko deleted the 2107-noSerSubnet branch August 2, 2021 14:11
@maflcko
Copy link
Member Author

maflcko commented Aug 2, 2021

Release notes for 23.0 added in #22603

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 4, 2021
….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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021
….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
Copy link
Member Author

maflcko commented Aug 7, 2021

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

maflcko pushed a commit that referenced this pull request Sep 6, 2021
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants