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

i2p: log connection was refused due to arbitrary port #29393

Merged
merged 1 commit into from Mar 9, 2024

Conversation

brunoerg
Copy link
Contributor

@brunoerg brunoerg commented Feb 6, 2024

For I2P, we do not try to connect if port is != 0. However, we do not have anything that indicates it or any error when trying to connect with port != 0. This PR adds a log for it. Also, it improves the functional test. With this log we can ensure the reason we won't connect is the port, in the current test, we cannot ensure it.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 6, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK jonatack, kristapsk, epiccurious, vasild, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@epiccurious
Copy link

Concept ACK 950f360.

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

cr utACK 950f360

@epiccurious
Copy link

Tested ACK 950f360.

@DrahtBot DrahtBot removed the request for review from epiccurious February 7, 2024 02:02
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.

Approach ACK

src/i2p.cpp Outdated Show resolved Hide resolved
@brunoerg
Copy link
Contributor Author

brunoerg commented Feb 8, 2024

Force-pushed addressing #29393 (comment)

@jonatack
Copy link
Contributor

jonatack commented Feb 8, 2024

ACK 5b358cd

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

re-ACK 5b358cd

@DrahtBot DrahtBot requested review from epiccurious and removed request for epiccurious February 8, 2024 23:01
@epiccurious
Copy link

re-ACK 5b358cd.

@DrahtBot DrahtBot removed the request for review from epiccurious February 9, 2024 14:58
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 5b358cd

How did you come to this? Did you encounter some !=0 I2P ports in the wild? I don't have any in my addrman:

$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.address |test("\\.i2p$"; "i"))) |map(select(.port != 0)) |length'
0

$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.address |test("\\.i2p$"; "i"))) |map(select(.port == 0)) |length'
1605

src/i2p.cpp Show resolved Hide resolved
@brunoerg
Copy link
Contributor Author

brunoerg commented Mar 1, 2024

How did you come to this? Did you encounter some !=0 I2P ports in the wild?

I didn't encounter in practice, in fact, I was testing some nuances of i2p and tried to add it using addnode to check the behaviour, then I noticed that. Reading the functional test I realized that the way we were checking it was weird and did not really ensure the problem is the port.

@achow101
Copy link
Member

achow101 commented Mar 9, 2024

ACK 5b358cd

@achow101 achow101 merged commit a78ca70 into bitcoin:master Mar 9, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants