Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LNHANCE mutinynet (CHECKSIGFROMSTACK and INTERNALKEY) #2

Closed

Conversation

reardencode
Copy link

This adds CHECKSIGFROMSTACK and INTERNALKEY to mutinynet for review and testing.

(Related to bitcoin#29198)

For completeness, also updates FormatScriptFlags to report on any bits
that do not match a script flag.
@reardencode reardencode changed the title LNHANCE mutinynet LNHANCE mutinynet (CHECKSIGFROMSTACK and INTERNALKEY) Jan 14, 2024
always active on regtest
active from 2024-01-12 until 2034-01-12 on signet
never active on testnet, mainnet
Testing is minimal, but so is the code.
Some code and ideas from Elements by stevenroose, and sanket1729
Porting help from moonsettler

Tests added to the transaction tests framework.
Copy link
Owner

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these comments are probably better for the real PR, will copy-paste over there too

Comment on lines +370 to +376
} else if (pubkey_in.size() == 32) {
if (!success) return true;
if (sig.size() != 64) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_SIZE);

XOnlyPubKey pubkey{pubkey_in};

if (!pubkey.VerifySchnorr(msg, sig)) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean you can do schnorr sigs with segwit v0 and p2sh? is that intended?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is intended. Since we're introducing new sigops, we have the option to add key type detection similar to Tapscript keys (as leveraged in APO).

Comment on lines +378 to +379
// Pubkeys of length 33 are only constrained in legacy and segwitv0. In these script types only those beginning
// with 0x02 or 0x03 are constrained. Others are unknown pubkey types.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if schnorr is allowed in non-taproot then why forbid ecdsa for taproot?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion there - would love to discuss on the BIP whether we want both on both!

bitcoin/bips#1535 <-- here's the bips PR for discussing design things.

return false;
}

if (msg.size() != 32) return set_error(serror, SCRIPT_ERR_INVALID_DATA_LENGTH);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should different message sizes be left for future upgrades

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ECDSA only size 32 is possible - any size message can be done by adding a OP_SHA256 first.

@TonyGiorgio
Copy link

I think it's far too early an idea to be putting into our mutinynet. maybe once the code is solid and there's decent desire for it.

@reardencode
Copy link
Author

Hi Tony, what's not solid about the code?

CSFS has been in Liquid and BCH for years, and INTERNALKEY is simply an alternative to APO's magic 0x01 key to access the taproot internal key in script.

This combination of features can be used to build a better LN-Symmetry than APO. We're in the process of prototyping that, and would like to test it on a public signet such as mutinynet.

@TonyGiorgio
Copy link

The PR into core is just a week old and there's still conflicting discussions around it. Your INTERNALKEY bip is also just a week old. I don't think everyone's new op codes should just go into mutinynet the second they create one. That's the whole point of everyone being able to spin up their own signet.

@reardencode
Copy link
Author

FWIW, COVTOOLS was merged to MutinyNet 15 days after the PR to core was opened :)

@reardencode reardencode marked this pull request as draft January 16, 2024 06:32
@reardencode
Copy link
Author

Converted this to a draft while I work through AJ's review on my similar PR to inquisition. Will port those changes over here and undraft when the time comes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants