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

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

Depends on #911

Motivation

The previous PR introduced the structure for sighash types, including sighash bitmask for P2PKH scripts. It fixed existing tests, but did not introduce any new tests. This PR does this.

Acceptance Criteria

Implement new tests for features introduced in PR Ops: Implement consensus check #125

codecov · 2024-01-08T02:18:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.35%. Comparing base (922055e) to head (75790a3).

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feat/sighash/bitmask     #912      +/-   ##
========================================================
+ Coverage                 84.12%   84.35%   +0.22%     
========================================================
  Files                       321      321              
  Lines                     24601    24601              
  Branches                   3773     3773              
========================================================
+ Hits                      20696    20751      +55     
+ Misses                     3168     3113      -55     
  Partials                    737      737

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

msbrogli · 2024-01-26T18:56:47Z

tests/tx/scripts/test_p2pkh.py

Should we test the the script regex can correctly match all these cases?

The script regex is only made to match output scripts, isn't it? It wouldn't match any of those input data

msbrogli · 2024-01-26T18:58:20Z

tests/tx/scripts/test_script_context.py

+
+def test_defaults() -> None:
+    settings = Mock()
+    settings.MAX_NUM_OUTPUTS = 99


Why not test using the current value?

I think doing it like this asserts that the code is correctly retrieving information from settings, instead of having a hardcoded value that happens to be the same as the one we have in settings. I added a test for the current value in 2bf7549, though

msbrogli · 2024-01-26T18:59:26Z

tests/tx/scripts/test_script_context.py

+        (0b00, set()),
+        (0b01, {0}),
+        (0b10, {1}),
+        (0b11, {0, 1}),


Should we add one more bit?

You mean testing 0b111, for example? That would raise an exception, as the test tx only has 2 outputs. This error is checked in test_sighash.py.

msbrogli · 2024-01-26T19:01:41Z

tests/tx/scripts/test_sighash.py

+            storage=self.manager1.tx_storage,
+            timestamp=token_creation_tx.timestamp + 1
+        )
+        self.manager1.cpu_mining_service.resolve(atomic_swap_tx)


Why are you resolving the tx here if it's gonna be modified just after this point?

Because the tx is going to be verified on line 99, so it needs to be resolved.

But you are modifying the transaction on line 90, so the hash will become invalid again. Am I missing something?

It passes the verification like this anyway. But I moved it closer to the verification line in 9147127

msbrogli · 2024-01-26T19:06:44Z

tests/tx/scripts/test_sighash.py

+        )
+
+        # At this point, the tx is partial. The inputs are valid, but they're mismatched with outputs
+        self.manager1.verification_service.verifiers.tx.verify_inputs(atomic_swap_tx)


Shouldn't it fail since there's no sighash all?

This requirement is only implemented in Part 7: #917

msbrogli · 2024-01-26T19:13:17Z

tests/tx/test_scripts.py

+        with self.assertRaises(MissingStackItems):
+            op_sighash_bitmask(ScriptContext(stack=[b''], extras=Mock(), logs=[], settings=Mock()))
+
+        with self.assertRaises(AssertionError):


Why would it be an assertion error? Is it because they are not bytes?

msbrogli · 2024-01-26T19:15:37Z

tests/tx/test_scripts.py

+        with self.assertRaises(CustomSighashModelInvalid):
+            op_sighash_bitmask(context)
+
+        context.stack = [bytes([0b111]), bytes([0b101])]


Maybe add tests with different endianness.

What do you mean? Endianness has no meaning in this context as this string of bits is never interpreted as a number, it's simply a string of bits

I meant that 0b1101 should be different from 0b1011.

Ah ok, done in 75790a3

msbrogli · 2024-01-26T19:16:19Z

tests/tx/test_scripts.py

+    def test_execute_op_code(self) -> None:
+        with (
+            patch('hathor.transaction.scripts.opcode.is_opcode_valid', lambda _: False),
+            self.assertRaises(ScriptError)


Should it be an INVALID_OPCODE or UNKNOWN_OPCODE?

I believe those have different meanings. An unknown opcode is a concept that already exists, for example, a number that doesn't represent any option in the Opcode enum. An invalid opcode, however, is a new concept introduced by this project. It's an opcode that we now exists, but it's not activated (valid) yet.

I got it. But shouldn't we capture the more specific exception? We have to be very careful in these changes because it's a critical part of the full node.

Got it, I added assertions for the exception message in 75790a3

msbrogli · 2024-01-26T19:17:13Z

tests/tx/test_scripts.py

+            patch('hathor.transaction.scripts.opcode.is_opcode_valid', lambda _: True),
+            self.assertRaises(ScriptError)
+        ):
+            execute_op_code(opcode=Opcode.OP_0, context=Mock())


What are we testing here?

That when is_opcode_valid returns True, execution must fail if it's not a "function opcode". I added a comment for it in 2bf7549

msbrogli · 2024-01-26T19:17:20Z

tests/tx/test_scripts.py

+            execute_op_code(opcode=Opcode.OP_0, context=Mock())
+
+        with patch('hathor.transaction.scripts.opcode.op_dup') as op_mock:
+            execute_op_code(opcode=Opcode.OP_DUP, context=Mock())


What are we testing here?

That the function associated with a valid opcode is correctly called. I added a comment for it in 2bf7549

msbrogli · 2024-01-26T20:49:03Z

tests/tx/scripts/__init__.py

Maybe add tests to make sure that a sighash all will mark all inputs and outputs as selected at least once.

This is implemented in Part 7: https://github.com/HathorNetwork/hathor-core/pull/917/files

glevco self-assigned this Jan 8, 2024

glevco added the tests label Jan 8, 2024

glevco mentioned this pull request Jan 8, 2024

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

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 force-pushed the feat/sighash/bitmask branch from 9ebcf3e to 9042fd7 Compare January 8, 2024 20:10

glevco force-pushed the tests/sighash/bitmask branch from 78b027b to 1b44d48 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

glevco force-pushed the tests/sighash/bitmask branch from 1b44d48 to fc82733 Compare January 19, 2024 22:15

msbrogli requested changes Jan 26, 2024

View reviewed changes

msbrogli reviewed Jan 26, 2024

View reviewed changes

glevco force-pushed the tests/sighash/bitmask branch from 86d10b4 to 2bf7549 Compare January 30, 2024 01:20

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

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

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

glevco force-pushed the tests/sighash/bitmask branch from 2bf7549 to ad1def9 Compare December 9, 2024 23:16

msbrogli approved these changes Dec 20, 2024

View reviewed changes

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

tests(sighash): add sighash bitmask tests

890c630

glevco force-pushed the tests/sighash/bitmask branch from ad1def9 to 890c630 Compare February 25, 2025 21:04

glevco added 2 commits February 25, 2025 18:32

fix tests

9147127

review changes

75790a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

glevco commented Jan 8, 2024 •

edited

Loading

codecov bot commented Jan 8, 2024 •

edited

Loading

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Dec 20, 2024

glevco Feb 25, 2025

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Dec 20, 2024

glevco Feb 25, 2025

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Dec 20, 2024

glevco Feb 25, 2025

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Jan 26, 2024

glevco Jan 30, 2024

msbrogli Jan 26, 2024

glevco Jan 30, 2024

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

Are you sure you want to change the base?

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

Conversation

glevco commented Jan 8, 2024 • edited Loading

Motivation

Acceptance Criteria

codecov bot commented Jan 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glevco commented Jan 8, 2024 •

edited

Loading

codecov bot commented Jan 8, 2024 •

edited

Loading