Remove STANDARD_LOCKTIME_VERIFY_FLAGS and mempool policy's flags

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: 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:

0CheckFinalTx(tx, flags)

0txmempool.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: 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: 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

Remove STANDARD_LOCKTIME_VERIFY_FLAGS and mempool policy’s flags #7574