Update MuSig RPC client to pick up missing txids & exchange multisigScriptKey fields#4497
Merged
HenrikJannsen merged 5 commits intobisq-network:mainfrom Mar 10, 2026
Conversation
Add a 'ContractualTxIds' proto, holding the five Deposit, Warning and Redirect txids of the MuSig trade, together with a 'contractualTxIds' field to 'PartialSignaturesMessage', to bring 'rpc.proto' up to date with the latest bisq-musig version. The field will always be populated in the first 'GetPartialSignatures' response from the service (though it will be missing in later responses when the 'buyerReadyToRelease' flag is set). But there is no need for the client to populate it in the subsequent 'SignDepositTx' call during trade setup, as the server always ignore it. So make the field Optional in the corresponding 'PartialSignaturesMessage' Java DTO.
Add missing 'NonceSharesRequest.tradeFeeReceiver' field that was added to 'rpc.proto' a while ago, and make it Optional in the DTO even though it isn't marked 'optional' in the proto def (which is redundant for message fields), since it may be left unset by the client. Similarly, make 'PartialSignaturesRequest.peerNonceShares' Optional even though it isn't marked as such in the proto def, as the client need not set it in the second call to 'GetPartialSignatures' when the buyer starts payment. (The DTOs appear to be currently unused anyway, as the request protos are populated directly when making gRPC calls to the Musig service.)
Update 'rpc.proto' & 'trade.proto' to exchange an additional multisig pubkey for use in custom script-path payouts (along with the existing buyer/seller payout MuSig2 pubkey shares), in the 'PubKeyShares' value object and 'PubKeySharesResponse' + 'NonceSharesRequest' gRPC protos. (The extra proto fields are in anticipation of pending Rust changes to support custom payouts for mediation. But since the fields default to empty byte arrays if missing, it is easier to make the proto changes on the client side first, as it just forwards them to the peer.)
WalkthroughAdds multisig script key propagation, a new ContractualTxIds domain class, an optional contractualTxIds field, an optional trade-fee receiver, and makes peersNonceShares optional; updates protobuf definitions, builders, fromProto/toProto mappings, equality/hashCode, and related handlers to carry new fields. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
trade/src/main/java/bisq/trade/mu_sig/messages/grpc/ContractualTxIds.java (1)
24-43: Consider making the classfinal.The class is immutable with all final fields, and the design doesn't suggest inheritance is intended. Making it
finalwould clarify the design intent and prevent unintended subclassing.Proposed fix
`@Getter` `@EqualsAndHashCode` -public class ContractualTxIds implements Proto { +public final class ContractualTxIds implements Proto {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trade/src/main/java/bisq/trade/mu_sig/messages/grpc/ContractualTxIds.java` around lines 24 - 43, Make the immutable class ContractualTxIds explicitly final to prevent subclassing and communicate intent: update the class declaration "public class ContractualTxIds implements Proto" to be final (e.g., "public final class ContractualTxIds implements Proto"), leaving all existing final fields (depositTxId, buyersWarningTxId, sellersWarningTxId, buyersRedirectTxId, sellersRedirectTxId) and the constructor unchanged.trade/src/main/proto/rpc.proto (2)
72-77: Field ordering note.The
multisigScriptKeyfield is assigned number 4 whilecurrentBlockHeightuses number 3. This is fine for wire compatibility but creates a non-sequential visual ordering. Consider documenting this in a comment if there's a specific reason for the ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trade/src/main/proto/rpc.proto` around lines 72 - 77, In PubKeySharesResponse the field numbers are visually out of sequence (multisigScriptKey = 4 placed before currentBlockHeight = 3); either reorder the two field declarations so their numeric tags appear in ascending order or add a short comment above the message (or above multisigScriptKey) explaining the intentional non-sequential numbering; reference the message PubKeySharesResponse and the fields multisigScriptKey and currentBlockHeight when making the change so readers understand why the tag order differs.
123-123: Consider markingcontractualTxIdsasoptionalin the proto definition.The Java DTO (
PartialSignaturesMessage) treats this field asOptional<ContractualTxIds>, indicating it may be absent. As per coding guidelines, theoptionalkeyword should be used in Protobuf for optional Java fields. This enableshasContractualTxIds()on the proto object for proper presence checking.Proposed fix
- ContractualTxIds contractualTxIds = 7; + optional ContractualTxIds contractualTxIds = 7;As per coding guidelines: "Use
optionalkeyword in Protobuf for optional Java fields".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trade/src/main/proto/rpc.proto` at line 123, The proto field ContractualTxIds contractualTxIds should be declared optional to match the Java DTO usage (PartialSignaturesMessage) and enable presence checks; update the proto line to mark the field as optional (i.e., optional ContractualTxIds contractualTxIds = 7), regenerate the Java protobuf classes so callers can use hasContractualTxIds() and adjust any code assuming non-null to use the presence API on the generated message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@trade/src/main/java/bisq/trade/mu_sig/messages/grpc/PubKeySharesResponse.java`:
- Around line 30-41: The mock server is not populating the new multisigScriptKey
field in PubKeySharesResponse; update MockMusigServer's initTrade response
builder to call setMultisigScriptKey(...) so tests exercise the field. In
MockMusigServer.initTrade add .setMultisigScriptKey(copyFrom(randomBytes(...)))
to the PubKeySharesResponse builder (matching how
buyerOutputPubKeyShare/sellerOutputPubKeyShare are set) so the multisigScriptKey
byte field is populated in the generated response.
---
Nitpick comments:
In `@trade/src/main/java/bisq/trade/mu_sig/messages/grpc/ContractualTxIds.java`:
- Around line 24-43: Make the immutable class ContractualTxIds explicitly final
to prevent subclassing and communicate intent: update the class declaration
"public class ContractualTxIds implements Proto" to be final (e.g., "public
final class ContractualTxIds implements Proto"), leaving all existing final
fields (depositTxId, buyersWarningTxId, sellersWarningTxId, buyersRedirectTxId,
sellersRedirectTxId) and the constructor unchanged.
In `@trade/src/main/proto/rpc.proto`:
- Around line 72-77: In PubKeySharesResponse the field numbers are visually out
of sequence (multisigScriptKey = 4 placed before currentBlockHeight = 3); either
reorder the two field declarations so their numeric tags appear in ascending
order or add a short comment above the message (or above multisigScriptKey)
explaining the intentional non-sequential numbering; reference the message
PubKeySharesResponse and the fields multisigScriptKey and currentBlockHeight
when making the change so readers understand why the tag order differs.
- Line 123: The proto field ContractualTxIds contractualTxIds should be declared
optional to match the Java DTO usage (PartialSignaturesMessage) and enable
presence checks; update the proto line to mark the field as optional (i.e.,
optional ContractualTxIds contractualTxIds = 7), regenerate the Java protobuf
classes so callers can use hasContractualTxIds() and adjust any code assuming
non-null to use the presence API on the generated message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd3b9c42-659a-4f2a-8238-2500088964ec
📒 Files selected for processing (10)
trade/src/main/java/bisq/trade/mu_sig/messages/grpc/ContractualTxIds.javatrade/src/main/java/bisq/trade/mu_sig/messages/grpc/NonceSharesRequest.javatrade/src/main/java/bisq/trade/mu_sig/messages/grpc/PartialSignaturesMessage.javatrade/src/main/java/bisq/trade/mu_sig/messages/grpc/PartialSignaturesRequest.javatrade/src/main/java/bisq/trade/mu_sig/messages/grpc/PubKeySharesResponse.javatrade/src/main/java/bisq/trade/mu_sig/messages/network/handler/maker/SetupTradeMessage_A_Handler.javatrade/src/main/java/bisq/trade/mu_sig/messages/network/handler/taker/BaseSetupTradeMessage_B_Handler.javatrade/src/main/java/bisq/trade/mu_sig/messages/network/mu_sig_data/PubKeyShares.javatrade/src/main/proto/rpc.prototrade/src/main/proto/trade.proto
HenrikJannsen
approved these changes
Mar 9, 2026
stejbac
added a commit
to stejbac/bisq-musig
that referenced
this pull request
Mar 9, 2026
Try to keep the field numbers in `rpc.proto` sequential (even though that breaks wire compatibility), as the corresponding change is being made in PR bisq-network/bisq2#4497 on the Bisq2 client side. Also add explicit 'optional' modifiers to message fields where they can be missing during normal usage of the 'Musig' service. (The modifier is technically redundant in those cases, and is just for clarity, as the fields have explicit presence and are already implicitly optional.)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@trade/src/main/proto/trade.proto`:
- Around line 180-194: The proto field renumbering in message NonceShares breaks
wire compatibility for persisted peersNonceShares (used in
MuSigTradeParty.java); either revert NonceShares field numbers to their original
assignments to preserve backward-compatible deserialization, or if the change is
intentional for dev snapshots, implement explicit migration: add a dev-only
startup routine in MuSigTradeParty (or persistence layer) that clears or
migrates existing persisted peersNonceShares before loading, and record this
required data wipe in the release notes/documentation so dev environments clear
persisted trade data before upgrading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 07158e2a-f890-4c61-bc01-c8052a2d435b
📒 Files selected for processing (3)
trade/src/main/java/bisq/trade/mu_sig/messages/grpc/ContractualTxIds.javatrade/src/main/proto/rpc.prototrade/src/main/proto/trade.proto
Provide a 'ContractualTxIds::toString' implementation, and mark message fields in 'rpc.proto' explicitly optional where they may not always be populated during normal API usage, to match the Java DTOs, instead of relying on implicit optionality.
Reorder the field numbers in 'rpc.proto' to try to keep them sequential while still in development, even though that breaks wire compatibility. (The corresponding changes are being made on Rust side in PR bisq-network/bisq-musig#131.)
stejbac
force-pushed
the
sync-rpc-client-to-pick-up-missing-txids
branch
from
bf350ea to
6503d22
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
trade/src/main/proto/rpc.proto (1)
95-106:⚠️ Potential issue | 🟠 MajorDon't renumber established protobuf tags in these response messages.
Lines 95-106 and 120-122 move existing
NonceSharesMessage/PartialSignaturesMessagefields onto new tag numbers. That is a wire break: mixed-version Bisq2 andbisq-musigpeers will deserialize nonce/signature bytes into the wrong fields instead of safely ignoring unknown additions. Please keep existing tags stable and assign fresh numbers only to newly added fields, or land this behind a strictly coordinated lockstep rollout with the Rust side.Also applies to: 120-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trade/src/main/proto/rpc.proto` around lines 95 - 106, The protobuf diff renumbers existing fields in NonceSharesMessage and PartialSignaturesMessage (e.g., fields like swapTxInputNonceShare, buyersWarningTxBuyerInputNonceShare, buyersClaimTxInputNonceShare, sellersClaimTxInputNonceShare, etc.), which is a wire-breaking change; revert these fields to their original tag numbers and when adding new fields assign fresh, unused tag numbers instead of reusing or shifting existing tags so mixed-version peers deserialize correctly; ensure all renamed/moved entries in both NonceSharesMessage and PartialSignaturesMessage keep their original tag IDs and only newly introduced fields get new unique tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@trade/src/main/proto/rpc.proto`:
- Around line 95-106: The protobuf diff renumbers existing fields in
NonceSharesMessage and PartialSignaturesMessage (e.g., fields like
swapTxInputNonceShare, buyersWarningTxBuyerInputNonceShare,
buyersClaimTxInputNonceShare, sellersClaimTxInputNonceShare, etc.), which is a
wire-breaking change; revert these fields to their original tag numbers and when
adding new fields assign fresh, unused tag numbers instead of reusing or
shifting existing tags so mixed-version peers deserialize correctly; ensure all
renamed/moved entries in both NonceSharesMessage and PartialSignaturesMessage
keep their original tag IDs and only newly introduced fields get new unique
tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 008f3230-1fd6-454c-82f8-6732f6f0e14d
📒 Files selected for processing (2)
trade/src/main/java/bisq/trade/mu_sig/messages/grpc/ContractualTxIds.javatrade/src/main/proto/rpc.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- trade/src/main/java/bisq/trade/mu_sig/messages/grpc/ContractualTxIds.java
Contributor
|
Maybe better we leave it open until the backend is merged, thus devs dont need to switch backend to the working branch... @stejbac Can you ping me once merged? |
Contributor
|
@stejbac told me its already merged. |
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.
Add a
ContractualTxIdsproto, holding the five Deposit, Warning and Redirect txids of the MuSig trade, together with acontractualTxIdsfield toPartialSignaturesMessage, to bringrpc.protoup to date with the latest bisq-musig version. This provides the Bisq2 client with the missing Deposit txid needed from the start of the trade.(I'm not sure if
ContractualTxIdsis quite the best name -- the idea was that the Deposit, Warning & Redirect txids could be considered an implicit/supplemental part of the trade contract. A trader would ideally require a signed message from the peer holding the five txids, and cross-check it before signing his Deposit Tx inputs, as the txids are relevant to arbitration/mediation cases. In particular, this would prevent any dispute over whose Warning Tx was actually used, which might otherwise be hard to prove.)NOTE: These changes are dependent on the followup PR bisq-network/bisq-musig#131 in order to work properly, which updates the Rust service to supply the txids as hex strings instead of raw bytes (for convenience to the Java client).
--
Additionally, update
rpc.proto&trade.prototo exchange provisional multisig pubkeys at the trade start, for use in custom script-path payouts for mediation, by adding an extra field to the exchangedPubKeySharesvalue object (and thePubKeySharesResponse&NonceSharesRequestgRPC protos).The corresponding Rust changes are still work-in-progress and not yet merged, but I'm fairly certain the extra multisig keys need to exchanged in the first round with the MuSig2 keys (and not later with the fee bump or claim payout addresses, say), and I don't expect any further alterations to the protos will be needed for that.
(Modifying the client first is a little easier, as it's just forwarding the data, and any missing proto fields default to empty.)
Summary by CodeRabbit
New Features
Chores