Skip to content

Include group public key in binding value computation#165

Closed
xoloki wants to merge 3 commits intomainfrom
bind-group-key
Closed

Include group public key in binding value computation#165
xoloki wants to merge 3 commits intomainfrom
bind-group-key

Conversation

@xoloki
Copy link
Copy Markdown
Owner

@xoloki xoloki commented May 7, 2025

Fixes #162

@xoloki xoloki self-assigned this May 7, 2025
@xoloki xoloki marked this pull request as ready for review May 7, 2025 17:16
@xoloki xoloki requested review from djordon and jferrant May 7, 2025 17:16
@xoloki xoloki added this to sBTC May 7, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in sBTC May 7, 2025
@xoloki xoloki added this to the Audit and Hardening milestone May 7, 2025
@djordon djordon moved this from Needs Triage to In Review in sBTC May 8, 2025
Copy link
Copy Markdown
Collaborator

@djordon djordon left a comment

Choose a reason for hiding this comment

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

I haven't really absorbed this one yet.

@djordon
Copy link
Copy Markdown
Collaborator

djordon commented May 13, 2025

Well, there isn't much to absorb. I'm just saying that I need one more pass

@xoloki xoloki force-pushed the bind-group-key branch 2 times, most recently from fa598a0 to d5edaf7 Compare May 15, 2025 15:17
let Some(group_key) = self.aggregate_public_key else {
return Err(Error::MissingAggregatePublicKey);
};
let (_, R) = compute::intermediate(&self.message, group_key, &party_ids, &nonces);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should probably use the same aggregate_nonce language for variables here as well.


let party_ids: Vec<u32> = sig_shares.iter().map(|ss| ss.id).collect();
let (Rs, R) = compute::intermediate(msg, &party_ids, nonces);
let (Rs, R) = compute::intermediate(msg, self.poly[0], &party_ids, nonces);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmmm, we shouldn't use direct array access here. Same thing in the changes above in this file.

I mean, it should always be fine, but we should add some encapsulation to the type so that we have that guarantee. No need to add encapsulation to this PR though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, why can't we use self.group_key here?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The aggregators don't have a self.group_key field. We could certainly add one, it's just duplicating a single 32-byte value.

Aggregator::init does return a Result, so we could always check there to make sure that the polynomial isn't empty. We could also check that each poly is appropriately size for the threshold, that the threshold isn't zero, etc. Do you want me to add an issue for this so we remember to handle it outside of this PR?

xoloki added 3 commits March 24, 2026 06:37
…pt spends (#199)

* add btc_sign_verify test that uses state machines to sign/verify btc taproot key and script spends

* dont take a reference of a slice

* clean up module imports and references
…lue computation

pass group key when necessary to compute binding value

use iter instead of into_iter when possible

set aggregate public key in nonce tests; use aggregate public key not tweaked public key when computing binding values

use RFC names for nonce commitments; add comments explaining what goes into the commitment lists; prefer let/else rather than if let/else

replace R with aggregate_nonce
@xoloki
Copy link
Copy Markdown
Owner Author

xoloki commented Mar 25, 2026

Fixed in #228, closing

@xoloki xoloki closed this Mar 25, 2026
@github-project-automation github-project-automation bot moved this from In Review to Done in sBTC Mar 25, 2026
@xoloki xoloki deleted the bind-group-key branch March 25, 2026 17:41
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.

[LOW] OS-STS-ADV-11 | Deviation from FROST Standards

2 participants