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

feat(sighash): implement OP_MAX_SIGHASH_SUBSETS [part 5/7] #915

Open
wants to merge 2 commits into
base: feat/sighash/multisig
Choose a base branch
from

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Jan 8, 2024

Depends on #914

Motivation

Implement new OP_MAX_SIGHASH_SUBSETS.

Acceptance Criteria

  • Create new OP_MAX_SIGHASH_SUBSETS and implement op_max_sighash_subsets().
  • Implement ability to push a OP_MAX_SIGHASH_SUBSETS in HathorScript, adding support in P2PKH and MultiSig.
  • Implement TooManySighashSubsets verification in TransactionVerifier.

@glevco glevco self-assigned this Jan 8, 2024
@glevco glevco marked this pull request as ready for review January 8, 2024 02:10
@glevco glevco force-pushed the feat/sighash/multisig branch from 7fff07d to 92a19dc Compare January 8, 2024 20:10
@glevco glevco force-pushed the feat/sighash/subsets branch from bc45412 to 267c89a Compare January 8, 2024 20:11
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.36%. Comparing base (bde5cc4) to head (17df617).

Additional details and impacted files
@@                    Coverage Diff                    @@
##           feat/sighash/multisig     #915      +/-   ##
=========================================================
+ Coverage                  84.31%   84.36%   +0.04%     
=========================================================
  Files                        321      321              
  Lines                      24667    24705      +38     
  Branches                    3785     3792       +7     
=========================================================
+ Hits                       20799    20843      +44     
+ Misses                      3126     3122       -4     
+ Partials                     742      740       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glevco glevco force-pushed the feat/sighash/multisig branch from 92a19dc to 7f3180b Compare January 19, 2024 22:23
@glevco glevco force-pushed the feat/sighash/subsets branch from 267c89a to 645d9b8 Compare January 19, 2024 22:28
@glevco glevco force-pushed the feat/sighash/multisig branch from 7f3180b to 2af28c2 Compare December 9, 2024 23:18
@glevco glevco force-pushed the feat/sighash/subsets branch from 645d9b8 to fe3fb85 Compare December 9, 2024 23:19
@@ -77,6 +77,14 @@ def push_sighash(self, sighash: SighashType) -> None:
case _:
assert_never(sighash)

def push_max_sighash_subsets(self, max_subsets: int | None) -> None:
"""Push a maximum limit for custom sighash subsets."""
if max_subsets is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in a previous PR. I don't know why we would accept None and simply skip it when it happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 44f39cf

@@ -82,7 +82,8 @@ class Opcode(IntEnum):
OP_DATA_MATCH_VALUE = 0xD1
OP_SIGHASH_BITMASK = 0xE0
OP_SIGHASH_RANGE = 0xE1
OP_MAX_INPUTS_OUTPUTS = 0xE2
OP_MAX_SIGHASH_SUBSETS = 0xE2
OP_MAX_INPUTS_OUTPUTS = 0xE3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check: It is safe to change the OPCODE because these opcodes haven't been activated in any network yet. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

@@ -184,6 +185,37 @@ def test_output_not_selected(self) -> None:

self.assertEqual(str(e.value), "Output at index 0 is not signed by any input.")

@patch('hathor.transaction.scripts.opcode.is_opcode_valid', lambda _: True)
def test_too_many_sighash_subsets(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this test covers only the sighash all. What about other possible subsets? Or even subsets generated by different sighash strategies (e.g., bitmask vs range)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a new more complex test case in 44f39cf

@glevco glevco force-pushed the feat/sighash/multisig branch from 2af28c2 to bead555 Compare February 26, 2025 15:39
@glevco glevco force-pushed the feat/sighash/subsets branch from fe3fb85 to 17df617 Compare February 26, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review (WIP)
Development

Successfully merging this pull request may close these issues.

2 participants