I wanted to see if the up for grabs label works and looked at PR #17822 originally opend by ahook I saw it had many acks for example by jonatack and practicalswift but needed rebasing.
So I checked out the remote branch rebased it resolved three conflicts and continued the rebase.
Hope everything is as expected (:
Concept ACK: explicit is better than implicit.
Warm welcome as a contributor @renepickhardt - I'm a big fan of your Lightning work etc :) Looking forward to reading Mastering the Lightning Network once it is released!
FWIW, we are already assuming that sizeof(uint16_t) == sizeof(unsigned short) implicitly in multiple places like here …
… and also explicitly in assumptions.h …
btw running git grep "unsigned short" results in:
src/secp256k1/src/tests.c: unsigned short sh;
I could make another commit to this branch where I also changed that but I am not sure if the bitcoin repo is the place to patch libsecp256k1 or if that should be done in https://github.com/bitcoin-core/secp256k1
at least the remote file https://github.com/bitcoin-core/secp256k1/blob/master/src/tests.c has 19 contributers according to git blame where in this repo it only has contributes from @sipa and @laanwj
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
5 | @@ -6,6 +6,7 @@ 6 | #define BITCOIN_QT_OPTIONSMODEL_H 7 | 8 | #include <amount.h> 9 | +#include <cstdint>
It looks like the linter is calling out the trailing whitespace here.
fixed
@renepickhardt Would you mind squashing? I think this one should be ready for final review after squash :)
removed trailing whitespace to make linter happy
Ok so I have squashed the two commits and rebased to current master to make merging easier (:
if there is anything more to do let me know @practicalswift
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd.
Nice to replace a C header with a C++ header as well.
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd -- patch looks correct :)
If anyone wants to follow up by checking potentially unintended/unwarranted uses of long this command might be helpful to find candidates :)
$ git grep -E '[^a-zA-Z_."]long[^a-zA-Z_."]' -- "*.cpp" "*.h" ":(exclude)src/univalue/" ":(exclude)src/qt/" ":(exclude)src/test/" ":(exclude)src/crc32c/" ":(exclude)src/crypto/common.h" ":(exclude)src/randomenv.cpp" | grep -vE '^[^ ]*:.*[/*]' | grep -vE "(too.long|ftell|strtoul|strol|strtol|how long|really long|fseek|as long)"
src/httpserver.cpp: int workQueueDepth = std::max((long)gArgs.GetArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L);
src/httpserver.cpp: int rpcThreads = std::max((long)gArgs.GetArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L);
src/rest.cpp: if (headers.size() == (unsigned long)count)
src/streams.h: long nLongPos = nPos;
src/txmempool.cpp: if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) {
src/txmempool.h: unsigned long size() const
Note the (mentioned in comments only, not an issue for this commit) libsecp256k1 unsigned short does in fact need to be an unsigned short...
36 | @@ -36,7 +37,7 @@ template <typename Data> 37 | bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data& data) 38 | { 39 | // Generate random temporary filename 40 | - unsigned short randv = 0; 41 | + uint16_t randv = 0; 42 | GetRandBytes((unsigned char*)&randv, sizeof(randv)); 43 | std::string tmpfn = strprintf("%s.%04x", prefix, randv);
This is okay only because strprintf has some type magic.
Yes, this isn't a problem, strprintf is type safe. There is never a need for size specifiers.
110 | @@ -110,9 +111,9 @@ void CConnman::AddOneShot(const std::string& strDest) 111 | vOneShots.push_back(strDest); 112 | } 113 | 114 | -unsigned short GetListenPort() 115 | +uint16_t GetListenPort() 116 | { 117 | - return (unsigned short)(gArgs.GetArg("-port", Params().GetDefaultPort())); 118 | + return (uint16_t)(gArgs.GetArg("-port", Params().GetDefaultPort()));
Can we just drop the cast entirely? Not sure I see the point...
The point is to make a potentially sign-changing narrowing conversion explicit.
GetArg returns int64_t so it makes sense to make the implicit conversion to uint16_t explicit using a cast.
798 | @@ -798,11 +799,11 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int 799 | ProxyCredentials random_auth; 800 | static std::atomic_int counter(0); 801 | random_auth.username = random_auth.password = strprintf("%i", counter++); 802 | - if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) { 803 | + if (!Socks5(strDest, (uint16_t)port, &random_auth, hSocket)) {
Why not change the type of port?
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd
After this, the only mention of unsigned short left is in the secp256k1 tests, which should not be changed here
In addition to @luke-jr's comment, could the type of the port parameter in the Socks5() function be changed from int to uint16_t?
I understand why @renepickhardt didn't do this. Right now, this is a pure refactor, only changing a type to a typedef of the same type (for, in practice, all architectures supported by bitcoin). Changing the parameter type would be an actual code change.
In addition to @luke-jr's comment, could the type of the port parameter in the Socks5() function be changed from int to uint16_t?
I understand why @renepickhardt didn't do this. Right now, this is a pure refactor, only changing a type to a typedef of the same type (for, in practice, all architectures supported by bitcoin). Changing the parameter type would be an actual code change.
Agree.
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd, I have reviewed the code and it looks OK, I agree it can be merged.