feat(sighash): implement sighash bitmask [part 1/7] by glevco · Pull Request #911 · HathorNetwork/hathor-core

glevco · 2024-01-08T01:57:03Z

Depends on #851

Motivation

Implement the new SIGHASH structure. This PR implements everything necessary for creating P2PKH inputs/outputs using OP_SIGHASH_BITMASK. That is, creating atomic swap txs is already fully functional, but gated to prevent a fork (this will later be toggled by Feature Activation).

Adding support for OP_SIGHASH_RANGE and MultiSig is straightforward and will be done in separate PRs. Tests for new functionality will also be implemented in a separate PR.

Acceptance Criteria

New models SighashAll, SighashBitmask, SighashRange, and SighashLimit.
Implement evaluate_final_context() and change execute_eval() to call it.
Change execute_eval() and script_eval() to return the selected outputs from the context.
Create new opcodes and implement op_sighash_bitmask() and op_max_inputs_outputs().
Update op_checksig() to use the ScriptContext for signature verification.
Implement is_opcode_valid() and use it to gate new opcodes in execute_op_code().
Update P2PKH.create_input_data() to support SighashBitmask and SighashLimit.
Update ScriptContext with methods for interacting with sighash.
Implement Transaction.get_custom_sighash_data() and change get_sighash_all() to unify implementations.
Update TransactionVerifier.verify_inputs() to check for unsigned outputs.
Update existing tests.

codecov · 2024-01-08T02:15:08Z

Codecov Report

Attention: Patch coverage is 63.33333% with 66 lines in your changes missing coverage. Please review.

Project coverage is 84.12%. Comparing base (dbcff55) to head (922055e).

Files with missing lines	Patch %	Lines
hathor/transaction/scripts/opcode.py	29.78%	32 Missing and 1 partial ⚠️
hathor/transaction/scripts/script_context.py	54.83%	12 Missing and 2 partials ⚠️
hathor/transaction/scripts/p2pkh.py	20.00%	6 Missing and 2 partials ⚠️
hathor/transaction/transaction.py	66.66%	6 Missing ⚠️
hathor/transaction/scripts/sighash.py	90.00%	3 Missing ⚠️
hathor/verification/transaction_verifier.py	84.61%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
- Coverage   84.26%   84.12%   -0.15%     
==========================================
  Files         320      321       +1     
  Lines       24455    24601     +146     
  Branches     3744     3773      +29     
==========================================
+ Hits        20608    20696      +88     
- Misses       3113     3168      +55     
- Partials      734      737       +3

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

hathor/transaction/exceptions.py

hathor/transaction/scripts/execute.py

tests/tx/test_scripts.py

hathor/transaction/transaction.py

hathor/verification/transaction_verifier.py

msbrogli · 2024-01-26T20:51:22Z

hathor/transaction/scripts/opcode.py

+            outputs=bytes_to_int(outputs)
+        )
+    except Exception as e:
+        raise CustomSighashModelInvalid('Could not construct sighash bitmask.') from e


How can we differentiate a code bug from a bad input? I feel that we should capture only the exceptions raised by the bytes_to_int() and let all others blow up.

Changed in b611297 to catch only pydantic.ValidationError. The bytes_to_int() function does not raise any exceptions explicitly, so the only expected exception to be raised here is the model validation from pydantic.

However, I believe this is a shortcoming caused by the try-except model itself. There's no way to formally differentiate between exceptions caused by bugs or bad inputs, because there's no formally typed list of expected exceptions. In my opinion we should improve this by using result monads throughout the code.

msbrogli · 2024-01-26T20:51:55Z

hathor/transaction/scripts/opcode.py

+            max_inputs=bytes_to_int(max_inputs),
+            max_outputs=bytes_to_int(max_outputs)
+        )
+    except Exception as e:


Done in b611297, see #911 (comment)

hathor/transaction/scripts/opcode.py

github-actions · 2024-08-13T00:02:46Z

Bencher Report

Branch	feat/sighash/bitmask
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.50 m (-7.99%) Baseline: 1.63 m	1.47 m (97.82%)	1.79 m (83.65%)

🐰 View full continuous benchmarking report in Bencher

msbrogli · 2024-12-10T23:12:59Z

hathor/transaction/scripts/execute.py



