-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Exclude SCRIPT_ENABLE_DIP0020_OPCODES from flags in mempool checks while DIP0020 is not activated yet #6299
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
fix: Exclude SCRIPT_ENABLE_DIP0020_OPCODES from flags in mempool checks while DIP0020 is not activated yet #6299
Conversation
…ks while DIP0020 is not activated yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK but let me think a bit about possible corner cases
@UdjinM6 let's add tests from 6298 ?
meanwhile tests will be broken in CI, no? |
|
No, it's #6256 which breaks tests with crashes. #6298 simply adds a p2p part to it, shouldn't break anything. Check CI: https://gitlab.com/dashpay/dash/-/pipelines/1477542913. |
c91ba8a fix: no crashes allowed (UdjinM6) a4cd1d6 fix: explicitly test no tx in mempool after invalidateblock (UdjinM6) 04b5db9 test: extend and refactor DIP0020 activation test (UdjinM6) Pull request description: ## Issue being fixed or feature implemented ~23590 backport in #6256 results in node crashes (for dashd compiled with `--enable-debug`) when we try to spend coins locked via disabled opcodes in `feature_dip0020_activation.py`. Also,~ there is only rpc part and no tests for relaying such txes via p2p. ## What was done? ## How Has This Been Tested? run test on develop and on top of #6256 + #6299 with and without `--enable-debug` ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK c91ba8a knst: utACK c91ba8a Tree-SHA512: 2ba16d6a6bb58cb98c01234ed60a8eecd4ff214d3d8386a4b8ed10f4776e0862d7794747791d82345d6031678a308df39c2dbdd361a902ee1e56cf7f05a73c1a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider #6302 instead
|
closing in fav of #6302 |
…ivated long time ago c8fd37d docs: added a comment about removed SCRIPT_ENABLE_DIP0020_OPCODES (Konstantin Akimov) 61bc300 feat: drop SCRIPT_ENABLE_DIP0020_OPCODES, make opcodes available from genesis block (Konstantin Akimov) 0e55abd feat: remove feature_dip0020_activationl.py functional test and related code (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented it's alternate solution for #6299 to fix a crash reported #6256 ## What was done? Removed code related to DIP0020 activation for various OP codes. DIP0020 is activated long time ago and no any historical blocks are violating rules, removing it's backwards compatible. ## How Has This Been Tested? Run unit and functional tests. See also changes in data for unit tests and removed functional test. It also re-index mainnet and testnet successfully ``` src/qt/dash-qt -reindex -assumevalid=0 src/qt/dash-qt -testnet -reindex -assumevalid=0 ``` Also extra test is done with bitcoin#23590 - no crash with it in `feature_dip0020_activation.py` [modified assuming it is always activated] ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK c8fd37d PastaPastaPasta: utACK c8fd37d Tree-SHA512: 05ddda4e8fb66305995e91c8a04fbda690aef8fb82acb23b7d62f302da60b5ec7e7a97bd988efd2523dbd9cafde9f4b65cae2db9e4b5257464ce1c8fcca6a40f
Issue being fixed or feature implemented
Should fix the crash reported in #6256
What was done?
How Has This Been Tested?
Breaking Changes
n/a, only affects txes prior to DIP0020 activation
Checklist: