test: Require standard txs in regtest by default #15891

maflcko · 2019-04-25T13:53:24Z

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.

gmaxwell · 2019-04-25T13:55:26Z

Sounds good to me.

harding · 2019-04-25T13:56:54Z

Yes, please.

sdaftuar · 2019-04-25T14:12:27Z

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.

maflcko · 2019-04-25T14:20:17Z

Adjusted title as requested by @sdaftuar

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

jnewbery · 2019-04-25T14:46:42Z

Concept ACK

DrahtBot · 2019-04-25T16:21:43Z

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.

Sjors

tACK faac8a2 on macOS 10.14.4 and Ubuntu 18.04

doc/release-notes.md

Sjors · 2019-04-27T19:24:54Z

test/functional/feature_rbf.py

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

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

luke-jr · 2019-05-01T06:19:41Z

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 · 2019-05-06T10:12:31Z

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 · 2019-05-06T12:28:35Z

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

maflcko · 2019-05-06T13:10:21Z

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.

doc/release-notes.md

jnewbery

tested ACK faac8a22faa62dcde3ded60063eff8beb042bafb

A few small suggestions inline

doc/release-notes.md

jnewbery · 2019-05-16T17:48:58Z

test/functional/feature_bip68_sequence.py

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

I think it is easier to read due to symmetry

jnewbery · 2019-05-16T17:49:51Z

test/functional/p2p_segwit.py

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

I think it is easier to read due to symmetry

src/init.cpp

maflcko · 2019-05-22T18:25:03Z

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)

maflcko · 2019-05-22T21:02:41Z

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

jnewbery · 2019-05-23T20:52:22Z

utACK cd338016f5663703cd6eb87bd4403a8fcb4f27a9

maflcko · 2019-05-24T12:15:05Z

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

jnewbery · 2019-05-24T14:08:18Z

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

utACK faa36e1303773bec382600cb70ccd65eb5bbd33b

maflcko · 2019-06-18T19:42:29Z

Rebased due to conflict in a test file

test/functional/feature_rbf.py

doc/release-notes.md

src/chainparams.h

luke-jr · 2019-06-20T15:20:00Z

test/functional/feature_block.py

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

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.

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

jnewbery

ACK fa70308485b2b8cca371581966f40dfcb7efd9fb modulo one requested change to the release notes.

jnewbery · 2019-06-20T17:18:06Z

ACK fad77227b2b5df547d59bc226b652663aa8bf185

ajtowns

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

src/chainparams.h

ajtowns · 2019-06-20T20:32:52Z

test/functional/feature_block.py

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.

jtimon · 2019-06-22T01:59:32Z

src/chainparams.h

-    /** Make miner stop after a block is found. In RPC, don't return until nGenProcLimit blocks are generated */
-    bool MineBlocksOnDemand() const { return fMineBlocksOnDemand; }
+    /** Whether it is possible to mine blocks on demand (no retargeting) */
+    bool MineBlocksOnDemand() const { return consensus.fPowNoRetargeting; }


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

jtimon · 2019-06-22T02:09:17Z

src/init.cpp


    fRequireStandard = !gArgs.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
