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

Use secp256k1 musig2 #405

Merged
merged 42 commits into from
Jan 17, 2025
Merged

Use secp256k1 musig2 #405

merged 42 commits into from
Jan 17, 2025

Conversation

ceyhunsen
Copy link
Member

@ceyhunsen ceyhunsen commented Jan 10, 2025

Description

Use the active secp256k1 branch for musig2 while replacing the musig2 crate.

Key changes:

  • Converted every secp256k1::* to bitcoin::secp256k1::* because secp256k1 library upgraded to a breaking version and we are sticking with the older bitcoin version
  • utils::SECP needs to return because bitcoin::secp256k1 doesn't feature global secp context
  • Secret nonces are not saved to database because of the security

Linked Issues

ceyhunsen and others added 9 commits January 16, 2025 14:54
* Add watchtower xonly_pk to get_params and save to db

* fix clippy error

* add db helper to get single xonly_pk, remove unwrap

* Adjust sighash.rs and transaction.rs to remove nofn sigs from challenge page tx

* Incorporate Ceyhun's feedback
* Update risc0-to-bitvm2, change anchor output

* Renaming fix from circuits to header-chain
* Use global context for secp256k1

* Lint

* Delete comment
* Implement kickoff_timeoout_tx

* Clippy

* Add kickoff_timeout_tx to sighash_stream

* Change num_required_sigs
@ceyhunsen ceyhunsen marked this pull request as ready for review January 16, 2025 12:54
@ekrembal
Copy link
Member

Are there any performance improvements?

@ekrembal ekrembal changed the base branch from dev to atacan/new_design January 16, 2025 13:33
@ekrembal ekrembal changed the base branch from atacan/new_design to dev January 16, 2025 13:33
@ceyhunsen
Copy link
Member Author

ceyhunsen commented Jan 16, 2025

Are there any performance improvements?

Looks like there isn't any noticable difference for rpc::aggregator::tests::aggregator_setup_and_deposit test. Because most of time is not spent on aggregation, signing etc.; It is spent on transferring this datas. It can be clearly seen on a regular OS system monitor (CPU usage).

Copy link
Contributor

@ozankaymak ozankaymak left a comment

Choose a reason for hiding this comment

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

Please consider my comments before merging.

Cargo.toml Show resolved Hide resolved
secp256k1::Message::from_digest(*msg.as_ref())
}

/// Possible Musig2 modes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs more documentation. A comprehensive documentation issue should be fine.

// Verify the signature fails with the untweaked aggregate public key
assert!(SECP
.verify_schnorr(&final_sig, &message, &agg_pk_no_tweak)
.is_err());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As I have said before, we need a test for both KeySpend and ScriptSpend for musig2 belonging to the same Taproot address. Please open a new issue if this PR will be merged directly.

@mmtftr
Copy link

mmtftr commented Jan 16, 2025

I agree with @ozankaymak on the dependency documentation issue. We should document and track how to update when upstream is released.

I've looked into rust-bitcoin, and they use 0.29.1 of secp. I upgraded it to 0.30 and opened this PR. The PR should also allow us to remove the compatibility layer when merged.

For the time being, I believe we can do something like this including the comments for future reference (haven't checked syntax/validity of toml):

[dependencies]
bitcoin = "0.33.0-alpha.0" # TODO: this is a major version upgrade, look into whether anything breaks on our side
secp256k1 = "0.30.0"

[patch.crates-io]
bitcoin = { git = "https://github.com/mmtftr/rust-bitcoin.git", branch = "update-secp256k1-0.30.0" } # change to rust-bitcoin/rust-bitcoin when https://github.com/rust-bitcoin/rust-bitcoin/pull/3913 is merged
# remove when a new release is made
secp256k1 = { git = "https://github.com/jlest01/rust-secp256k1", branch = "musig2-module" } # change to rust-bitcoin/rust-secp256k1 when https://github.com/rust-bitcoin/rust-secp256k1/pull/716 is merged
# remove when a new release is made

For more info on patching, check the docs.

@ceyhunsen
Copy link
Member Author

Hold merge a bit; I am addressing the issues.

@ekrembal ekrembal merged commit a562f4d into dev Jan 17, 2025
9 checks passed
@ekrembal ekrembal deleted the ceyhun/secp256k1_musig2 branch January 17, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants