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

policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) #29843

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Apr 10, 2024

This PR changes -acceptnonstdtxn=1 so that it also skips the non-mandatory script checks, allowing txs that (eg) use OP_NOPx or OP_SUCCESS into the mempool. This remains only available on test nets.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 10, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK 1440000bytes, benthecarman, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)

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.

@ajtowns
Copy link
Contributor Author

ajtowns commented Apr 10, 2024

Previous discussion in #28334.

If you enable this feature as a non-mining node, you risk having txs pinned in your mempool that conflict or double-spend standard txs in the normal mempool, which may cause problems with applications or layer-2 protocols.

If you enable this feature as a mining node, you have a small risk of ending up with invalid txs remaining in your mempool if a tx non-standard tx is accepted in your mempool and remains in there during a soft-fork activation that renders the tx invalid. At that point if that tx is included in blocks you generate, they will be invalid.

For these reasons this feature continues to only be available on test nets, not mainnet.

Having this be available in mainline bitcoin may make it easier to observe test transactions using proposed new soft-fork features (eg OP_CAT or SIGHASH_APO is enabled via the inquisition client which considers those txs standard, but the mainline client will consider them non-standard and has no option to accept them). discussion link

@1440000bytes
Copy link

Concept ACK

@benthecarman
Copy link
Contributor

Concept ACK this would be nice for testing soft forks on test networks

@luke-jr
Copy link
Member

luke-jr commented Apr 21, 2024

Seems like this goes beyond the expected behaviour. Maybe make it =2 at least?

@glozow
Copy link
Member

glozow commented Apr 22, 2024

concept ACK fwiw

@instagibbs
Copy link
Member

Seems like this goes beyond the expected behaviour. Maybe make it =2 at least?

Since these are test-network-only changes, are we required to preserve current behavior?

@twosatsmaxi
Copy link

Following.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23651574970

…n=0 node

Prepare for updating -acceptnonstdtxn to allow txns that violate
STANDARD_SCRIPT_VERIFY_FLAGS but not MANDATORY_SCRIPT_VERIFY_FLAGS by
checking the non-mandatory flags with node that enforces standardness.
@ajtowns ajtowns force-pushed the 202303-acceptnonstdscript branch from 2320ece to affee04 Compare May 16, 2024 15:53
@ajtowns
Copy link
Contributor Author

ajtowns commented May 16, 2024

Bumped past #29086

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants