feat(sighash): implement sighash for MultiSig [part 4/7] #914
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/sighash/range #914 +/- ##
=====================================================
Coverage ? 84.31%
=====================================================
Files ? 321
Lines ? 24667
Branches ? 3785
=====================================================
Hits ? 20799
Misses ? 3126
Partials ? 742 ☔ View full report in Codecov by Sentry. |
glevco
force-pushed
the
feat/sighash/range
branch
from
c3467a9 to
479cb8a
Compare
glevco
force-pushed
the
feat/sighash/multisig
branch
from
7fff07d to
92a19dc
Compare
glevco
force-pushed
the
feat/sighash/range
branch
from
479cb8a to
b815b65
Compare
glevco
force-pushed
the
feat/sighash/multisig
branch
from
92a19dc to
7f3180b
Compare
glevco
force-pushed
the
feat/sighash/range
branch
from
b815b65 to
e2a9b75
Compare
glevco
force-pushed
the
feat/sighash/multisig
branch
from
7f3180b to
2af28c2
Compare
| case _: | ||
| assert_never(sighash) | ||
|
|
||
| def push_inputs_outputs_limit(self, limit: InputsOutputsLimit | None) -> None: |
There was a problem hiding this comment.
It seems weird to accept None and simply do nothing. I guess the caller should take care of skipping this call.
| for signature in signatures: | ||
| while pubkey_index < len(pubkeys): | ||
| pubkey = pubkeys[pubkey_index] | ||
| new_stack = [signature, pubkey] | ||
| op_checksig(ScriptContext(stack=new_stack, logs=context.logs, extras=context.extras, settings=settings)) | ||
| context.stack = new_stack | ||
| op_checksig(context) |
There was a problem hiding this comment.
I guess that's the bug fix, right? I'd like more information about the bug, its origin, and the fix.
There was a problem hiding this comment.
It's not a preexisting bug, it's just an issue caused by the new sighash features.
op_checkmultisig calls op_checksig for each signature-pubkey pair, trying to find matches. Before, it would call op_checksig with a new ad-hoc ScriptContext, with a new stack.
Now, since the sighash configuration is persisted in the ScriptContext by the new sighash opcodes, this information was missing from this ad-hoc context passed to op_checksig. So I simply changed the code to reuse the same context, only changing the stack (and later changing it back to the original one).
The behavior is exactly the same as before, except now the same context is reused, guaranteeing the sighash config is considered.
| ] | ||
|
|
||
| private_keys = [ | ||
| '3081de304906092a864886f70d01050d303c301b06092a864886f70d01050c300e04089abeae5e8a8f75d302020800301d060960864801650304012a0410abbde27221fd302280c13fca7887c85e048190c41403f39b1e9bbc5b6b7c3be4729c054fae9506dc0f8361adcff0ea393f0bb3ca9f992fc2eea83d532691bc9a570ed7fb9e939e6d1787881af40b19fb467f06595229e29b5a6268d831f0287530c7935d154deac61dd4ced988166f9c98054912935b607e2fb332e11c95b30ea4686eb0bda7dd57ed1eeb25b07cea9669dde5210528a00653159626a5baa61cdee7f4', # noqa: E501 |
There was a problem hiding this comment.
You can break this huge line in smaller lines.
glevco
force-pushed
the
feat/sighash/range
branch
2 times, most recently
from
d7aa643 to
887a8a7
Compare
glevco
force-pushed
the
feat/sighash/multisig
branch
from
2af28c2 to
bead555
Compare
Depends on #913
Motivation
Add custom sighash support for MultiSig scripts.
Acceptance Criteria
P2PKHtoHathorScript, generalizing it (push_sighash()andpush_inputs_outputs_limit()).MultiSig.create_input_data().generate_signature_for_data()for MultiSig.op_checkmultisig()callsop_checksig().