Skip to content

banman: save the banlist in a JSON format on disk #20966

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 4 commits into from
Jun 23, 2021
Merged

banman: save the banlist in a JSON format on disk #20966

maflcko merged 4 commits into from
Jun 23, 2021

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Jan 19, 2021 •
edited by maflcko
Loading

Save the banlist in banlist.json instead of banlist.dat.

This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).

Only read banlist.dat if it exists and banlist.json does not exist (first start after an upgrade).

Supersedes #20904
Resolves #19748

This was referenced Jan 19, 2021
Closed
Closed
Dusto-beep
Dusto-beep suggested changes Jan 19, 2021
View reviewed changes
Copy link

@Dusto-beep Dusto-beep left a comment •
edited by laanwj
Loading

Choose a reason for hiding this comment

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

(off topic reply deleted)

@laanwj laanwj added the P2P label Jan 19, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 20, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

This was referenced Jan 20, 2021
Closed
Merged
@theStack
Copy link
Contributor

theStack commented Jan 20, 2021

Concept ACK

@vasild
Copy link
Contributor Author

vasild commented Jan 20, 2021 •
edited
Loading

790727eea..fd751850e: add the tests from #20904 (closed without merge), they are precious

@vasild
Copy link
Contributor Author

vasild commented Jan 20, 2021

fd751850e..2d287f639: add release notes

maflcko
maflcko reviewed Jan 20, 2021
View reviewed changes
doc/release-notes.md Outdated
Copy link
Member

@maflcko maflcko Jan 20, 2021

Choose a reason for hiding this comment

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

Please add the release notes to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/%2320852-Release-notes-snippet

Otherwise they will have to be removed here after merge, then added to the 0.21 branch. (Release notes are only added to the point releases, not the next major version that also includes those fixes)

Copy link
Contributor Author

@vasild vasild Jan 20, 2021

Choose a reason for hiding this comment

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

I guess after/if this PR is merged?

@maflcko maflcko added this to the 0.21.1 milestone Jan 20, 2021
@laanwj
Copy link
Member

laanwj commented Jan 20, 2021

Concept ACK

@vasild
Copy link
Contributor Author

vasild commented Jan 20, 2021

2d287f639...fd751850e: remove edits of doc/release-notes.md

@vasild
Copy link
Contributor Author

vasild commented Jan 20, 2021

