-
Notifications
You must be signed in to change notification settings - Fork 38.7k
scripted-diff: remove ResetI2PPorts() (revert e0a2b390c14) #22497
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
Conversation
`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
fe67f03 to
d4b67c8
Compare
jonatack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
ACK d4b67c8 |
|
review ACK d4b67c8 😲 Show signature and timestampSignature: Timestamp of file with hash |
|
Looks like #22496 is a similar PR, but it removes somewhat more, also |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
…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
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
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
CAddrMan::ResetI2PPorts()was temporary. Remove it:I2P nodes
Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed #22467
Changing I2P ports in addrman may wrongly skip some entries from "new" buckets #22470
-BEGIN VERIFY SCRIPT-
git show e0a2b39 |git apply -R
-END VERIFY SCRIPT-
Fixes #22467
Fixes #22470