Avoid using numeric_limits for sequence numbers and lock times

ryanofsky commented at 9:25 pm on November 1, 2018: member

Switches to named constants, because numeric_limits calls can be harder to read and less portable.

Change was suggested by jamesob in #10973 (review)

There are no changes in behavior except on some platforms we don’t support (ILP64, IP16L32, I16LP32), where SignalsOptInRBF and MutateTxAddInput functions would now work correctly.

Replace platform dependent type with proper const e4dc39b3bc

Remove duplicated code

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

bafb921507

Avoid using numeric_limits for sequence numbers and lock times

Switches to named constants, because numeric_limits calls can be harder to read
and less portable.

Change was suggested by James O'Beirne <james.obeirne@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/10973#discussion_r213473620

There are no changes in behavior except on some platforms we don't support
(ILP64, IP16L32, I16LP32), where SignalsOptInRBF() and MutateTxAddInput()
functions would now work correctly.

535203075e

MarcoFalke added the label Refactoring on Nov 1, 2018

MarcoFalke commented at 9:29 pm on November 1, 2018: member

Tagged “refactoring” because the platforms that are broken are supposedly not supported.

practicalswift commented at 9:43 pm on November 1, 2018: contributor

utACK 8041cd39f0979a7db5d51bd1fc83d97d311836f7

in src/policy/rbf.cpp:10 in 8041cd39f0 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 < CTxIn::SEQUENCE_FINAL - 1) {

hebasto commented at 9:46 pm on November 1, 2018:

could if (txin.nSequence <= MAX_BIP125_RBF_SEQUENCE) { ?

fanquake commented at 0:54 am on November 2, 2018:

This was suggested by gmexwell here

DrahtBot commented at 11:39 pm on November 1, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

fanquake commented at 0:55 am on November 2, 2018: member

@ryanofsky Can you combine the test change from #14463 into this? I’ll close that PR, as this has more extensive changes.

ryanofsky force-pushed on Nov 5, 2018

ryanofsky commented at 7:51 pm on November 5, 2018: member

Updated 8041cd39f0979a7db5d51bd1fc83d97d311836f7 -> 90a9e728070c9dd6e236b036cc25ab9ff477f096 (pr/climit.1 -> pr/climit.2) adding commits from #14463

in src/script/script.h:45 in 90a9e72807 outdated

37@@ -38,6 +38,12 @@ static const int MAX_STACK_SIZE = 1000;
38 // otherwise as UNIX timestamp.
39 static const unsigned int LOCKTIME_THRESHOLD = 500000000; // Tue Nov  5 00:53:20 1985 UTC
40 
41+// Maximum nLockTime. Since a lock time indicates the last invalid timestamp, a
42+// transaction with this lock time will never be valid unless lock time
43+// checking is disabled (by setting all input sequence numbers to
44+// SEQUENCE_FINAL).
45+static const uint32_t LOCKTIME_MAX = 0xffffffff;

gmaxwell commented at 9:56 pm on November 5, 2018:

0xFFFFFFFFU ? lest we immediately get another hysterically titled “prevent integer overflow” PR?

practicalswift commented at 10:16 pm on November 5, 2018:

@gmaxwell Why would an U suffix be needed in this case? 0xffffffff is unsigned on all platforms we support, right?

ryanofsky force-pushed on Nov 5, 2018

ryanofsky commented at 10:43 pm on November 5, 2018: member

Updated 90a9e728070c9dd6e236b036cc25ab9ff477f096 -> 535203075e50eedef8f00852328f81f440233278 (pr/climit.2 -> pr/climit.3) adding U suffix (doesn’t hurt to be explicit).

practicalswift commented at 10:56 pm on November 5, 2018: contributor

@ryanofsky I think the U suffix suggestion was based on a misunderstanding regarding the types of C++ integer literals.

More specifically that the integer literal of 4294967295 can be assumed to have the same type as the integer literal 0xffffffff.

That assumption is false :-)

Proof:

0[cling]$ #include <typeindex>
1[cling]$ #include <typeinfo>
2[cling]$ std::type_index(typeid(4294967295)) == std::type_index(typeid(0xffffffff))
3(bool) false
4[cling]$ 4294967295
5(long) 4294967295
6[cling]$ 0xffffffff
7(unsigned int) 4294967295

ryanofsky commented at 11:15 pm on November 5, 2018: member

The u suffix isn’t needed, but it’s nice because it makes it more obvious that compiler will choose an unsigned type. (According to the “type of the literal” table in https://en.cppreference.com/w/cpp/language/integer_literal, the compiler without the u would choose smallest signed or unsigned type that holds the value.)

jimpo commented at 5:26 pm on November 6, 2018: contributor

ACK 535203075e50eedef8f00852328f81f440233278

MarcoFalke commented at 7:51 pm on November 6, 2018: member

utACK 535203075e50eedef8f00852328f81f440233278

Checked that e4dc39b3bc4f199c863a71235f7269f7f9e336de happens to produce the same bitcoind binaries on my arch
Don’t care about bafb921507761217a2e9013a42e0aa619f831ccf
Checked that 535203075e50eedef8f00852328f81f440233278 happens to produce the same bitcoin-tx and bitcoind binaries on my arch.

MarcoFalke added this to the milestone 0.18.0 on Nov 6, 2018

ken2812221 commented at 3:17 am on November 7, 2018: contributor

utACK 535203075e50eedef8f00852328f81f440233278

MarcoFalke referenced this in commit e8d490f27e on Nov 7, 2018

MarcoFalke merged this on Nov 7, 2018

MarcoFalke closed this on Nov 7, 2018

5tefan referenced this in commit fe1aa6fe0e on Jul 28, 2021

5tefan referenced this in commit d1ea7d9e4c on Jul 28, 2021

5tefan referenced this in commit c64f797467 on Jul 28, 2021

PastaPastaPasta referenced this in commit 59cfd5263a on Jul 28, 2021

MarcoFalke locked this on Sep 8, 2021

Avoid using numeric_limits for sequence numbers and lock times #14636

Conflicts