Fix opt-in RBF reliance on compiler integer size #10172

pull awemany wants to merge 0 commits into bitcoin:master from awemany:rbf-numlimits-fix changing 0 files +0 −0

awemany commented at 4:59 PM on April 8, 2017: contributor

Opt-in RBF was using std::numeric_limits<unsigned int>::max() for determining whether a TXN is RBF- able or not, thus relying on an architecture/compiler detail, which would fail e.g. for ILP64 data types.

This commit replaces the above with the SEQUENCE_FINAL protocol constant.

Fix opt-in RBF reliance on compiler integer size

Opt-in RBF was using std::numeric_limits<unsigned int>::max() for
determining whether a TXN is RBF- able or not, thus relying on an
architecture/compiler detail, which would fail e.g. for ILP64 data types.

This commit replaces the above with the SEQUENCE_FINAL protocol constant.

f84cdb9095

awemany referenced this in commit 4bb2a6886f on Apr 8, 2017
TheBlueMatt commented at 7:52 PM on April 8, 2017: member

I'm sure this is far from the only place that we have a reliance on integer sizes, but utACK f84cdb9095b1b0f646f8447555e5281ac018f7a6
luke-jr commented at 10:37 PM on April 8, 2017: member

Conflicts with and irrelevant due to MAX_BIP125_RBF_SEQUENCE in #9672
fanquake added the label Validation on Apr 8, 2017
dcousens approved
laanwj commented at 7:42 AM on April 10, 2017: member

@luke-jr Yes, defining a dedicated constant would be even better.
laanwj commented at 1:39 PM on June 7, 2017: member

#9672 has been merged - but it doesn't seem that this actually conflicts.
laanwj commented at 3:05 PM on June 28, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10172/commits

in src/validation.cpp:640 in f84cdb9095

 636 | @@ -637,7 +637,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
 637 |                  {
 638 |                      BOOST_FOREACH(const CTxIn &_txin, ptxConflicting->vin)
 639 |                      {
 640 | -                        if (_txin.nSequence < std::numeric_limits<unsigned int>::max()-1)
 641 | +                        if (_txin.nSequence < CTxIn::SEQUENCE_FINAL-1)

luke-jr commented at 7:21 PM on June 28, 2017:

Probably should just use MAX_BIP125_RBF_SEQUENCE

jonasschnelli commented at 7:43 PM on June 28, 2017: contributor

utACK f84cdb9095b1b0f646f8447555e5281ac018f7a6 I think CTxIn::SEQUENCE_FINAL makes more sense at this place (src/validation.cpp:L640).
laanwj referenced this in commit 04d395e832 on Jul 26, 2017
jtimon commented at 12:30 AM on September 6, 2017: contributor
I agree with @luke-jr in MAX_BIP125_RBF_SEQUENCE being more appropriate than CTxIn::SEQUENCE_FINAL here. The latter is block consensus and the former is tx relay policy, more specifically, the line in question is inside:
```
if (fEnableReplacement)
```
inside:
```
if (!setConflicts.count(ptxConflicting->GetHash()))
```
and currently bip125 is the first and only optional replacement policy that we have.

Perhaps MAX_BIP125_RBF_SEQUENCE should be CTxIn::SEQUENCE_FINAL -1 and we should s/CTxIn::SEQUENCE_FINAL -1/MAX_BIP125_RBF_SEQUENCE/ everywhere. If it's the consensus one, it should't need the -1.

Besides that, very good catch, utACK
laanwj commented at 12:59 PM on October 4, 2017: member

Needs rebase.
fanquake commented at 3:10 PM on November 17, 2017: member

Was replaced by #10854.
fanquake closed this on Nov 17, 2017
awemany deleted the branch on Nov 24, 2017
codablock referenced this in commit f7a6790623 on Sep 23, 2019
barrystyle referenced this in commit 4bf59cd785 on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10854 Avoid using sizes on non-fixed-width types to derive protocol constants.#10938 Build failed on Ubuntu 14.04LTS