test: Require standard txs in regtest by default #15891

MarcoFalke commented at 1:53 PM on April 25, 2019: member

I don't see a reason why regtest should allow non-standard txs, as it makes testing mainnet behaviour such as #15846 unnecessarily hard and unintuitive.

Of course, testnet policy remains unchanged to allow propagation of non-standard txs.

fanquake added the label Tests on Apr 25, 2019

gmaxwell commented at 1:55 PM on April 25, 2019: contributor

Sounds good to me.

MarcoFalke added the label TX fees and policy on Apr 25, 2019

harding commented at 1:56 PM on April 25, 2019: contributor

Yes, please.

sdaftuar commented at 2:12 PM on April 25, 2019: member

Concept ACK.

FYI I found the title of the PR and the PR description a bit confusing, because it wasn't clear to me if this was going to make it so that we would be able to configure a regtest node to accept non-standard transactions, or if regtest would be treated the same as mainnet and it would not be allowed at all.

MarcoFalke renamed this:
~~test: Require standard txs in regtest~~
test: Require standard txs in regtest by default
on Apr 25, 2019

MarcoFalke commented at 2:20 PM on April 25, 2019: member

Adjusted title as requested by @sdaftuar

Also, please review commit-by-commit. The first commit is a cleanup of (8d1de43f0c Remove internal miner), the others are explained in the commit subject line.

jnewbery commented at 2:46 PM on April 25, 2019: member

Concept ACK

DrahtBot commented at 4:21 PM on April 25, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16264 (policy: Add experimental -mempoolreplacement=full (off by default) by MarcoFalke)
#16060 (Bury bip9 deployments by jnewbery)
#15169 (Parallelize CheckInputs() in AcceptToMemoryPool() by sdaftuar)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in doc/release-notes.md:89 in faac8a22fa outdated

  84 | +relevant in production. The changes are mentioned for completeness.
  85 | +
  86 | +RPC
  87 | +---
  88 | +
  89 | +- Example item

Sjors commented at 6:56 PM on April 27, 2019:

Nit: lorum ipsum :-) Or just use a PR specific release note file

jnewbery commented at 5:47 PM on May 16, 2019:

This still needs to be removed

