Skip to content

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Jan 11, 2021

This is a followup to #20852
which allowed non-IP subnets, but some of them, e.g. torv3, cannot be
serialized in 16 bytes (addrv1) and must use addrv2.

This PR changes the format of banlist.dat in such a way that old
versions (before this commit) will not be able to read a file written by
new versions (after this commit).

This is a followup to bitcoin#20852
which allowed non-IP subnets, but some of them, e.g. torv3, cannot be
serialized in 16 bytes (addrv1) and must use addrv2.

This commit changes the format of `banlist.dat` in such a way that old
versions (before this commit) will not be able to read a file written by
new versions (after this commit).
If we would try to write a torv3 subnet in addrv1 format, it would
serialize as a dummy-all-0s IPv6 address and subsequently, when
deserialized will not result in the same subnet.
Change the `peer_discouragement` test to use `CNode` pointers so that
the nodes it uses can be added to `CConnman::vNodes` and cleaned up
properly. Make it use `CConnmanTest` instead of `CConnman`. This is
needed because we want to check `CNode::fDisconnect` and for this flag
to be flipped by `CConnman::DisconnectNode()` the node must be in
`CConnman::vNodes`.

Extend the test with one torv3 peer and check that it is discouraged and
disconnected as expected.
@vasild
Copy link
Contributor Author

vasild commented Jan 11, 2021

TODO:

  • See if we ser/deser CSubNet somewhere else in the code, other than in BanMan and if yes, then what are the implications for that other code.
  • See what is the relationship between this and other changes that alter the format of banlist.dat, e.g. rpc: simpler setban and new ban manipulation commands #19825.

Copy link
Member

@maflcko maflcko 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, didn't look at the code changes

@DrahtBot
Copy link
Contributor

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.

@sipa
Copy link
Member

sipa commented Jan 11, 2021

How does this maintain forward compatibility (new versions being able to read subnets written by old versions)?

@vasild
Copy link
Contributor Author

vasild commented Jan 12, 2021

How does this maintain forward compatibility (new versions being able to read subnets written by old versions)?

I does not, but it should.

@vasild vasild mentioned this pull request Jan 13, 2021
vasild added a commit to vasild/bitcoin that referenced this pull request Jan 19, 2021
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).

Still read `banlist.dat` (if it exists) and merge its contents with
`banlist.json` (if it exists). This is because we need to read
`banlist.dat` at least once in order to convert it and it is easier to
read it every time (not just once).

Supersedes bitcoin#20904
Resolves bitcoin#19748
@vasild
Copy link
Contributor Author

vasild commented Jan 19, 2021

Closing in favor of #20966.

@vasild vasild closed this Jan 19, 2021
@bitcoin bitcoin deleted a comment Jan 19, 2021
vasild added a commit to vasild/bitcoin that referenced this pull request Jan 28, 2021
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).

Still read `banlist.dat` (if it exists) and merge its contents with
`banlist.json` (if it exists). This is because we need to read
`banlist.dat` at least once in order to convert it and it is easier to
read it every time (not just once).

Supersedes bitcoin#20904
Resolves bitcoin#19748
vasild added a commit to vasild/bitcoin that referenced this pull request Feb 15, 2021
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).

Still read `banlist.dat` (if it exists) and merge its contents with
`banlist.json` (if it exists). This is because we need to read
`banlist.dat` at least once in order to convert it and it is easier to
read it every time (not just once).

Supersedes bitcoin#20904
Resolves bitcoin#19748
vasild added a commit to vasild/bitcoin that referenced this pull request Mar 10, 2021
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).

Still read `banlist.dat` (if it exists) and merge its contents with
`banlist.json` (if it exists). This is because we need to read
`banlist.dat` at least once in order to convert it and it is easier to
read it every time (not just once).

Supersedes bitcoin#20904
Resolves bitcoin#19748
vasild added a commit to vasild/bitcoin that referenced this pull request Mar 15, 2021
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).

Still read `banlist.dat` (if it exists) and merge its contents with
`banlist.json` (if it exists). This is because we need to read
`banlist.dat` at least once in order to convert it and it is easier to
read it every time (not just once).

Supersedes bitcoin#20904
Resolves bitcoin#19748
@fanquake fanquake removed this from the 0.21.1 milestone Mar 24, 2021
vasild added a commit to vasild/bitcoin that referenced this pull request Mar 24, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Apr 6, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Apr 6, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Apr 19, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Apr 20, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Apr 20, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Apr 26, 2021
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 added a commit to vasild/bitcoin that referenced this pull request May 18, 2021
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 added a commit to vasild/bitcoin that referenced this pull request May 24, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Jun 14, 2021
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 added a commit to vasild/bitcoin that referenced this pull request Jun 21, 2021
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
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jun 23, 2021
…t on disk

bb719a0 style: remove () from assert in rpc_setban.py (Vasil Dimov)
24b10eb doc: fix grammar in doc/files.md (Vasil Dimov)
dd4e957 test: ensure banlist can be read from disk after restart (Vasil Dimov)
d197977 banman: save the banlist in a JSON format on disk (Vasil Dimov)

Pull request description:

  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/bitcoin#20904
  Resolves bitcoin/bitcoin#19748

ACKs for top commit:
  jonatack:
    Code review re-ACK bb719a0 per `git range-diff 6a67366 4b52c72 bb719a0`
  achow101:
    Code Review ACK bb719a0

Tree-SHA512: fc135c3a1fe20bcf5d008ce6bea251b4135e56c78bf8f750b4bd8144c095b81ffe165133cdc7e4715875eec7e7c4e13ad9f5d2450b21102af063d7c8abf716b6
josibake pushed a commit to josibake/bitcoin that referenced this pull request Jul 21, 2021
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
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Nov 5, 2021
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/bitcoin#20904
Resolves bitcoin/bitcoin#19748
stickies-v pushed a commit to stickies-v/bitcoin-devwiki that referenced this pull request Mar 24, 2022
@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.

5 participants