update protocol#906
Open
JssDWt wants to merge 12 commits into
Open
Conversation
dangeross
reviewed
May 26, 2026
dangeross
reviewed
May 28, 2026
Comment on lines
+884
to
+888
| if chunks.len() != 3 { | ||
| return Err(ServiceError::Generic( | ||
| "Not enough signing commitments returned".to_string(), | ||
| )); | ||
| } |
Collaborator
There was a problem hiding this comment.
This is only checking the commitments are chunked into 3, which could lead to a panic accessing the chunks later. Should we check signing_commitments.len() == 3 * leaves.len() instead?
Collaborator
Author
There was a problem hiding this comment.
Good catch on that panic. I added guards around that.
| ) | ||
| .await?; | ||
|
|
||
| // TODO: Verify the returned tx signatures |
Collaborator
There was a problem hiding this comment.
I'm wondering if we should definitely do this now we've dropped verifying the signature shares verifying key. Wdyt (could be a follow up PR)?
Collaborator
Author
There was a problem hiding this comment.
Sounds good. I added it for the deposit. I will do others in a follow-up PR, after adding Turnkey integration with the higher-level signer.
JssDWt
force-pushed
the
jssdwt-upate-protocol
branch
from
317e463 to
68a74b6
Compare
Replaces ad-hoc `signing_commitments.chunks(leaves.len())` patterns in prepare_transfer_package, prepare_claim_package and submit_coop_exit_transfer with a shared `split_signing_commitments_by_variant` helper that rejects zero node_id_count up front (Vec::chunks(0) panics) and validates total length equals `3 * node_id_count`. Adds empty-input guards and a checked u32 conversion for leaves.len() at the claim site.
JssDWt
added a commit
that referenced
this pull request
May 29, 2026
The previous fused start_deposit_tree_creation flow returned signature
shares that we aggregated locally; the new package flow aggregates
server-side, so we relied on the operators implicitly. Restore the
end-to-end check by validating, on the response:
* the returned verifying public key matches the one we used, and
* the aggregated Schnorr key-path signatures on node_tx, refund_tx
and direct_from_cpfp_refund_tx verify against the sighashes we
computed earlier in the flow.
Addresses PR review #906#discussion_r3317972290.
JssDWt
force-pushed
the
jssdwt-upate-protocol
branch
from
9642240 to
0ba6869
Compare
The previous fused start_deposit_tree_creation flow returned signature
shares that we aggregated locally; the new package flow aggregates
server-side, so we relied on the operators implicitly. Restore the
end-to-end check by validating, on the response:
* the returned verifying public key matches the one we used, and
* the aggregated Schnorr key-path signatures on node_tx, refund_tx
and direct_from_cpfp_refund_tx verify against the sighashes we
computed earlier in the flow.
The verification helper lives next to sighash_from_tx in
bitcoin/service.rs so it can be reused by other flows.
Addresses PR review #906#discussion_r3317972290.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change updates how the Breez SDK talks to the Spark network for three core
operations: depositing Bitcoin into Spark, receiving a transfer from another user,
and cooperatively withdrawing back to Bitcoin. Each previously took two network
round-trips with the Spark operators; we collapse them into one, matching what the
reference Spark JS SDK already uses.