Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Require standard txs in regtest by default #15891

Merged
merged 3 commits into from Jul 16, 2019

Conversation

@MarcoFalke
Copy link
Member

commented Apr 25, 2019

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 fanquake added the Tests label Apr 25, 2019

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Sounds good to me.

@harding

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Yes, please.

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

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 MarcoFalke changed the title test: Require standard txs in regtest test: Require standard txs in regtest by default Apr 25, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

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

Sjors approved these changes Apr 27, 2019

Copy link
Member

left a comment

tACK faac8a2 on macOS 10.14.4 and Ubuntu 18.04

doc/release-notes.md Outdated Show resolved Hide resolved
@@ -67,14 +67,15 @@ def set_test_params(self):
self.num_nodes = 2
self.extra_args = [
[
"-acceptnonstdtxn=1",

This comment has been minimized.

Copy link
@Sjors

Sjors Apr 27, 2019

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 27, 2019

Author Member

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 1, 2019

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

This comment has been minimized.

Copy link
Member

commented May 6, 2019

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

This comment has been minimized.

Copy link
Member

commented May 6, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

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 Outdated Show resolved Hide resolved
@jnewbery
Copy link
Member

left a comment

tested ACK faac8a2

A few small suggestions inline

doc/release-notes.md Outdated Show resolved Hide resolved
self.extra_args = [[], ["-acceptnonstdtxn=0"]]
self.extra_args = [
["-acceptnonstdtxn=1"],
["-acceptnonstdtxn=0"],

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2019

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 16, 2019

Author Member

I think it is easier to read due to symmetry

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"]]
self.extra_args = [
["-whitelist=127.0.0.1", "-acceptnonstdtxn=1", "-vbparams=segwit:0:999999999999"],
["-whitelist=127.0.0.1", "-acceptnonstdtxn=0", "-vbparams=segwit:0:999999999999"],

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2019

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 16, 2019

Author Member

I think it is easier to read due to symmetry

src/init.cpp Outdated Show resolved Hide resolved

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch from faac8a2 to faf3ba4 May 22, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

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 MarcoFalke closed this May 22, 2019

@MarcoFalke MarcoFalke reopened this May 22, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch from faf3ba4 to faa4cba May 22, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch from faa4cba to faa36e1 May 23, 2019

@DrahtBot DrahtBot removed the Needs rebase label May 23, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 23, 2019

utACK cd338016f5663703cd6eb87bd4403a8fcb4f27a9

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

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

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 24, 2019

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

utACK faa36e1

MarcoFalke added some commits Apr 23, 2019

chainparams: Remove unused fMineBlocksOnDemand
It is equal to consensus.fPowNoRetargeting

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch from faa36e1 to fa14bda Jun 18, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

Rebased due to conflict in a test file

@DrahtBot DrahtBot removed the Needs rebase label Jun 18, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch from fa14bda to fa70308 Jun 19, 2019

doc/release-notes.md Outdated Show resolved Hide resolved
src/chainparams.h Outdated Show resolved Hide resolved
@@ -78,7 +78,7 @@ class FullBlockTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
self.setup_clean_chain = True
self.extra_args = [[]]
self.extra_args = [['-acceptnonstdtxn=1']] # This is a consensus block test, we don't care about tx policy

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 20, 2019

Member

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jun 20, 2019

Contributor

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.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 21, 2019

Member

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

@jnewbery
Copy link
Member

left a comment

ACK fa70308 modulo one requested change to the release notes.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch 2 times, most recently from fa2a0ce to fad7722 Jun 20, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

ACK fad7722

@ajtowns
Copy link
Contributor

left a comment

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

src/chainparams.h Outdated Show resolved Hide resolved
@@ -78,7 +78,7 @@ class FullBlockTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
self.setup_clean_chain = True
self.extra_args = [[]]
self.extra_args = [['-acceptnonstdtxn=1']] # This is a consensus block test, we don't care about tx policy

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jun 20, 2019

Contributor

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.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch from fad7722 to fabf87d Jun 21, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-testRequireStandard branch from fabf87d to fa89bad Jun 21, 2019

/** 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; }

This comment has been minimized.

Copy link
@jtimon

jtimon Jun 22, 2019

Member

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

@@ -1150,8 +1150,9 @@ bool AppInitParameterInteraction()
}

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

This comment has been minimized.

Copy link
@jtimon

jtimon Jun 22, 2019

Member

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?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jun 22, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

utACK beyond nits.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

ACK fa89bad

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

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

@MarcoFalke MarcoFalke merged commit fa89bad into bitcoin:master Jul 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jul 16, 2019

Merge bitcoin#15891: test: Require standard txs in regtest by default
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

@MarcoFalke MarcoFalke deleted the MarcoFalke:1904-testRequireStandard branch Jul 16, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 29, 2019

Merge bitcoin#15891: test: Require standard txs in regtest by default
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.