feat: secret service #28
Conversation
There was a problem hiding this comment.
At the moment, it feels like this is still very much in the scaffolding stage rather than in the concrete implementation. The only concrete implementation I see is in the client-server parts of the secret service rather than what it's supposed to be doing.
I can also see that the types have been laid out such that the server also does the musig2 session book-keeping but the actual implementation for this book-keeping/signing/aggregating has not been written out yet. Another thing missing is where all this data is stored. If this service is going to handle the musig2 signing session as well, it will need this information to be persistent and recoverable.
One more thing I don't see is a wallet-like functionality, specifically an API to retrieve unspent UTXOs.
I would recommend working on the concrete parts of the implementation:
- musig2 signing/aggregating
- wots signing/aggregating
- p2p signing
- pre-image generation
- wallet integration
...before moving on to the binary/client implementation.
The reason I bring this up is that the way it currently stands, at least some of the interfaces and designs might change and bitcoin-core being what it is, I can easily see these taking a full sprint which is far from ideal.
Check the secret-service-server crate for the musig2 in-memory session management. The implementor (secret-service) is responsible for handling persistence.
Correct. This will be handled via sled in the implementor. Server is going to have some minor changes so session recovery happens on boot. This bit is relatively trivial compared to the rest of it. PS: whaddya mean by bitcoin core? None of the secret service touches or depends on core. |
Is the implementor also in scope for this PR?
Right but when you write tests for all the trait implementors, you will need to eventually broadcast a signed transaction. And debugging those can be a pain. PS: I also see that you've brought the entire |
There was a problem hiding this comment.
The direction is good, but it needs ton of polishing:
- Get rid of the
musig2code and do tuple wrapper structs in the same fashion as how we did formusig2stuff for Borsh instrata, see Just do the same to what we did on strata for the musig2 stuff for Borsh.
See https://github.com/alpenlabs/strata/blob/a0d413d711bb3af5ce312544f45600806f3f18bb/crates/primitives/src/bridge.rs#L225-L263 for examples. - Needs to be better organized, the crates are huge
lib.rsfiles and the binary should be moved to abin/subdir somewhere. - Needs also @Rajil1213's clippy lints secret sauce config for the
Cargo.tomls.
| [package] | ||
| name = "secret-service" | ||
| version = "0.1.0" | ||
| edition = "2021" |
There was a problem hiding this comment.
This should be moved to bin/ directory maybe.
Cc @Rajil1213.
There was a problem hiding this comment.
Yes, anything that has a main function should go into the bin/ directory.
Yes, I'm working on it at the moment. The other crates are mostly functionally complete but need tidying once I'm done |
| [package] | ||
| name = "secret-service" | ||
| version = "0.1.0" | ||
| edition = "2021" |
There was a problem hiding this comment.
Yes, anything that has a main function should go into the bin/ directory.
Zk2u
force-pushed
the
feat/mi6
branch
2 times, most recently
from
f0f64bb to
17a4cda
Compare
There was a problem hiding this comment.
Since this PR is in the testing/documentation phase right now, could you add the following section to the Cargo.toml file in each of the crates?
[lints]
rust.missing_debug_implementations = "warn"
rust.rust_2018_idioms = { level = "deny", priority = -1 }
rust.unreachable_pub = "warn"
rust.unused_crate_dependencies = "deny"
rust.unused_must_use = "deny"
rust.unsafe_op_in_unsafe_fn = "warn"
rust.missing_docs = "warn"
rustdoc.all = "warn"These are mostly for stricter automatic checks for documentation. There is also a lint for unsafe code since I saw some unsafe blocks in this PR (which will be warn by default in the next rust edition anyway).
* doc: secret-service-client * doc: secret-service-proto wire * doc: secret-service-proto traits * doc: secret-service-proto rhyv_wrappers * doc: secret-service-server * doc: secret-service * doc: S2 is too ambiguous... * Apply suggestions from code review Co-authored-by: azz <[email protected]> * doc: rebase * doc: add spaces --------- Co-authored-by: azz <[email protected]>
|
Looks like the Note that we'll need to update the |
|
@Rajil1213 is this just waiting for an approval then? |
The docs are still failing. So that needs fixing. Will review it after the CI is green. But it should be good to go and we should be able to punt any changes down the line. |
|
@Rajil1213 @storopoli ci is passing now |
There was a problem hiding this comment.
few nits.
Ideally we would also mock a MuSig2 first round and second round,
check the tests vectors in BIP-327
If you have time for that that would be great!
crates/secret-service/src/tests.rs
Outdated
| // operator signer | ||
| let op_signer = client.operator_signer(); | ||
| let pubkey = op_signer.pubkey().await.expect("good response"); | ||
|
|
||
| let handles = (0..1000) | ||
| .map(|_| { | ||
| let secp_ctx = Secp256k1::verification_only(); | ||
| let op_signer = op_signer.clone(); | ||
| tokio::spawn(async move { | ||
| let to_sign = thread_rng().gen(); | ||
| let sig = op_signer.sign(&to_sign).await.expect("good response"); | ||
| assert!(secp_ctx | ||
| .verify_schnorr(&sig, &Message::from_digest(to_sign), &pubkey) | ||
| .is_ok()); | ||
| }) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| for handle in handles { | ||
| handle.await.unwrap(); | ||
| } | ||
|
|
||
| // p2p signer | ||
| let p2p_signer = client.p2p_signer(); | ||
| p2p_signer.secret_key().await.expect("good response"); | ||
| } |
There was a problem hiding this comment.
Can we also test a stake preimage, a wots 160 and 256 secret keys, and a secret nonce?
|
Yes ok will add these this afternoon. |
There was a problem hiding this comment.
The musig2 stuff might need a few changes. Other than that, the secret service crate should also be broken down. It contains a library part that handles the actual logic and a binary part main.rs that runs the binary. This should be split out so that the library part of it is what remains inside the crates directory while the main.rs should be moved to separate binary in the bin directory. This library part should also be fully tested (given its nature, I think this is doable). I also think the disk module is a bit of a misnomer at this point.
| if !pubkeys.contains(&my_pub_key) { | ||
| pubkeys.push(my_pub_key); | ||
| } | ||
| pubkeys.sort(); |
There was a problem hiding this comment.
The pubkeys are not sorted like this in our impl. The order of the keys is determined by the the operator indexes. We could switch over to this sorting logic but that requires changes in multiple places. For now, you can just assume that the order in which the keys are received is their canonical ordering so far as key aggregation is concerned. You can also assume that this signer's public key will be present in the list of public keys provided so it does not need to be mutable. You could add a sanity check though -- if this signer's key is not part of the list of public keys, it's a catastrophic issue because it would mean that the operator's pubkey is not present in the strata chain state.
|
Having some weird issues with the musig2 setup so waiting for some help from Aaron and wiill fix (will also cover the pubkey stuff from @Rajil1213). Also @Rajil1213 I agree the disk module naming could be clearer. Will rename that. But I think we should keep the implementation inside that crate for the moment. We can totally add unit tests inside the individual modules and keep it a binary. |
That's probably fine because there's not much going on there at the moment. However, at some point, that is where most of the non-trivial logic will live (musig2 session-management, etc.). Plus it's also doing a bunch of key-derivation. I feel like it's better to have that kind of thing in a separate crate (similar to how it is in |
Description
This PR implements a secret service system consisting of 3 library crates, server, proto and client, and 1 binary that acts as the server implementor. The client can be used by the other bridge binaries.
The "server" is responsible for keeping secrets used in strata bridge. The "clients" are the sequencer/operator binaries for their respective duties. They communicate using a QUIC connection with a binary-based rkyv wire protocol. Security is handled through mutually authenticated TLS 1.3 as part of QUIC.
Type of Change
Notes to Reviewers
Checklist
Related Issues
STR-934