BIP348 OP_CHECKSIGFROMSTACK by instagibbs · Pull Request #72 · bitcoin-inquisition/bitcoin

instagibbs · 2025-03-13T18:33:13Z

Partial resurrection of #45 , focused specifically on BIP348 spec adherence.

I kept the limited tests that pertained to OP_CHECKSIGFROMSTACK, but clearly it will need to cover the basic cases the BIP calls for.

If there's any intention to review this I can add the testing we think is appropriate, and activate on regtest/signet/whatever.

src/script/interpreter.cpp

dergoegge

Code review ACK c401fa4 - Seems reasonable to enable this for experimentation (in the same vein as the other already deployed changes on signet).

Reviewed that this matches the spec in the BIP as well as bitcoin#32247 (the implementations are different but behave the same afaict).

instagibbs · 2025-06-11T12:37:00Z

I helped at fairly complete tests to bitcoin#32247 so those can be ported over (again, if there is interest by the maintainer here)

darosior

Looks good to me, save for setting discouragement by standardness before activation. Happy to reACK after you pull in the additional test coverage.

darosior · 2025-06-20T14:44:54Z

src/script/interpreter.h

+    SCRIPT_VERIFY_CHECKSIGFROMSTACK = (1U << 28),
+
+    // Making OP_CHECKSIGFROMSTACK non-standard
+    SCRIPT_VERIFY_DISCOURAGE_CHECKSIGFROMSTACK = (1U << 29),


You didn't add it to the call to DepDiscourageFlags() in PolicyScriptChecks()

stevenroose · 2025-06-22T20:56:18Z

Concept ACK and ACK of correctness of opcode implementation compared with BIP 348. I'm not too familiar with how policy rules are expressed, so can't speak for that.

DrahtBot · 2025-06-23T11:21:02Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#73 (BIP349 OP_INTERNALKEY by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

instagibbs · 2025-06-23T12:38:49Z

Added a regtest-only deployment and functional testing.

I have another branch where I tested pre-activation relay logic and things checked out, but activating a heretical deployment was a little more annoying than in Core so I left it out: https://github.com/instagibbs/bitcoin/tree/2025-06-csfs-activation-tests

CI failure seems unrelated?

darosior · 2025-06-26T17:30:39Z

src/kernel/chainparams.cpp

You probably want different values from the OP_CAT deployment?

I'm going to need an explainer to figure out deployments in inquisition; can fix it along with potential signet one

@ajtowns could you provide guidance here?

See #82; though you'll need a binana number. Maybe figure out a nice way to make a binana doc just reference a particular revision of a bip? (Have BIN-2025-xxx.md just have a header and a table that maps binana revision numbers to bips repo commit ids maybe?)

bitcoin-inquisition/binana#1 turns out IK is already a thing in BINANA (and compatible), CSFS needs modification. Can the BINANA be modified with author's consent or do you suggest a new one? I believe the *VERIFY variant is considered abandoned.

Modifications should be with author's agreement, and bumping the revision number and date. If it's just changing to reference an updated spec from the same author in the bips repo, I'd be happy to assume the author's agreement. Just getting a new number assigned is fine too (though I guess I'd want to add the author to the table in the README to better distinguish them in that case?).

If it's just changing to reference an updated spec from the same author in the bips repo, I'd be happy to assume the author's agreement

I'd want the literal spec in BIP348 (obviously), seems easiest.

@reardencode Are you ok with this idea?

Yes, that seems reasonable. Definitely better than trying to keep the two docs in sync.

Maybe figure out a nice way to make a binana doc just reference a particular revision of a bip?

You might like to reference BIN16-118 for how to simplify BINANA assignments for BIPs.

theStack

ACK 5a7b2ac2aa3705f5e5fa244d81f37924dcf690ec

Verified that the consensus changes match BIP-348 and reviewed the tests, didn't look closely at the deployment parameters (only checked that it's regtest-only).

theStack · 2025-07-03T16:26:54Z

test/functional/test_framework/script.py

duplicate line, it's already added in the main commit

Suggested change

OP_CHECKSIGFROMSTACK = CScriptOp(0xcc)

theStack · 2025-07-03T16:30:19Z

src/script/interpreter.cpp

nit: could introduce a new error code SCRIPT_ERR_DISCOURAGE_OP_CHECKSIGFROMSTACK

Implements script validation, but no exposure to any network as of yet.

reardencode

Ack 69394e6

reardencode · 2025-07-15T04:12:31Z

test/functional/feature_taproot.py


    # == Test for sigops ratio limit ==

+    # BIP348 CSFS signatures are embedded directly into the tapleaves vs the witness stack


This comment was a bit confusing to me - perhaps someting like: "BIP348 CSFS messages and signatures can be embedded directly into the tapleaves to simplify testing since they do necessarily cover the spending transaction"

benthecarman · 2025-07-30T17:41:10Z

src/script/interpreter.cpp

+        execdata.m_validation_weight_left -= VALIDATION_WEIGHT_PER_SIGOP_PASSED;
+        if (execdata.m_validation_weight_left < 0) {
+            return set_error(serror, SCRIPT_ERR_TAPSCRIPT_VALIDATION_WEIGHT);
+        }


doesn't sound right to validate the signature before checking this

EvalChecksigTapscript does this check first before validating the sig and in the core PR it is done first as well

This PR follows the BIP more closely than random implementation details of taproot. I'm willing to change it but I'd need general agreement on it.

The BIP says "These opcodes are treated identically to other signature checking opcodes and count against the sigops and budget." which to me implies it should closely follow the random implementation details of the other signature checking opcodes...

(Checking the budget before incurring the expense doesn't seem like a random implementation detail, though...)

I think this is strictly equivalent? There is no execution path where we would perform the verification but not end up either accounting for it here or immediately stopping Script execution.

you'd do one extra signature validation, not the end of the world jut calling it out that its different from how checksig implemented

@benthecarman I assume he meant consensus semantically, not implementation-wise.

If it's a blocker for moving forward it can be changed...

Not a huge deal, just calling out the difference

I think this is strictly equivalent?

You get a different error message (failing signature vs exceeded the budget); and you spend more execution time reporting the failed signature. Still fails in either case, so not the end of the world, but seems strictly worse, and not particularly justified by any of the BIP text. Matching the EvalChecksigTapscript implementation seems more logical to me. 🤷‍♂️

"reverted" to taproot flow in #87

benthecarman · 2025-07-30T17:48:04Z

src/script/interpreter.cpp

+ *  Returns false when script execution must immediately fail.
+ *  `success_out` must be set when returning true.
+ */
+static bool EvalChecksigFromStack(const valtype& sig, const valtype& msg, const valtype& pubkey_in, ScriptExecutionData& execdata, unsigned int flags, SigVersion sigversion, ScriptError* serror, bool& success_out)


nit: this is between EvalChecksigPreTapscript and EvalChecksigTapscript which can be a little confusing/annoying

moved in #87

instagibbs · 2025-08-27T13:20:13Z

closing for now since 29.x is active

instagibbs commented Mar 13, 2025

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the 2025-03-bip348-inq-28 branch 3 times, most recently from 8b70161 to c401fa4 Compare March 20, 2025 13:29

instagibbs mentioned this pull request Mar 20, 2025

BIP349 OP_INTERNALKEY #73

Closed

dergoegge approved these changes Jun 11, 2025

View reviewed changes

darosior approved these changes Jun 20, 2025

View reviewed changes

stevenroose approved these changes Jun 23, 2025

View reviewed changes

instagibbs force-pushed the 2025-03-bip348-inq-28 branch from c401fa4 to 5a7b2ac Compare June 23, 2025 12:37

DrahtBot mentioned this pull request Jun 23, 2025

Prevent 64 byte txs at consensus level #24

Closed

darosior reviewed Jun 26, 2025

View reviewed changes

theStack reviewed Jul 3, 2025

View reviewed changes

ajtowns added this to the 28.x milestone Jul 4, 2025

instagibbs force-pushed the 2025-03-bip348-inq-28 branch from 5a7b2ac to 28b0830 Compare July 7, 2025 12:13

instagibbs added 3 commits July 8, 2025 14:53

XOnlyPubKey::VerifySchnorr: take span

7379372

consensus: implement BIP348 OP_CHECKSIGFROMSTACK

6e55d27

Implements script validation, but no exposure to any network as of yet.

test: basic BIP348 unit tests

a25cf58

instagibbs force-pushed the 2025-03-bip348-inq-28 branch from 28b0830 to 871b0b0 Compare July 8, 2025 18:54

test: Add bip348 coverage to feature_taproot

69394e6

instagibbs force-pushed the 2025-03-bip348-inq-28 branch from 871b0b0 to 69394e6 Compare July 8, 2025 20:10

reardencode reviewed Jul 15, 2025

View reviewed changes

benthecarman reviewed Jul 30, 2025

View reviewed changes

instagibbs mentioned this pull request Aug 19, 2025

Implement BIP 348 validation (CHECKSIGFROMSTACK) #87

Merged

instagibbs closed this Aug 27, 2025

ajtowns added the bip348 label Sep 8, 2025


		# == Test for sigops ratio limit ==

		# BIP348 CSFS signatures are embedded directly into the tapleaves vs the witness stack

Conversation

instagibbs commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jun 11, 2025

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Jun 22, 2025

Uh oh!

DrahtBot commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

instagibbs commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

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!

Choose a reason for hiding this comment

Uh oh!

reardencode left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benthecarman Jul 31, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 13, 2025 •

edited

Loading

DrahtBot commented Jun 23, 2025 •

edited

Loading

instagibbs commented Jun 23, 2025 •

edited

Loading

benthecarman Jul 31, 2025 •

edited

Loading