in test/functional/feature_rbf.py:70 in faac8a22fa outdated

  66 | @@ -67,14 +67,15 @@ def set_test_params(self):
  67 |          self.num_nodes = 2
  68 |          self.extra_args = [
  69 |              [
  70 | +                "-acceptnonstdtxn=1",

Sjors commented at 7:24 PM on April 27, 2019:

I can confirm this is needed to pass the test, but I'm confused why.

MarcoFalke commented at 9:16 PM on April 27, 2019:

The test spends from anyone-can-spend "padded" scriptPubKeys such as CScript([b'a' * 35])

Sjors approved

Sjors commented at 7:31 PM on April 27, 2019: member

tACK faac8a2 on macOS 10.14.4 and Ubuntu 18.04

luke-jr commented at 6:19 AM on May 1, 2019: member

Concept NACK. Tests affected by policy (other than tests for the policy itself) are broken. Not enforcing policy on such tests helps improve test quality.

laanwj commented at 10:12 AM on May 6, 2019: member

I don't particularly like re-introducing a chain type enumeration. The introduction of the ChainParams structure was supposed to make the entire chain information data-driven (with a possibility to add custom hybrid chain types for specific testing, for example, in the future), and this seems a step back from this by making it easier to match on "chain type" instead of add a bit/parameter to the structure for a specific property.

luke-jr commented at 12:28 PM on May 6, 2019: member

Perhaps instead it should be toggled in the test framework's default options?

MarcoFalke commented at 1:10 PM on May 6, 2019: member

In this case the property really is IsMainnet, which does not have a bit allocated in the struct.

Personally, I don't see why an enumeration can not have an additional type ChainType::CUSTOM_HYBRID if needed. But if I am also happy to use string matching on the chain name (could lead to weird run-time behavior) or add a bit to indicate if the params are the mainnet params.

in doc/release-notes.md:95 in faac8a22fa outdated

  90 | +
  91 | +Tests
  92 | +-----
  93 | +
  94 | +- The regression test chain, that can be enabled by the `-regtest` command line
  95 | +  flag, now requires transactions to not violoate standard policy by default.

practicalswift commented at 4:18 PM on May 7, 2019:

Should be "violate"? :-)

MarcoFalke commented at 10:16 PM on May 10, 2019:

Thx, will fix the typo the next time I touch the release notes or the next time I have to touch this pull (for other reasons)

in test/functional/feature_bip68_sequence.py:34 in faac8a22fa outdated

  28 | @@ -29,7 +29,10 @@
  29 |  class BIP68Test(BitcoinTestFramework):
  30 |      def set_test_params(self):
  31 |          self.num_nodes = 2
  32 | -        self.extra_args = [[], ["-acceptnonstdtxn=0"]]
  33 | +        self.extra_args = [
  34 | +            ["-acceptnonstdtxn=1"],
  35 | +            ["-acceptnonstdtxn=0"],

jnewbery commented at 5:48 PM on May 16, 2019:

Suggest you just remove this. We don't usually specify config in tests where it's the default.

MarcoFalke commented at 7:17 PM on May 16, 2019:

I think it is easier to read due to symmetry

in test/functional/p2p_segwit.py:189 in faac8a22fa outdated

 181 | @@ -182,7 +182,11 @@ def set_test_params(self):
 182 |          self.setup_clean_chain = True
 183 |          self.num_nodes = 3
 184 |          # This test tests SegWit both pre and post-activation, so use the normal BIP9 activation.
 185 | -        self.extra_args = [["-whitelist=127.0.0.1", "-vbparams=segwit:0:999999999999"], ["-whitelist=127.0.0.1", "-acceptnonstdtxn=0", "-vbparams=segwit:0:999999999999"], ["-whitelist=127.0.0.1", "-vbparams=segwit:0:0"]]
 186 | +        self.extra_args = [
 187 | +            ["-whitelist=127.0.0.1", "-acceptnonstdtxn=1", "-vbparams=segwit:0:999999999999"],
 188 | +            ["-whitelist=127.0.0.1", "-acceptnonstdtxn=0", "-vbparams=segwit:0:999999999999"],

jnewbery commented at 5:49 PM on May 16, 2019:

as above, I suggest you remove "-acceptnonstdtxn=0" from here since it's default config

MarcoFalke commented at 7:16 PM on May 16, 2019:

I think it is easier to read due to symmetry

jnewbery commented at 5:59 PM on May 16, 2019: member

tested ACK faac8a22faa62dcde3ded60063eff8beb042bafb

A few small suggestions inline

in src/init.cpp:1172 in faac8a22fa outdated

1168 | @@ -1169,8 +1169,9 @@ bool AppInitParameterInteraction()
1169 |      }
1170 |  
1171 |      fRequireStandard = !gArgs.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
1172 | -    if (chainparams.RequireStandard() && !fRequireStandard)
1173 | +    if (chainparams.NetworkID() == ChainType::MAIN && !fRequireStandard) {

sipa commented at 5:12 PM on May 22, 2019:

I generally dislike having conditions in the code that check for a specific chain. What is wrong with leaving this as chainparams.RequireStandard()? AFAICT it does the same thing.

MarcoFalke commented at 5:29 PM on May 22, 2019:

No it does not do the same thing. This pull request changes regtest such that txs are required to be standard by default. If I left the check as is, it would be impossible to pass in a different setting on the command line.

sipa commented at 5:34 PM on May 22, 2019:

Perhaps split it into a RequireStandard() and a DefaultRequireStandard chainparams flag then?

MarcoFalke force-pushed on May 22, 2019

MarcoFalke commented at 6:25 PM on May 22, 2019: member

Remove the two intermediate enum class ChainType refactoring commits (thanks for the review @sipa and @laanwj) and fixed up the release notes in the last commit (thanks for the review @practicalswift, @Sjors and @jnewbery)

MarcoFalke closed this on May 22, 2019

MarcoFalke reopened this on May 22, 2019

MarcoFalke force-pushed on May 22, 2019

MarcoFalke commented at 9:02 PM on May 22, 2019: member

Sorry had to rebase due to silent merge conflicts in the tests

DrahtBot added the label Needs rebase on May 23, 2019

MarcoFalke force-pushed on May 23, 2019

DrahtBot removed the label Needs rebase on May 23, 2019

jnewbery commented at 8:52 PM on May 23, 2019: member

utACK cd338016f5663703cd6eb87bd4403a8fcb4f27a9

MarcoFalke commented at 12:15 PM on May 24, 2019: member

@jnewbery Looks like you posted this on the wrong repo?

jnewbery commented at 2:08 PM on May 24, 2019: member

Oops. Sorry, I rebased this PR on a later master which is how I got the cd33801.. hash.

utACK faa36e1303773bec382600cb70ccd65eb5bbd33b

DrahtBot added the label Needs rebase on Jun 18, 2019

chainparams: Remove unused fMineBlocksOnDemand

It is equal to consensus.fPowNoRetargeting

fa613ca0a8

test: Add test that mainnet requires standard txs fa9b419160

MarcoFalke force-pushed on Jun 18, 2019

MarcoFalke commented at 7:42 PM on June 18, 2019: member

Rebased due to conflict in a test file

DrahtBot removed the label Needs rebase on Jun 18, 2019

in test/functional/feature_rbf.py:77 in fa14bda653 outdated

  73 |                  "-limitancestorcount=50",
  74 |                  "-limitancestorsize=101",
  75 |                  "-limitdescendantcount=200",
  76 |                  "-limitdescendantsize=101",
  77 |              ],
  78 | +            [

jnewbery commented at 9:04 PM on June 19, 2019:

This needs to be removed

MarcoFalke commented at 11:55 AM on June 20, 2019:

Ah good catch. Done

MarcoFalke force-pushed on Jun 19, 2019

in doc/release-notes.md:106 in fa70308485 outdated

 102 | @@ -103,6 +103,23 @@ Low-level Changes section below.
 103 |  Low-level changes
 104 |  =================
 105 |  
 106 | +This section describes RPC changes mainly useful for testing, mostly not

jnewbery commented at 2:58 PM on June 20, 2019:

This isn't true for all the text in the low-level changes section. I suggest you remove this sentence.

MarcoFalke commented at 3:48 PM on June 20, 2019:

This sentence is copied from https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.18.0.md#low-level-changes. Since I am adding this section, I need to add the description as well.

MarcoFalke commented at 3:50 PM on June 20, 2019:

Changed wording

jnewbery commented at 4:44 PM on June 20, 2019:

Weird. That text is clearly wrong in the v0.18 release notes.

jnewbery commented at 4:45 PM on June 20, 2019:

Text is still not correct. The changes described in the section are not for testing only. Please just remove the sentence.

MarcoFalke commented at 5:08 PM on June 20, 2019:

Removed for now

in src/chainparams.h:106 in fa70308485 outdated

 102 | @@ -101,7 +103,7 @@ class CChainParams
 103 |      std::vector<SeedSpec6> vFixedSeeds;
 104 |      bool fDefaultConsistencyChecks;
 105 |      bool fRequireStandard;
 106 | -    bool fMineBlocksOnDemand;
 107 | +    bool m_user_can_change_chain_params;

luke-jr commented at 3:19 PM on June 20, 2019:

Policies are not really chain params...

ajtowns commented at 8:31 PM on June 20, 2019:

+1 on luke's comment -- you can't change chain params, they're a unique_ptr<const ..> so the compiler would complain if you even tried...

Seems like it would be better named as m_allow_non_std_tx or something, making the later test if (!chainParams.AllowNonStandardTx() && !fRequireStandard). (At least, I don't think it ever makes sense to make -acceptnonstd=0 an invalid option)

MarcoFalke commented at 9:14 PM on June 20, 2019:

you can't change chain params, they're a unique_ptr<const ..> so the compiler would complain if you even tried...

That sounds like an implementation detail to me. Effectively you can change the chain param: The const fRequireStandard in the chain params global serves as default and the global fRequireStandard is the effective value. I think having two globals with the same name (in different namespaces) is confusing and should go, but I am not doing this here.

My opinion is that user should be able to change chain params whenever they are running a test chain (e.g. testnet, regtest or testchains #8994 by @jtimon). And they should be forbidden to change them when they are running on mainnet.

ajtowns commented at 9:42 PM on June 20, 2019:

The name of the variable is also an implementation detail :)

The way I look at it is ChainParams for mainnet/testnet/regtest is effectively a compile-time constant that includes consensus parameters, various policy defaults, and some constraints on user's ability to change policy (this one, allow_fallback_fee). You can't change any of those things (without recompiling). That you can choose something different to a default doesn't mean you're changing the default though, which is what you'd be doing if you were actually changing the chain params. Just like you're not changing DEFAULT_BLOCK_MAX_WEIGHT when you set -blockmaxweight, eg.

Even in a testchains world, I think many of the chain params for testnet (or public signets) probably should remain hard to change -- changing the address prefixes/hrp or default port or network magic or consensus parameters when you don't know what you're doing seems like a great way to shoot yourself in the foot, for instance. (If you do know what you're doing, obviously you can change the code to do what you want and recompile)

MarcoFalke commented at 9:48 PM on June 20, 2019:

Ok makes sense, but I still fail to see why the m_user_can_change_chain_params member is wrong. I could call it m_user_can_override_chain_params (as opposed to overwrite/change) to make clear that chainparams are the default values, that can potentially be overridden?

ajtowns commented at 12:53 AM on June 21, 2019:

If it let you override any chain param, that name would make sense; but it only lets you override this specific one. I don't think it makes much sense to try to make it more general; if it did, combining m_fallback_fee_enabled into it ought to work, but I think just having separate bools enabling the individual features is clearer.

luke-jr commented at 3:10 AM on June 21, 2019:

My point was only that this isn't a chain param at all. The default value for the policy just happens to reside on the chainparams object as an implementation detail.

MarcoFalke commented at 8:13 PM on June 21, 2019:

My goal is to remove the wallet-specific m_fallback_fee_enabled from the chain params later on. I can rename m_user_can_change_chain_params to m_is_test_chain, which should address both concerns raised in the comments here.

jtimon commented at 1:55 AM on June 22, 2019:

Note that #8994 maintains the unique_ptr<const ..> and the chainparams can never be changed after initialization, not even for custom chains.

MarcoFalke commented at 12:53 PM on June 22, 2019:

Ok, removed the "user can change chain params" field.

in test/functional/feature_block.py:81 in fa70308485 outdated

  77 | @@ -78,7 +78,7 @@ class FullBlockTest(BitcoinTestFramework):
  78 |      def set_test_params(self):
  79 |          self.num_nodes = 1
  80 |          self.setup_clean_chain = True
  81 | -        self.extra_args = [[]]
  82 | +        self.extra_args = [['-acceptnonstdtxn=1']]  # This is a consensus block test, we don't care about tx policy

luke-jr commented at 3:20 PM on June 20, 2019:

Most tests shouldn't care about tx policy (only policy-specific tests should), so this should be set by default in the test framework.

ajtowns commented at 8:32 PM on June 20, 2019:

Disagree with the conclusion here -- most tests should be working with std transactions, so there should be an error reported when a non-std tx is accidently used. Having tests that do use non-std txs declare it explicitly seems right to me.

luke-jr commented at 3:11 AM on June 21, 2019:

Tests should not begin to fail just because the node policy changes (unless those tests are specifically covering policy code).

luke-jr changes_requested

jnewbery commented at 3:37 PM on June 20, 2019: member

ACK fa70308485b2b8cca371581966f40dfcb7efd9fb modulo one requested change to the release notes.

MarcoFalke force-pushed on Jun 20, 2019

jnewbery commented at 5:18 PM on June 20, 2019: member

ACK fad77227b2b5df547d59bc226b652663aa8bf185

ajtowns commented at 9:02 PM on June 20, 2019: member

Approach ACK, not keen on "user can change chain params" naming, but the rest looks good to me.

MarcoFalke force-pushed on Jun 21, 2019

test: Require standard txs in regtest fa89badf88

MarcoFalke force-pushed on Jun 21, 2019

in src/chainparams.h:75 in fa613ca0a8 outdated

  70 | @@ -71,8 +71,8 @@ class CChainParams
  71 |      uint64_t AssumedBlockchainSize() const { return m_assumed_blockchain_size; }
  72 |      /** Minimum free space (in GB) needed for data directory when pruned; Does not include prune target*/
  73 |      uint64_t AssumedChainStateSize() const { return m_assumed_chain_state_size; }
  74 | -    /** Make miner stop after a block is found. In RPC, don't return until nGenProcLimit blocks are generated */
  75 | -    bool MineBlocksOnDemand() const { return fMineBlocksOnDemand; }
  76 | +    /** Whether it is possible to mine blocks on demand (no retargeting) */
  77 | +    bool MineBlocksOnDemand() const { return consensus.fPowNoRetargeting; }

jtimon commented at 1:59 AM on June 22, 2019:

Not sure why the most newly introduced but redundant one is removed instead of the other way around, but not important.

in src/init.cpp:1153 in fa89badf88

1149 | @@ -1150,8 +1150,9 @@ bool AppInitParameterInteraction()
1150 |      }
1151 |  
1152 |      fRequireStandard = !gArgs.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
1153 | -    if (chainparams.RequireStandard() && !fRequireStandard)
1154 | +    if (!chainparams.IsTestChain() && !fRequireStandard) {

jtimon commented at 2:09 AM on June 22, 2019:

I didn't even know it was forbidden to use -acceptnonstdtxn=1 on mainchain... Anyway, I don't like IsTestChain much, it is not descriptive. What about forbid_nonstd? Or Leave this at RequireStandard and separate another DefaultRequireStandard or DefaultAcceptNonStd like @sipa suggested?

MarcoFalke commented at 1:00 PM on June 22, 2019:

I am happy to extend the docstring of IsTestChain if it is not descriptive enough, but I'd like to keep it unless others agree that it should go.

I think it is useful to have a single boolean param to indicate whether the chain's purpose is test-only.

jtimon commented at 2:13 AM on June 22, 2019: contributor

utACK beyond nits.

ajtowns commented at 6:42 AM on June 25, 2019: member

ACK fa89badf887dcc01e5bdece248b5e7d234fee227

MarcoFalke commented at 3:46 PM on June 27, 2019: member

This will be merged next week Wednesday, unless there are objections.

MarcoFalke merged this on Jul 16, 2019

MarcoFalke closed this on Jul 16, 2019

MarcoFalke referenced this in commit 24dbcf3808 on Jul 16, 2019

MarcoFalke deleted the branch on Jul 16, 2019

sidhujag referenced this in commit e283a6f907 on Jul 29, 2019

deadalnix referenced this in commit c12ec9d8c9 on Apr 20, 2020

ftrader referenced this in commit 799eeb67e8 on Aug 17, 2020

DrahtBot locked this on Dec 16, 2021