Notes:

  • I guess it would be possible to write the ban list to settings.json instead of in banlist.json (as originally proposed in Deprecate banlist.dat #19748), but the periodic dump may create some complications and also it is unclear what to do if we are running with -nosettings.

  • The current patch would write banlist.json on the first change of the ban list. This means that a node that upgrades from before this PR with only banlist.dat on disk may not create banlist.json for some time. I think this is ok.

This was referenced Jan 20, 2021
Closed
Closed
promag
promag reviewed Jan 27, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

Should try to read JSON first, and if it exists then skip old format? Or am I missing something?

Personally I wouldn't add from/to JSON in CBanEntry, BanMapFromJson and BanMapToJson seem enough. Also, there should be some validation in BanMapFromJson?

@jonatack
Copy link
Member

jonatack commented Jan 27, 2021

Concept ACK

790727e..fd75185: add the tests from #20904 (closed without merge), they are precious

Good to see these tests brought in.

@vasild
Copy link
Contributor Author

vasild commented Jan 28, 2021

fd751850e...e74196741: read banlist.dat only if banlist.json is not present and catch all exceptions that could come from an invalid JSON.

@promag

Should try to read JSON first, and if it exists then skip old format?

I made it to read both if present so that if the user temporary downgrades and makes some changes to banlist.dat those changes would be picked up by the new version. However this has the deficiency that if an entry is deleted from banlist.json it will keep being loaded from banlist.dat. Changed to what you suggest above.

Personally I wouldn't add from/to JSON in CBanEntry, BanMapFromJson and BanMapToJson seem enough.

CBanEntry being able to ser/unser itself to/from JSON improves its encapsulation.

Also, there should be some validation in BanMapFromJson?

Right! Added try/catch to handle invalid JSONs.

@DrahtBot DrahtBot mentioned this pull request Feb 13, 2021
Merged
@maflcko
Copy link
Member

maflcko commented Feb 15, 2021

needs rebase after #21167

jonatack
jonatack reviewed Jun 14, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

re-ACK 4b52c7234f3c2e3067d7d6c6fd7ebf2b96bb8a45 per git range-diff 5c4f0c4 88b843e 4b52c72, rebase only, debug build clean, test passes, repeatedly tested banning/unbanning peers on signet while inspecting banlist.json in my editor including through restarts. However, there are gotchas when downgrading/upgrading; it may be a good idea to warn and suggest best practices about that in the release note.

@jonatack
Copy link
Member

jonatack commented Jun 14, 2021 •
edited
Loading

  • The list of banned hosts and networks (via setban RPC) is now saved on disk
    in JSON format in banlist.json instead of banlist.dat. banlist.dat is
    only read on startup if banlist.json is not present. Changes are only
    written to the new banlist.json. A future version of Bitcoin Core may
    completely ignore banlist.dat. (banman: save the banlist in a JSON format on disk #20966)

Regarding the release note, peers may be banned and unbanned via the GUI peers window in addition to the setban RPC. As mentioned in my previous comment, it may be a good idea to describe user gotchas and best practices for upgrading and downgrading.

@sipa
Copy link
Member

sipa commented Jun 15, 2021

utACK 4b52c7234f3c2e3067d7d6c6fd7ebf2b96bb8a45

I think it would be useful to make the code ignore future version numbers, so it will not completely wipe banlists on downgrading (from future versions that introduce additional features to this format), but that can be done later.

promag
promag reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK 4b52c7234f3c2e3067d7d6c6fd7ebf2b96bb8a45.

@maflcko maflcko modified the milestones: 0.21.2, 22.0 Jun 21, 2021
vasild added 4 commits June 21, 2021 14:39
@vasild
banman: save the banlist in a JSON format on disk
d197977 
Save the banlist in `banlist.json` instead of `banlist.dat`.

This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).

Only read `banlist.dat` if it exists and `banlist.json` does not
exist (first start after an upgrade).

Supersedes bitcoin#20904
Resolves bitcoin#19748
@vasild
test: ensure banlist can be read from disk after restart
dd4e957 
With `banlist.dat` (being written in addrv1 format) if we would try to
write a Tor v3 subnet, it would serialize as a dummy-all-0s IPv6
address and subsequently, when deserialized will not result in the same
subnet.

This problem does not exist with `banlist.json` where the data is saved
in textual, human-readable form.
@vasild
doc: fix grammar in doc/files.md
24b10eb
@vasild
style: remove () from assert in rpc_setban.py
bb719a0
@vasild
Copy link
Contributor Author

vasild commented Jun 21, 2021

4b52c7234f...5fcc96457d: rebase due to conflicts

@vasild
Copy link
Contributor Author

vasild commented Jun 21, 2021

5fcc96457d...bb719a08db: minor tweaks in rpc_setban.py - log a message and use node instead of self.nodes[1]

@jonatack
Copy link
Member

jonatack commented Jun 21, 2021

Code review re-ACK bb719a0 per git range-diff 6a67366 4b52c72 bb719a0

@achow101
Copy link
Member

achow101 commented Jun 22, 2021

Code Review ACK bb719a0

@maflcko maflcko merged commit d6e0d78 into bitcoin:master Jun 23, 2021
@vasild vasild deleted the json_bans branch June 23, 2021 08:04
@maflcko
Copy link
Member

maflcko commented Jun 23, 2021

Removed from backport, but if someone wants to backport, it will need to be done together with #20852 (comment)

@maflcko
Copy link
Member

maflcko commented Jun 23, 2021

Added to release notes: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/22.0-Release-Notes-draft/_history

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 24, 2021
Merge bitcoin#20966: banman: save the banlist in a JSON format on disk
620c144
@maflcko maflcko mentioned this pull request Jun 28, 2021
Merged
laanwj added a commit to bitcoin-core/gui that referenced this pull request Dec 15, 2021
@laanwj
Merge bitcoin/bitcoin#22362: Drop only invalid entries when reading b…
2d0bdb2 
…anlist.json

faa6c3d net: Drop only invalid entries when reading banlist.json (MarcoFalke)

Pull request description:

  All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.

  Also suggested in bitcoin/bitcoin#20966 (comment)

ACKs for top commit:
  laanwj:
    Re-ACK faa6c3d

Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 15, 2021
@laanwj
Merge bitcoin#22362: Drop only invalid entries when reading banlist.json
a210f0c 
faa6c3d net: Drop only invalid entries when reading banlist.json (MarcoFalke)

Pull request description:

  All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.

  Also suggested in bitcoin#20966 (comment)

ACKs for top commit:
  laanwj:
    Re-ACK faa6c3d

Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge d6e0d78 into merged_master (Bitcoin PR bitcoin/bitcoin#20966)
e8b84fa
@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.

Reviewers

@hebasto hebasto hebasto approved these changes

+5 more reviewers

@dhruv dhruv dhruv left review comments

@jonatack jonatack jonatack left review comments

@promag promag promag left review comments

@maflcko maflcko maflcko approved these changes

@Dusto-beep Dusto-beep Dusto-beep requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

22.0

Development

Successfully merging this pull request may close these issues.

Deprecate banlist.dat