init: adding check for : for -torcontrol flag #28018
Conversation
right now for -torcontrol you can pass in any string as long as it doesn't have a : but that is invalid. I added an additional check before calling SplitHostPort to see if there is a :
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
|
|
Overall, I think the code is good. However, there have been some earlier remarks in this comment on where to place this code. Someone with a better understanding of the entire codebase should comment on the placement of this. In any case, I'm testing it locally and will report back on whether it meets the requirements soon (one suggestion I think you should avoid using |
Yea I could totally move the logic to a util or somewhere other than init.cpp since there might be too much logic in there. Not exactly sure where would be the best spot. Also another thing I wanted to clarify is that will |
|
Tested this locally.
Yes, I think |
|
Unrelated to the actual implementation in this PR, but I disagree with some of Luke's points:
The way this is currently documented, both host and port should be required. However, it does align with many of our other options to have a default port, and if that makes sense here too then the help for
Whilst this is technically true for low-level networking (code), IMO it's extremely un-common to not require users to use dot-decimal notation for hosts, and I think we should reject non dot-decimal hosts to prevent exactly the issue in #23589. We'll lose no flexibility by doing so as users who want to use 0.0.0.1 can just, set the option to that directly? |
I think it makes sense to make the port optional if that is also what is being done for the other options.
one of my concerns is that if there are users not using non dot-decimal hosts then it would cause breakage for those users if we begin to reject non dot-decimal hosts do we reject non dot-decimal hosts anywhere else in this codebase? |
|
closing this PR for now since this #28101 should fix the issue, the PR just changes the helper text to explain that a default port is included but no additional checks are added |
… to specify that a default port is used 9a84200 doc, refactor: Changing -torcontrol help to specify that a default port is used (kevkevin) Pull request description: Right now when we get the help for -torcontrol it says that there is a default ip and port we dont specify if there is a specified ip that we would also use port 9051 as default Also I create a new const instead of using 9051 directly in the function linking this PR because this was discussed here bitcoin/bitcoin#28018 ACKs for top commit: jonatack: re-ACK 9a84200 achow101: ACK 9a84200 MarnixCroes: utACK 9a84200 kristapsk: utACK 9a84200 Tree-SHA512: 21d9e65f3c280a2853a9cf60d4e93e8d72caccea106206d1862c19535bde7ea6ada7f55e6ea19a1fc0f59dbe791ec6fc4084fdbe7fa6d6991fa89c62070db637
…ify that a default port is used 9a84200 doc, refactor: Changing -torcontrol help to specify that a default port is used (kevkevin) Pull request description: Right now when we get the help for -torcontrol it says that there is a default ip and port we dont specify if there is a specified ip that we would also use port 9051 as default Also I create a new const instead of using 9051 directly in the function linking this PR because this was discussed here bitcoin#28018 ACKs for top commit: jonatack: re-ACK 9a84200 achow101: ACK 9a84200 MarnixCroes: utACK 9a84200 kristapsk: utACK 9a84200 Tree-SHA512: 21d9e65f3c280a2853a9cf60d4e93e8d72caccea106206d1862c19535bde7ea6ada7f55e6ea19a1fc0f59dbe791ec6fc4084fdbe7fa6d6991fa89c62070db637
right now for -torcontrol you can pass in any string as long as it doesn't have a : but that is invalid. I added an additional check before calling SplitHostPort to see if there is a :
closes #23589