-
Notifications
You must be signed in to change notification settings - Fork 38
feat: deprecate unnecessary opcodes #1563
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
base: master
Are you sure you want to change the base?
Conversation
6b863ed to
746d648
Compare
|
| Branch | feat/remove-unused-opcodes |
| Testbed | ubuntu-22.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚷 view threshold | 1.70 m(-1.22%)Baseline: 1.72 m | 1.55 m (91.12%) | 2.06 m (82.31%) |
d094ac2 to
10a4030
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1563 +/- ##
==========================================
- Coverage 86.27% 85.76% -0.51%
==========================================
Files 437 437
Lines 33662 33707 +45
Branches 5269 5268 -1
==========================================
- Hits 29042 28909 -133
- Misses 3611 3796 +185
+ Partials 1009 1002 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e4b1772 to
9fe977f
Compare
fba3149 to
ca88671
Compare
ca88671 to
c7ccccb
Compare
| count_checkdatasig_op=False, | ||
| nanocontracts=False, | ||
| fee_tokens=False, | ||
| opcodes_version=OpcodesVersion.V1, |
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.
All these settings are weird to me.
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.
Again, they're compatible with the base PR which is compatible with what's in master. All these are unused here, as this class only calls verify_basic.
|
|
||
|
|
||
| def execute_op_code(opcode: Opcode, context: ScriptContext) -> None: | ||
| def execute_op_code(opcode: Opcode, context: ScriptContext, version: OpcodesVersion) -> None: |
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.
I'd rather refactor it to a class that will be first configured and then will execute the script. Maybe it's too much for this PR.
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.
I agree we could do it in another PR.
| """Verify inputs signatures and ownership and all inputs actually exist""" | ||
| self._verify_inputs(self._settings, tx, params, skip_script=skip_script) | ||
|
|
||
| @classmethod |
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.
Why?
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.
Because this method doesn't use any instance attributes, so I can call TransactionVerifier._verify_inputs in the consensus mempool re-verification without having to inject the verifier instance there. The same structure was used for the _verify_sigops_output re-verification which is already in place.
| try: | ||
| NanoHeaderVerifier._verify_nc_signature(self._settings, tx, params) | ||
| except Exception: | ||
| return False |
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.
Should we log the exception? I guess, nothing else should happen besides the tx failing the verification.
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.
Done in 0dfe92e
9fe977f to
73d3e56
Compare
c7ccccb to
846a2b7
Compare
0dfe92e to
3e32c9b
Compare
Motivation
There are unused opcodes that were once created for a cancelled implementation. This PR defines a Feature Activation to remove them.
Acceptance Criteria
.venvto ignore ofyamllint. It was complaining in my local env sometimes.Feature.OPCODES_V2and respective settingHathorSettings.ENABLE_OPCODES_V2, defaulting to disabled.OpcodeVersion, removing deprecated opcodes onV2, which is configured via feature activation.SignedData.checksigis set to useV2directly, without feature activation. This is allowed since there are no blueprints withSignedDataon mainnet.V2directly, so we reject txs with deprecated opcodes in the mempool.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged