Remove STANDARD_LOCKTIME_VERIFY_FLAGS and mempool policy's flags #7574

NicolasDorier commented at 5:38 PM on February 21, 2016: contributor

Remove flags from CheckFinalTx and removeForReorg. MTP is always enforced since 0.11.2 in mempool. See release notes: https://bitcoin.org/en/release/v0.11.2

jonasschnelli added the label Mempool on Feb 21, 2016

laanwj commented at 1:13 PM on March 31, 2016: member

Needs rebase. I agree moving the CTxMemPool::removeForReorg function seems unnecessary.

jtimon commented at 3:06 PM on March 31, 2016: contributor

+1 nack on moving CTxMemPool::removeForReorg for no apparent reason. I have mixed feelings about this. On one hand, I kind of hate default values for params (like flags=-1) and this has some nice cleanups. On the other hand, I would like to be more explicit about flags everywhere, not less. I've been saying this multiple times in "random" places, but...can't we just move LOCKTIME_VERIFY_SEQUENCE and LOCKTIME_MEDIAN_TIME_PAST to script/interpreter.h? Never mind, don't want to hijack the PR, I will just open another related one.

NicolasDorier force-pushed on Mar 31, 2016

NicolasDorier commented at 3:23 PM on March 31, 2016: contributor

rebased, @jtimon I'm not sure what you mean by "moving removeForReorg" ? can you check again ? The big diff just before was just a problem of diff presentation I think, I did not attempt to move anything.

btcdrak commented at 3:25 PM on March 31, 2016: contributor

@NicolasDorier Looks much better now.

in src/txmempool.cpp:None in ad930cdab3 outdated

 527 | @@ -528,7 +528,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
 528 |          const CTransaction& tx = it->GetTx();
 529 |          LockPoints lp = it->GetLockPoints();
 530 |          bool validLP =  TestLockPointValidity(&lp);
 531 | -        if (!CheckFinalTx(tx, flags) || !CheckSequenceLocks(tx, flags, &lp, validLP)) {
 532 | +        if (!CheckFinalTx(tx, flags) || !CheckSequenceLocks(tx, LOCKTIME_VERIFY_SEQUENCE, &lp, validLP)) {

MarcoFalke commented at 4:44 PM on March 31, 2016:

CheckFinalTx(tx, flags)

txmempool.cpp:531:31: error: ‘flags’ was not declared in this scope

NicolasDorier commented at 5:06 PM on March 31, 2016:

I'm not using flags anymore as you can see, I think you checkout gone wrong ?

jtimon commented at 5:02 PM on March 31, 2016: contributor

@NicolasDorier I meant what @laanwj (and I would swear someone else before him, but I can't see the "Someone commented on an outdated diff" thing) said (that CTxMemPool::removeForReorg was being moved unnnecessarly [and probably unintentionally too]). Looks fixed now.

Rewarding the only-slightly related comment, this is what I mean is in #7779 .

NicolasDorier commented at 5:06 PM on March 31, 2016: contributor

@jtimon I think the problem was the way github presented the diff. I never intended to move a function. Somehow the rebase fixed it.

Remove STANDARD_LOCKTIME_VERIFY_FLAGS and mempool policy's flags

Remove flags from CheckFinalTx and removeForReorg.
MTP is always enforced since 0.11.2 in mempool.
See release notes: https://bitcoin.org/en/release/v0.11.2

81f9c68d04

NicolasDorier force-pushed on Mar 31, 2016

jtimon commented at 5:22 PM on March 31, 2016: contributor

Oh, I was complaining about github's traceability, but maybe we don't want to keep traceability of their bugs, only ours (not saying that accidentally moving a function was a bug if that had been the case). Anyway, mystery solved or not, let's move on: it's fixed.

in src/main.cpp:None in ad930cdab3 outdated

 680 | @@ -681,18 +681,10 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
 681 |      return true;
 682 |  }
 683 |  
 684 | -bool CheckFinalTx(const CTransaction &tx, int flags)
 685 | +bool CheckFinalTx(const CTransaction &tx)
 686 |  {
 687 |      AssertLockHeld(cs_main);
 688 |  
 689 | -    // By convention a negative value for flags indicates that the

jtimon commented at 5:26 PM on March 31, 2016:

I always dislike removing documentation but I was never sure where to move this or how to replace it. Is https://github.com/bitcoin/bitcoin/pull/7779/commits/22a80055021cdfa6f6bd1496ce15308285424c29 useful?

NicolasDorier commented at 5:31 PM on March 31, 2016:

I like having a single flag, not sure about the pratical purpose of VALIDATION_LEVEL though,but I think would be better suited for a separate PR, this one is more about removing useless code only than refactoring.

in src/policy/policy.h:None in 81f9c68d04

  45 | @@ -46,10 +46,6 @@ static const unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VERIFY
  46 |  /** For convenience, standard but not mandatory verify flags. */
  47 |  static const unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS = STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS;
  48 |  
  49 | -/** Used as the flags parameter to sequence and nLocktime checks in non-consensus code. */
  50 | -static const unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS = LOCKTIME_VERIFY_SEQUENCE |

jtimon commented at 5:35 PM on March 31, 2016:

put in STANDARD_NOT_MANDATORY_VERIFY_FLAGS until putting it in mandatory? ping @TheBlueMatt (we had a related unsolved discussion before).

morcos commented at 5:47 PM on March 31, 2016: member

I'm in favor of cleaning up these flags, but I'm not sure this is the right approach.

Probably something along the lines of what @jtimon is suggesting where there is just one set of consensus flags that are used would be better.

I think we still want to pass flags to CheckFinalTx because we might want to preserve a way of accepting non-MTP locktime txs in the mempool. Certainly its a bit premature to eliminate that until the soft forks activate.

NicolasDorier commented at 5:52 PM on March 31, 2016: contributor

Closing for jtimon approach

NicolasDorier closed this on Mar 31, 2016

DrahtBot locked this on Sep 8, 2021