chainparams: Explicitly use uint16 for nDefaultPort #15586

pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2019-03-uint16-port changing 1 files +2 −2
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me