-def script_eval(tx: Transaction, txin: TxInput, spent_tx: BaseTransaction) -> None:
+def script_eval(tx: Transaction, spent_tx: BaseTransaction, *, input_index: int) -> 'ScriptContext':


As you're already changing the method signature, you can put it all as kwargs only, right? The docstring is also outdated with the new signature.

Done in 2f8579f

msbrogli · 2024-12-20T05:10:14Z

hathor/verification/transaction_verifier.py

-                self.verify_script(tx=tx, input_tx=input_tx, spent_tx=spent_tx)
+                script_context = self.verify_script(tx=tx, spent_tx=spent_tx, input_index=input_index)
+                selected_outputs = script_context.get_selected_outputs()
+                all_selected_outputs = all_selected_outputs.union(selected_outputs)


You can use all_selected_outputs.update(selected_outputs) to prevent creating a new set.

glevco self-assigned this Jan 8, 2024

glevco mentioned this pull request Jan 8, 2024

tests(sighash): add sighash bitmask tests [part 2/7] #912

Open

glevco marked this pull request as ready for review January 8, 2024 02:10

glevco requested review from jansegre and msbrogli as code owners January 8, 2024 02:10

glevco commented Jan 8, 2024

View reviewed changes

hathor/transaction/exceptions.py Show resolved Hide resolved

glevco force-pushed the feat/sighash/bitmask branch from 9ebcf3e to 9042fd7 Compare January 8, 2024 20:10

glevco force-pushed the feat/sighash/bitmask branch 3 times, most recently from 5df23a6 to 9f778fd Compare January 19, 2024 22:13

msbrogli requested changes Jan 26, 2024

View reviewed changes

hathor/transaction/scripts/execute.py Show resolved Hide resolved

tests/tx/test_scripts.py Show resolved Hide resolved

hathor/transaction/transaction.py Outdated Show resolved Hide resolved

hathor/verification/transaction_verifier.py Show resolved Hide resolved

msbrogli reviewed Jan 26, 2024

View reviewed changes

hathor/transaction/scripts/opcode.py Show resolved Hide resolved

glevco force-pushed the feat/sighash/bitmask branch from b611297 to 783f397 Compare March 5, 2024 16:11

jansegre previously approved these changes Mar 25, 2024

View reviewed changes

glevco added this to the Test Gabriel milestone Aug 8, 2024

glevco dismissed jansegre’s stale review via ebebb1f August 12, 2024 23:40

glevco force-pushed the feat/sighash/bitmask branch from 783f397 to ebebb1f Compare August 12, 2024 23:40

glevco force-pushed the feat/sighash/bitmask branch from ebebb1f to 3c1f9a7 Compare August 20, 2024 21:18

glevco force-pushed the feat/sighash/bitmask branch from 3c1f9a7 to a457f0a Compare December 9, 2024 23:14

msbrogli reviewed Dec 10, 2024

View reviewed changes

msbrogli previously approved these changes Dec 20, 2024

View reviewed changes

glevco force-pushed the feat/sighash/bitmask branch from 2f8579f to 6bbc55a Compare February 25, 2025 20:59

feat(sighash): implement sighash bitmask

922055e

glevco dismissed msbrogli’s stale review via 922055e February 25, 2025 21:02

glevco force-pushed the feat/sighash/bitmask branch from 6bbc55a to 922055e Compare February 25, 2025 21:02

jansegre approved these changes Feb 25, 2025

View reviewed changes

glevco mentioned this pull request Feb 25, 2025

feat(sighash): implement sighash range [part 3/7] #913

Open

glevco moved this from In Review (WIP) to Todo in Hathor Network Mar 26, 2025

msbrogli force-pushed the master branch 2 times, most recently from eb416fa to 21d7909 Compare February 12, 2026 23:12



		def script_eval(tx: Transaction, txin: TxInput, spent_tx: BaseTransaction) -> None:
		def script_eval(tx: Transaction, spent_tx: BaseTransaction, *, input_index: int) -> 'ScriptContext':

Conversation

glevco commented Jan 8, 2024

Motivation

Acceptance Criteria

Uh oh!

codecov bot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msbrogli Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 8, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

msbrogli Dec 10, 2024 •

edited

Loading