SigType check for cost-analysis in SegwitV1 scripts #340

SarcasticNastik · 2022-04-07T16:36:21Z

Previously, the struct CompilerExtData provided cost analysis for a given script considering the SegwitV0 standards. This PR aims to track the type/ (context) of the script and provide the respective cost analysis.

sanket1729

Let some comments. I think we can clean this up a lot if we move the Ctx as suggested. Please try that and let me know if there are any issues.

sanket1729 · 2022-04-10T21:01:36Z

src/policy/compiler.rs

+                + match Ctx::sig_type() {
+                    SigType::Ecdsa => 73.0 * k as f64,
+                    SigType::Schnorr => 64. * k as f64,
+                },


You would need a from_multi_a here. Add appropriate unreachable! depending on context

Addressed in 2565f5e.

I think the implementation of from_multi_a is missing here for CompilerExtData. Note that the multi_a fragment operates differently from multi as it consumes empty signatures for dissatisfactions.

sanket1729 · 2022-04-10T21:07:21Z

src/policy/compiler.rs

-            dissat_cost: Some(33.0),
+            sat_cost: match Ctx::sig_type() {
+                SigType::Ecdsa => 33.0,
+                SigType::Schnorr => 32.,


This should be 33 because we also include the push_bytes in the cost.

src/policy/compiler.rs

sanket1729 · 2022-04-10T21:25:12Z

src/policy/compiler.rs

-            sat_cost: 73.0,
+            sat_cost: match Ctx::sig_type() {
+                SigType::Ecdsa => 73.,
+                SigType::Schnorr => 64.,


I think this should probably be 66 so that we include the varint prefix and the hash type flag. This is because the key itself is 64 bytes. It can optionally contain a sighash flag (1 byte) and there is also a pre-fix of varint type denoting the length of sig that follows. It will end up as <var_int_prefix><sig><sighash_type> on the stack.

We should also add a comment explaining why this is 66 and we need to update this everywhere.

Addressed in 2565f5e

sanket1729 · 2022-04-10T21:25:45Z

src/policy/compiler.rs

+        })
+    }
+
+    fn and_n(a: Self, b: Self) -> Result<Self, types::ErrorKind> {


Note to self and reviewers: This is not new code added. This is moved.

sanket1729 · 2022-04-10T21:27:50Z

src/policy/compiler.rs

@@ -470,8 +521,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> AstElemExt<Pk, Ctx> {
        r: &AstElemExt<Pk, Ctx>,
    ) -> Result<AstElemExt<Pk, Ctx>, types::Error<Pk, Ctx>> {
        let lookup_ext = |n| match n {
-            0 => Some(l.comp_ext_data),
-            1 => Some(r.comp_ext_data),
+            0 => Some(l.comp_ext_data.clone()),


Why do you need .clone() here? CompExtData is already Copy

In the previous version having CompilerExtData<Ctx: ScriptKey>, the closure type changes to FnOnce since it moves the variable l out of its environment. This SO post answers for this issue.

sanket1729

Almost there

sanket1729 · 2022-04-11T03:50:40Z

src/policy/compiler.rs

-            dissat_cost: Some(1.0 + 34.0),
+            sat_cost: match Ctx::sig_type() {
+                SigType::Ecdsa => 73.0 + 34.0,
+                SigType::Schnorr => 66.0 + 32.0,


They key cost here and elsewhere would be 33 instead of 32

sanket1729 · 2022-04-11T03:53:10Z

src/policy/compiler.rs

+                + match Ctx::sig_type() {
+                    SigType::Ecdsa => 73.0 * k as f64,
+                    SigType::Schnorr => 64. * k as f64,
+                },


I think the implementation of from_multi_a is missing here for CompilerExtData. Note that the multi_a fragment operates differently from multi as it consumes empty signatures for dissatisfactions.

sanket1729 · 2022-04-18T22:30:24Z

src/miniscript/types/correctness.rs

@@ -164,7 +165,7 @@ impl Property for Correctness {
        }
    }

-    fn from_multi(_: usize, _: usize) -> Self {
+    fn from_multi<Ctx: ScriptContext>(_: usize, _: usize) -> Self {


Why do we need ctx here? multi should have SigType Ecdsa and multi_a should have Schnorr

I think this would also resolve the awkward things that you had to do later with the 0 value

Initially I introduced this to keep a check on misuse of SigType, but that is indeed taken care of in the consensus checks. Will update that in the following commit!

sanket1729

code review ACK 25560a3

@sanket1729

747876e Rectify `from_multi_a` max satisfaction size (Aman Rojjha) Pull request description: Cost incurred for a single Schnorr signature in the witness stack := `<var_int><64-byte signature><sig_type>` = 1 + 64 + 1 (as clarified by @sanket1729 in #340 ). ACKs for top commit: sanket1729: ACK 747876e Tree-SHA512: f20652240f1daa7b33b1d525870114e0bd6412e82845e80a94c616dedcfb3d8ae2f7ef19d7c08760f79270e0d465c18b713eff2e3e599b45c65f2a64da36713b

SarcasticNastik · 2022-05-16T05:06:33Z

@sanket1729 I overlooked that you merged #346 and not this PR. My apologoies!
I've rebased this PR onto master.

SarcasticNastik · 2022-05-16T06:10:42Z

@sanket1729 @tcharding The Integration test fails at this run here because of curl stopping abruptly which should not have been the case. Kindly take a look into that, thanks!
We can have some retry policy on download, for e.g.

curl --connect-timeout 5 \
    --max-time 10 \
    --retry 5 \
    --retry-delay 0 \
    --retry-max-time 40 \
    https://bitcoincore.org/bin/bitcoin-core-$BITCOINVERSION/bitcoin-$BITCOINVERSION-x86_64-linux-gnu.tar.gz

P.S. Test passes on my fork and the next run :p.

sanket1729 · 2022-05-16T18:13:12Z

P.S. Test passes on my fork and the next run :p.

Unfortunately, there is little we can do if we face network connection issues. Trying again is the best solution we have.

sanket1729

ACK 52d6647

SarcasticNastik marked this pull request as ready for review April 7, 2022 18:40

sanket1729 reviewed Apr 10, 2022

View reviewed changes

SarcasticNastik force-pushed the refactor/CompilerExtData branch from b3825d3 to 2565f5e Compare April 10, 2022 23:40

sanket1729 reviewed Apr 11, 2022

View reviewed changes

SarcasticNastik force-pushed the refactor/CompilerExtData branch from 2565f5e to 9d8402f Compare April 18, 2022 21:25

sanket1729 reviewed Apr 18, 2022

View reviewed changes

SarcasticNastik force-pushed the refactor/CompilerExtData branch 2 times, most recently from 7cb61e1 to 25560a3 Compare April 18, 2022 22:59

SarcasticNastik mentioned this pull request Apr 18, 2022

Rectify from_multi_a max satisfaction size #346

Merged

sanket1729 previously approved these changes Apr 18, 2022

View reviewed changes

SarcasticNastik closed this May 4, 2022

SarcasticNastik reopened this May 16, 2022

SarcasticNastik dismissed sanket1729’s stale review via 60d6956 May 16, 2022 05:02

SarcasticNastik force-pushed the refactor/CompilerExtData branch from 25560a3 to 60d6956 Compare May 16, 2022 05:02

SarcasticNastik force-pushed the refactor/CompilerExtData branch 3 times, most recently from ba29af1 to 63d8687 Compare May 16, 2022 05:54

Add SigType check in CompilerExtData for deciding key byte-lengths

52d6647

SarcasticNastik force-pushed the refactor/CompilerExtData branch from 63d8687 to 52d6647 Compare May 16, 2022 06:21

sanket1729 approved these changes May 19, 2022

View reviewed changes

sanket1729 merged commit df6096a into rust-bitcoin:master May 19, 2022

sanket1729 mentioned this pull request May 19, 2022

Improve timelock code #414

Merged

SarcasticNastik deleted the refactor/CompilerExtData branch June 3, 2022 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SigType check for cost-analysis in SegwitV1 scripts #340

SigType check for cost-analysis in SegwitV1 scripts #340

SarcasticNastik commented Apr 7, 2022

sanket1729 left a comment

sanket1729 Apr 10, 2022

SarcasticNastik Apr 10, 2022

sanket1729 Apr 11, 2022

sanket1729 Apr 10, 2022

sanket1729 Apr 10, 2022

SarcasticNastik Apr 11, 2022

sanket1729 Apr 10, 2022

sanket1729 Apr 10, 2022

SarcasticNastik Apr 11, 2022 •

edited

Loading

sanket1729 left a comment

sanket1729 Apr 11, 2022

sanket1729 Apr 11, 2022

sanket1729 Apr 18, 2022

sanket1729 Apr 18, 2022

SarcasticNastik Apr 18, 2022

sanket1729 left a comment

SarcasticNastik commented May 16, 2022 •

edited

Loading

SarcasticNastik commented May 16, 2022 •

edited

Loading

sanket1729 commented May 16, 2022

sanket1729 left a comment

SigType check for cost-analysis in SegwitV1 scripts #340

SigType check for cost-analysis in SegwitV1 scripts #340

Conversation

SarcasticNastik commented Apr 7, 2022

sanket1729 left a 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

SarcasticNastik Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

sanket1729 left a 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

sanket1729 left a comment

Choose a reason for hiding this comment

SarcasticNastik commented May 16, 2022 • edited Loading

SarcasticNastik commented May 16, 2022 • edited Loading

sanket1729 commented May 16, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

SarcasticNastik Apr 11, 2022 •

edited

Loading

SarcasticNastik commented May 16, 2022 •

edited

Loading

SarcasticNastik commented May 16, 2022 •

edited

Loading