-    if (chainparams.RequireStandard() && !fRequireStandard)
+    if (!chainparams.IsTestChain() && !fRequireStandard) {


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?

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 · 2019-06-22T02:13:51Z

utACK beyond nits.

ajtowns · 2019-06-25T06:42:24Z

ACK fa89bad

maflcko · 2019-06-27T15:46:07Z

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

fa89bad test: Require standard txs in regtest (MarcoFalke) fa9b419 test: Add test that mainnet requires standard txs (MarcoFalke) fa613ca chainparams: Remove unused fMineBlocksOnDemand (MarcoFalke) Pull request description: I don't see a reason why regtest should allow non-standard txs, as it makes testing mainnet behaviour such as bitcoin#15846 unnecessarily hard and unintuitive. Of course, testnet policy remains unchanged to allow propagation of non-standard txs. ACKs for top commit: ajtowns: ACK fa89bad Tree-SHA512: c4c675affb054868850bd2683aa07f4c741a448cbacb2ea8334191e105f426b0790fe6a468be61e9c5880d24154f7bf1c7075051697172dce92180c1bc3a1c90

Summary: fa89badf887dcc01e5bdece248b5e7d234fee227 test: Require standard txs in regtest (MarcoFalke) fa9b4191609c3ef75e69d391eb91e4d5c1e0bcf5 test: Add test that mainnet requires standard txs (MarcoFalke) fa613ca0a8f99c4771859de9e571878530d3ecb5 chainparams: Remove unused fMineBlocksOnDemand (MarcoFalke) Pull request description: 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. ACKs for top commit: ajtowns: ACK fa89badf887dcc01e5bdece248b5e7d234fee227 Tree-SHA512: c4c675affb054868850bd2683aa07f4c741a448cbacb2ea8334191e105f426b0790fe6a468be61e9c5880d24154f7bf1c7075051697172dce92180c1bc3a1c90 --- This is a backport of Core [[bitcoin/bitcoin#15891 | PR15891]], plus changing our own tests to allow for non-standard transactions and prevent them from failing under the new rule. Test Plan: ninja check ./test_runner.py --extended Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5764

fanquake added the Tests label Apr 25, 2019

maflcko added the TX fees and policy label Apr 25, 2019

maflcko changed the title ~~test: Require standard txs in regtest~~ test: Require standard txs in regtest by default Apr 25, 2019

Sjors approved these changes Apr 27, 2019

View reviewed changes

practicalswift reviewed May 7, 2019

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

maflcko mentioned this pull request May 7, 2019

validation: Add missing mempool locks #14193

Merged

jnewbery reviewed May 16, 2019

View reviewed changes

sipa reviewed May 22, 2019

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1904-testRequireStandard branch from faac8a2 to faf3ba4 Compare May 22, 2019 18:22

maflcko closed this May 22, 2019

maflcko reopened this May 22, 2019

maflcko force-pushed the 1904-testRequireStandard branch from faf3ba4 to faa4cba Compare May 22, 2019 21:01

DrahtBot added the Needs rebase label May 23, 2019

maflcko force-pushed the 1904-testRequireStandard branch from faa4cba to faa36e1 Compare May 23, 2019 16:52

DrahtBot removed the Needs rebase label May 23, 2019

test: Add test that mainnet requires standard txs

fa9b419

maflcko force-pushed the 1904-testRequireStandard branch from faa36e1 to fa14bda Compare June 18, 2019 19:41

DrahtBot removed the Needs rebase label Jun 18, 2019

jnewbery reviewed Jun 19, 2019

View reviewed changes

test/functional/feature_rbf.py Outdated Show resolved Hide resolved

maflcko force-pushed the 1904-testRequireStandard branch from fa14bda to fa70308 Compare June 19, 2019 22:06

jnewbery reviewed Jun 20, 2019

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

luke-jr suggested changes Jun 20, 2019

View reviewed changes

jnewbery reviewed Jun 20, 2019

View reviewed changes

maflcko force-pushed the 1904-testRequireStandard branch 2 times, most recently from fa2a0ce to fad7722 Compare June 20, 2019 16:56

ajtowns reviewed Jun 20, 2019

View reviewed changes

maflcko force-pushed the 1904-testRequireStandard branch from fad7722 to fabf87d Compare June 21, 2019 20:21

test: Require standard txs in regtest

fa89bad

maflcko force-pushed the 1904-testRequireStandard branch from fabf87d to fa89bad Compare June 21, 2019 20:45

jtimon reviewed Jun 22, 2019

View reviewed changes

maflcko merged commit fa89bad into bitcoin:master Jul 16, 2019

maflcko deleted the 1904-testRequireStandard branch July 16, 2019 20:13

jtimon mentioned this pull request Aug 1, 2019

Remove wallet settings from chainparams #16402

Merged

azuchi mentioned this pull request Jan 15, 2020

NetworkId parameter to start multiple Tapyrus-networks chaintope/tapyrus-core#37

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

test: Require standard txs in regtest by default #15891

test: Require standard txs in regtest by default #15891

Uh oh!

Conversation

maflcko commented Apr 25, 2019

Uh oh!

gmaxwell commented Apr 25, 2019

Uh oh!

harding commented Apr 25, 2019

Uh oh!

sdaftuar commented Apr 25, 2019

Uh oh!

maflcko commented Apr 25, 2019

Uh oh!

jnewbery commented Apr 25, 2019

Uh oh!

DrahtBot commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented May 1, 2019

Uh oh!

laanwj commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented May 6, 2019

Uh oh!

maflcko commented May 6, 2019

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented May 22, 2019

Uh oh!

maflcko commented May 22, 2019

Uh oh!

jnewbery commented May 23, 2019

Uh oh!

maflcko commented May 24, 2019

Uh oh!

jnewbery commented May 24, 2019

Uh oh!

maflcko commented Jun 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 20, 2019

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 25, 2019 •

edited

Loading

laanwj commented May 6, 2019 •

edited

Loading