Skip to content

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Jul 19, 2021

CAddrMan::ResetI2PPorts() was temporary. Remove it:

-BEGIN VERIFY SCRIPT-
git show e0a2b39 |git apply -R
-END VERIFY SCRIPT-

Fixes #22467
Fixes #22470

@vasild
Copy link
Contributor Author

vasild commented Jul 19, 2021

This PR supersedes (makes obsolete):
#22468
#22471

`CAddrMan::ResetI2PPorts()` was temporary. Remove it:
* it has partially achieved its goal: probably ran on about half of the
  I2P nodes
* it is hackish, deemed risky and two bugs where found in it
  bitcoin#22467
  bitcoin#22470

-BEGIN VERIFY SCRIPT-
git show e0a2b39 |git apply -R
-END VERIFY SCRIPT-

Fixes bitcoin#22467
Fixes bitcoin#22470
@vasild vasild force-pushed the remove_ResetI2PPorts branch from fe67f03 to d4b67c8 Compare July 19, 2021 12:34
Copy link
Member

@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.

@maflcko maflcko added this to the 22.0 milestone Jul 19, 2021
@laanwj
Copy link
Member

laanwj commented Jul 19, 2021

ACK d4b67c8

@maflcko
Copy link
Member

maflcko commented Jul 19, 2021

review ACK d4b67c8 😲

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK d4b67c8ebc2bb7488bcaaccc3a801cdef1cf1678 😲
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg6zgv/ULh0hXhpAy7WL5DBG3Xn76U2HO3veHWtNVy64a4Djut74lceEGUsn+SA
PD+5wM7jadH3O17vV/Pl6nh5YhZvgzgG1F2o+R+abK0Xu27agXI6F3utWr72GtvL
tKEITPpCESRgmC4GpP38hoydi9tKM2fyqAm2wUReD0TGgIhJfNYS9p4nfbP+HdGf
3h709FtciYB5ANT6SXZ4U3+qhYBdX5FsnT3Un+w6KzsyfBKhfY1BZcxK/bN7SDsj
2uxeZ+a58/mNjlzQ2Lm+5X7PZRBGL9Q3+5RkImQj40KCxA16/r3WVIxNbiqLJO0p
4RLUZ/A4lqUCw2i8+RL7NbU4rW61Xk5rT4FHhCrlL+qoG7xRtcvbAB6ZfU8EQaf1
ybdNJ6v//MjtZFbxH899GiEpELq5m/qhhFoPlCtWeJ/hEq+aSz17yys8ZPFSV+cp
1LUHstYGFQotCal8vS93+gIketmOfl/8e0W49wjGCTdIvF3xoe5Loh1aAW/IFquo
AOWaqAtG
=jJjT
-----END PGP SIGNATURE-----

Timestamp of file with hash 306e37b9caea4ca3333d35e458fad27fb95264adcc2faf9318af45c358d9e9cd -

@laanwj
Copy link
Member

laanwj commented Jul 19, 2021

Looks like #22496 is a similar PR, but it removes somewhat more, also RemoveInvalid, and prevents I2P addresses with non-zero ports from being inserted. Might be best to rebase it on top of this (which is a clean revert).

@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.

@fanquake fanquake merged commit 624a193 into bitcoin:master Jul 20, 2021
@vasild vasild deleted the remove_ResetI2PPorts branch July 20, 2021 05:49
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021
…a2b39)

d4b67c8 scripted-diff: remove ResetI2PPorts() (revert e0a2b39) (Vasil Dimov)

Pull request description:

  `CAddrMan::ResetI2PPorts()` was temporary. Remove it:
  * it has partially achieved its goal: probably ran on about half of the
    I2P nodes
  * it is hackish, deemed risky and two bugs where found in it:
    bitcoin#22467
    bitcoin#22470

  -BEGIN VERIFY SCRIPT-
  git show e0a2b39 |git apply -R
  -END VERIFY SCRIPT-

  Fixes bitcoin#22467
  Fixes bitcoin#22470

ACKs for top commit:
  laanwj:
    ACK d4b67c8
  MarcoFalke:
    review ACK d4b67c8 😲
  jonatack:
    ACK d4b67c8 per IRC discussions https://www.erisian.com.au/bitcoin-core-dev/log-2021-07-16.html#l-212 and https://www.erisian.com.au/bitcoin-core-dev/log-2021-07-19.html#l-210

Tree-SHA512: 60d8f0ea0f66a8fcedfcb9c8944a419b974b15509b54ddfeec58db49ae9418e6916df712bba3fbd6b29497d85f7951fb9aa2e48eb9c59f88d09435685bd00b4c
fanquake added a commit that referenced this pull request Aug 3, 2021
65332b1 [addrman] Remove RemoveInvalid() (John Newbery)

Pull request description:

  PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs:

  - #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed)
  - #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets)

  Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants.

  Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`.

ACKs for top commit:
  sipa:
    utACK 65332b1. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any.
  fanquake:
    ACK 65332b1 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code.

Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 22, 2022
Summary:
> net: change assumed I2P port to 0
>
> * When accepting an I2P connection, assume the peer has port 0 instead
>   of the default 8333 (for mainnet). It is not being sent to us, so we
>   must assume something.
> * When deriving our own I2P listen CService use port 0 instead of the
>   default 8333 (for mainnet). So that we later advertise it to peers
>   with port 0.
>
> In the I2P protocol SAM 3.1 and older (we use 3.1) ports are not used,
> so they are irrelevant. However in SAM 3.2 and newer ports are used and
> from the point of view of SAM 3.2, a peer using SAM 3.1 seems to have
> specified port=0.

> net: distinguish default port per network
>
> Change `CChainParams::GetDefaultPort()` to return 0 if the network is
> I2P.

> net: do not connect to I2P hosts on port!=0
>
> When connecting to an I2P host we don't specify destination port and it
> is being forced to 0 by the SAM 3.1 proxy, so if we connect to the same
> host on two different ports, that would be actually two connections to
> the same service (listening on port 0).
>
> Fixes bitcoin/bitcoin#21389

> test: ensure I2P ports are handled as expected

> doc: mention that we enforce port=0 in I2P
>
> Co-authored-by: Jon Atack <[email protected]>

This is a backport of [[bitcoin/bitcoin#22112 | core#22112]] and [[bitcoin/bitcoin#22497 | core#22497]] (reverts all changes to `addrman`  and `addrman_tests.cpp`)

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11083
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 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.

Projects

None yet

6 participants