chainparams: Explicitly use uint16 for nDefaultPort #15586

1. 
   dongcarl commented at 9:52 pm on March 12, 2019: member

   Port numbers are 16-bit unsigned integers.
2. chainparams: Explicitly use uint16 for nDefaultPort

   Port numbers are 16-bit unsigned integers.

   7c6b079883
3. 
   promag commented at 9:55 pm on March 12, 2019: member

   Could also update call sites?
4. 
   MarcoFalke commented at 10:03 pm on March 12, 2019: member

   Unless you switch the constructors to use member initizlizer lists with the “{}-syntax” that checks for overflows, I don’t see how this adds any value.

   Also, #8394 (comment)

5. fanquake added the label Refactoring on Mar 12, 2019
6. 
   laanwj commented at 10:39 am on March 13, 2019: member

   sorry, NACK, I don’t see the point, this doesn’t improve the code in any user-visible way
7. ryanofsky approved
8. 
   ryanofsky commented at 2:31 pm on March 13, 2019: member

   utACK 7c6b079883b579b1973dd91df7b60bf95e85ef20. Code change itself looks good and is more clear and correct. Will happily defer to Marco/Wladimir on project management & prioritization, though, if this shouldn’t be merged on those grounds.
9. practicalswift approved
10. 
    practicalswift commented at 3:27 pm on March 13, 2019: contributor

    utACK 7c6b079883b579b1973dd91df7b60bf95e85ef20 (agree with @ryanofsky)
11. 
    dongcarl commented at 2:39 pm on March 14, 2019: member

    @MarcoFalke You mean use member initializer lists for CChainParams?
12. 
    dongcarl commented at 2:43 pm on March 14, 2019: member

    w/re the parsing comment #8394 (comment), would @laanwj and @MarcoFalke be more inclined to merge if this PR added relevant ArgsManager changes? This would be usable for both -port and -rpcport I believe.
13. 
    dongcarl commented at 3:19 pm on March 14, 2019: member

    Just for some context, this was motivated by my looking at GetListenPort(), which I believe casts signed ints to unsigned shorts. I think pairing this change with an ArgsManager change with good parsing would make it worthwhile.

    Also related: #14856 (review)

14. 
    MarcoFalke commented at 10:04 pm on March 14, 2019: member

    I believe it casts int64_t to unsigned short, but that should be fine as long as we check that the value is in range before the cast. No need to change the return type here.
15. 
    luke-jr commented at 8:39 pm on April 17, 2019: member

    This is just one of many cases where a 16-bit int type could cause problems, so I’m okay with fixing it for future portability. But I agree that if we do, we should fix the call sites too:

    • GetListenPort (in net.cpp)
    • CConnman::ConnectNode’s default_port (in net.cpp)
    • Lookup’s portDefault, port (in netbase.cpp)
    • • SplitHostPort’s portOut (in util/strencodings.cpp)
    • LookupNumeric’s portDefault (in net.cpp)
16. 
    dongcarl commented at 7:42 pm on May 15, 2019: member

    I’ll pick this back up someday. Feel free to grab it from me.
17. dongcarl closed this on May 15, 2019

    ---

18. dongcarl added the label Up for grabs on May 15, 2019
19. fanquake removed the label Up for grabs on Mar 2, 2021
20. 
    fanquake commented at 1:45 am on March 2, 2021: member

    Removing up for grabs, as this is currently included in #21328.
21. DrahtBot locked this on Aug 16, 2022

Contributors
dongcarl promag MarcoFalke laanwj ryanofsky practicalswift luke-jr fanquake

Labels
Refactoring