Reduce usage of the platform dependent unsigned int type #14463

1. 
   hebasto commented at 10:29 am on October 11, 2018: member

   unsigned int is a platform dependent type.
2. 
   practicalswift commented at 1:23 pm on October 11, 2018: contributor

   Concept ACK

   Good catch!

   Perhaps worth investigating if you can find any similar cases in:

   0$ git grep -E 'std::numeric_limits<.*>::(min|max)' | grep -vE 'int[0-9]'
   

3. 
   hebasto commented at 6:08 pm on October 11, 2018: member

   @achow101 Would you mind to look into the Travis build log to find out the reason of the rpc_psbt.py functional test failure? Does 54a546f brake the code in any way?
4. 
   practicalswift commented at 8:18 pm on October 11, 2018: contributor

   @hebasto That was likely just a temporary Travis hiccup. I’ve now restarted the build job :-)
5. 
   hebasto commented at 9:05 pm on October 11, 2018: member

   @practicalswift

   I’ve now restarted the build job :-)

   Thank you!

6. hebasto renamed this:
   Remove a platform dependant type
   Reduce usage of the platform dependent `unsigned int` type
   on Oct 12, 2018
7. 
   hebasto commented at 1:04 pm on October 12, 2018: member

   I’ve investigated in direction pointed by @practicalswift and found two cases which are, IMO, safe to fix (i.e. not belong to the consensus part or other critical parts of the code):

   1. this LOC https://github.com/bitcoin/bitcoin/blob/f34c8c466a0e514edac2e8683127b4176ad5d321/src/test/skiplist_tests.cpp#L173 effectively duplicates that one: https://github.com/bitcoin/bitcoin/blob/f34c8c466a0e514edac2e8683127b4176ad5d321/src/test/skiplist_tests.cpp#L169

   2. another case in the src/bloom.cpp.

8. 
   hebasto commented at 1:07 pm on October 12, 2018: member

   @practicalswift Thank you for your review and a piece of advice.

   Perhaps worth investigating if you can find any similar cases in: $ git grep -E 'std::numeric_limits<.*>::(min|max)' | grep -vE 'int[0-9]'

   It’s done. Please re-review.

9. in src/bloom.cpp:304 in 1d1af6bdc4 outdated

   300@@ -301,7 +301,7 @@ bool CRollingBloomFilter::contains(const uint256& hash) const
   301 
   302 void CRollingBloomFilter::reset()
   303 {
   304-    nTweak = GetRand(std::numeric_limits<unsigned int>::max());
   305+    nTweak = GetRand(std::numeric_limits<uint64_t>::max());
   

   ---

   ---

   

   practicalswift commented at 1:25 pm on October 12, 2018:

   In this case I think it makes more sense to match the type of nTweak (currently unsigned int) :-)

   ---

   

   hebasto commented at 1:43 pm on October 12, 2018:

   Agree.
10. in src/test/skiplist_tests.cpp:173 in 1d1af6bdc4 outdated

    169@@ -170,7 +170,6 @@ BOOST_AUTO_TEST_CASE(findearliestatleast_edge_test)
    170     BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(-1)->nHeight, 0);
    171 
    172     BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(std::numeric_limits<int64_t>::min())->nHeight, 0);
    173-    BOOST_CHECK_EQUAL(chain.FindEarliestAtLeast(std::numeric_limits<unsigned int>::min())->nHeight, 0);
    

    ---

    ---

    

    practicalswift commented at 1:27 pm on October 12, 2018:

    How is this one redundant? :-)

    ---

    

    hebasto commented at 1:54 pm on October 12, 2018:

    std::numeric_limits<unsigned int>::min() == 0, therefore, this LOC duplicates that one: https://github.com/bitcoin/bitcoin/blob/f34c8c466a0e514edac2e8683127b4176ad5d321/src/test/skiplist_tests.cpp#L169

    ---

    

    practicalswift commented at 2:07 pm on October 12, 2018:

    Now I follow! Thanks
11. hebasto force-pushed on Oct 12, 2018
12. Replace platform dependent type with proper const 6ac4e5d0fd
13. Remove duplicated code

    The deleted LOC is a dup so far as
    `std::numeric_limits<unsigned int>::min()` == 0

    917590980b
14. in src/policy/rbf.cpp:10 in b9d29a5955 outdated

     6@@ -7,7 +7,7 @@
     7 bool SignalsOptInRBF(const CTransaction &tx)
     8 {
     9     for (const CTxIn &txin : tx.vin) {
    10-        if (txin.nSequence < std::numeric_limits<unsigned int>::max()-1) {
    11+        if (txin.nSequence < 0xffffffff - 1) {
    

    ---

    ---

    

    gmaxwell commented at 4:48 pm on October 12, 2018:

    <= MAX_BIP125_RBF_SEQUENCE
15. hebasto force-pushed on Oct 12, 2018
16. 
    hebasto commented at 6:13 pm on October 12, 2018: member

    @gmaxwell Thank you for your review. Fixed. Please re-review.
17. 
    sipa commented at 7:02 pm on October 12, 2018: member

    I believe unsigned int is 32 bits on all our supported platforms.

    ACK on the change, but there must be many more places where the size of a unsigned int is assumed?

18. 
    hebasto commented at 7:15 pm on October 12, 2018: member

    @sipa Thank you for your review.

    … there must be many more places where the size of a unsigned int is assumed?

    Yes. I’ve found some dozens of them :) But those cases belong to the consensus part or other critical parts of the code which I’d prefer to not touch.

19. meshcollider added the label Refactoring on Oct 12, 2018
20. 
    MarcoFalke commented at 0:17 am on October 24, 2018: member

    utACK 917590980be69f83291613e3b8ed6d14a1b30619
21. 
    DrahtBot commented at 0:13 am on November 2, 2018: member

    • #14636 (Avoid using numeric_limits for sequence numbers and lock times by ryanofsky)

    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.

22. 
    fanquake commented at 0:56 am on November 2, 2018: member

    I’ve asked @ryanofsky to pull your changes into #14636, as that PR has more extensive changes, so I’ll close this for now.
23. fanquake closed this on Nov 2, 2018

    ---

24. hebasto deleted the branch on Nov 8, 2018
25. MarcoFalke locked this on Sep 8, 2021

Contributors
hebasto practicalswift gmaxwell sipa MarcoFalke DrahtBot fanquake

Labels
Refactoring