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

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.

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.

@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.

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.

@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.

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.

@jtimon Yup, ACK DefaultPolicy()

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?

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.

@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?

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.

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

(Lock can be removed in another patch)

@gmaxwell thoughts on removing AssertLockHeld(cs_main); from IsStandardTx() within this PR?

I think it's fine to remove.

@jtimon @gmaxwell Removed.

reACK

Tested ACK

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

ACK