Reject non-final txs even in testnet/regtest #5521

1. 
   petertodd commented at 5:43 am on December 21, 2014: contributor

   Previous behavior with IsFinalTx() being an IsStandard() rule was rather confusing and interferred with testing of protocols that depended on nLockTime. A particularly bad example is with CHECKLOCKTIMEVERIFY where it gave the impression that the CLTV opcode didn’t do anything as CLTV-using txs that aren’t yet minable because the locktimes haven’t been reached are accepted.
2. 
   jtimon commented at 12:58 pm on December 21, 2014: contributor

   utACK. I also wonder if this check should be part of the node’s policy as in #5071 or it should be left in main. At least “chainActive.Height() + 1” doesn’t seem to be something you should call from policy.o, at most pass it as parameter.
3. 
   petertodd commented at 2:44 pm on December 21, 2014: contributor

   @jtimon How is this sanely a node policy thing? We’re talking about whether or not we accept transactions that can’t be mined to the mempool; remember the nLockTime can be any amount in the future.
4. 
   jtimon commented at 2:59 pm on December 21, 2014: contributor

   That’s what I’m saying, this probably shouldn’t go with the policy code. Thus this change is good for a later move of Params().RequireStandard() && !IsStandardTx(tx, reason) to policy.
5. 
   petertodd commented at 3:08 pm on December 21, 2014: contributor

   @jtimon Oh, you mean should #5071 include this pull-req? Sure, but anyway IMO this pull-req makes sense to merge regardless of what we do with policy in a wider sense.

   FWIW I keep meaning to making IsStandard() something you can turn off via a command-line flag, as it makes testing software on testnet/regtest annoying due to the differences in behavior.

6. 
   jtimon commented at 3:53 pm on December 21, 2014: contributor

   Yes, this PR makes sense independently, just saying that it also makes sense from the policy-encapsulation perspective. About decoupling policy selection from network (chainparams) selection, in #5180 I proposed to turn bool CChainParams::RequireStandard() into std::string CChainParams::DefaultPolicy() so that the user selection of policy overwrites the default per-network while remaining backwards compatible with what one would expect from each mode without knowing about the new policy parameter. I could rebase that or make a simpler version with a subset of #5071 if there’s interest.
7. 
   petertodd commented at 4:09 pm on December 21, 2014: contributor

   @jtimon Yup, ACK DefaultPolicy()
8. 
   jtimon commented at 8:15 pm on December 27, 2014: contributor

   A few questions:

   1. Doesn’t the access to chainActive.Height() require you to AssertLockHeld(cs_main) even though it’s going to be checked again in IsFinalTx() ?

   2. After removing that access to chainActive from IsStandardTx(), why does it need to AssertLockHeld(cs_main)? Only for fIsBareMultisigStd and minRelayTxFee? Aren’t those set only on init?

9. 
   jtimon commented at 3:16 pm on December 29, 2014: contributor

   To be clear, this is what I’m suggesting: https://github.com/jtimon/bitcoin/commits/5521 Maybe it is a bad suggestion for some reason I’m missing.
10. in src/main.cpp: in bd3f7f023a outdated

    930+    // can't know what timestamp the next block will have, and there aren't
    931+    // timestamp applications where it matters.
    932+    if (!IsFinalTx(tx, chainActive.Height() + 1))
    933+        return state.DoS(0,
    934+                         error("AcceptToMemoryPool : non-final"),
    935+                         REJECT_NONSTANDARD, reason);
    

    ---

    ---

    

    jtimon commented at 10:45 pm on December 29, 2014:

    s/reason/“non-final”

    ---

    

    petertodd commented at 4:35 am on January 3, 2015:

    Thanks!
11. 
    petertodd commented at 4:31 am on January 3, 2015: contributor

    @jtimon AcceptToMemoryPool has an AssertLockHeld(cs_main); at the beginning of the function, so we don’t need another. That said I do think you are correct that IsStandardTx() doesn’t need to have cs_main locked anymore. @sipa @gmaxwell @laanwj Care to confirm this?
12. petertodd force-pushed on Jan 3, 2015
13. 
    jtimon commented at 3:17 pm on January 3, 2015: contributor

    Yeah, the main point is that it is not required in IsStandardTx(), that if it was needed, it was only for the access to activeChain, making the function more independent from main to move it out later.
14. 
    gmaxwell commented at 5:58 pm on January 4, 2015: contributor

    utACK. Just wrote a almost bytewise identical patch (my update to the comment was slightly more minimal).

    (Lock can be removed in another patch)

15. 
    jtimon commented at 6:21 pm on January 4, 2015: contributor

    @gmaxwell thoughts on removing AssertLockHeld(cs_main); from IsStandardTx() within this PR?
16. 
    gmaxwell commented at 7:06 pm on January 4, 2015: contributor

    I think it’s fine to remove.
17. Reject non-final txs even in testnet/regtest

    Previous behavior with IsFinalTx() being an IsStandard() rule was rather
    confusing and interferred with testing of protocols that depended on
    nLockTime.

    0ea28baeb8
18. 
    petertodd commented at 7:22 pm on January 4, 2015: contributor

    @jtimon @gmaxwell Removed.
19. petertodd force-pushed on Jan 4, 2015
20. 
    jtimon commented at 10:46 pm on January 4, 2015: contributor

    reACK
21. 
    rnicoll commented at 0:44 am on January 5, 2015: contributor

    Tested ACK
22. 
    laanwj commented at 3:27 pm on January 7, 2015: member

    @petertodd I’ve also checked and indeed, nothing is being done that requires cs_main lock anymore.

    ACK

23. laanwj merged this on Jan 7, 2015
24. laanwj closed this on Jan 7, 2015

    ---

25. laanwj referenced this in commit 8e4578af0f on Jan 7, 2015
26. MarcoFalke locked this on Sep 8, 2021

Contributors
petertodd jtimon gmaxwell rnicoll laanwj