Custom payouts for mediation part 1: add script paths#134
Custom payouts for mediation part 1: add script paths#134stejbac wants to merge 7 commits intobisq-network:mainfrom
Conversation
The needless qualification of imports that are already in scope has been flagged by the IDE a number of times, but it turns out this is covered by a rustc rather than a Clippy lint, so add it to the workspace lints (and suppress as needed for the autogenerated proto bindings). Also fix the 'testenv' build, which needs Tokio's 'net' feature enabled, and tidy the manifests a little, by reordering dependencies slightly and removing Tokio from the dev-dependencies of 'protocol'.
Also make some minor fixes to the import lists in 'wallet' & 'testenv'. (This is to reflect the recently changed 'rustfmt.conf' settings.)
Use 'Self::reveal_next_address' in place of 'next_unused_address' to get fresh non-change addresses for the trade protocol, in the BDK wallet impl of the 'TradeWallet' trait, in order to prevent the same unused address being repeatedly returned. (This doesn't actually affect any production code ATM, since 'rpc::protocol' still uses a mock TradeWallet and 'protocol::protocol_musig_adapter' obtains new addresses by itself.) However, this likely-mistaken use of 'next_unused_address' separately affects 'protocol_musig_adaptor' and 'BMPWallet::next_address', so leave FIXMEs there, as it was likely unintended that the same address would be returned over and over until it's either seen in a tx or marked as used. Also add a unit test to 'protocol::psbt' to exercise the bug.
Move the Claim script & Warning escrow merkle root calculations to a new
module, 'protocol::script_paths', and similarly add a multisig script
for mediated custom payouts from the Deposit payout outputs, used by the
pub fns: 'script_paths::(deposit_payout|warning_escrow)_merkle_root'.
Also add a unit test to check the raw scripts against the miniscript:
"and_v(v:pk({buyer_pub_key}),pk({seller_pub_key}))",
"and_v(v:pk({pub_key}),older({lock_time}))"
for the multisig & Claim paths respectively. This shortens the latter by
one byte, from:
<lock_time> OP_CSV OP_DROP <pub_key> OP_CHECKSIG
to:
<pub_key> OP_CHECKSIGVERIFY <lock_time> OP_CSV
NOTE: The new multisig script is unused, with the Deposit Tx payouts
remaining keyspend-only for now. TODO: Actually add the script paths.
Provide a 'new_internal_key' trade wallet trait method to obtain HD pub keys for use in multisig scripts, similar to the existing 'new_address' method used to get fee-bump & payout addresses for the trade protocol. Also provide 'MockTradeWallet' & 'bdk_wallet::Wallet' impls for the new method, with the former just reusing funding-coin IKs for convenience (at least in 'psbt::(buyer|seller)_mock_trade_wallet()'), and the latter deriving the IK from the index of a fresh wallet address. Provide a unit test for the latter to verify the IKs come from the same keychain as the new addresses, and cause the wallet derivation index to increment.
Uncomment the provisional 'multisigScriptKey' & 'peersMultisigScriptKey' fields in 'rpc.proto', adding logic to exchange a buyer/seller multisig 'secp256k1::XOnlyPublicKey' along with the MuSig2 pubkey shares. Add a dedicated 'ExchangedKeys' struct for this purpose, in place of 2-element arrays and individual "(buyer|seller)_output_pub_key: Point" vars. (Use 'secp256k1' structs directly instead of their 'secp' wrappers, since the handling of wallet keys is really the responsibility of the Rust Bitcoin and BDK libs rather than 'musig2'/'secp'.) Similarly, rename the trade model "key_ctxs: KeyCtxs" field & struct to "keys: Keys" and add the exchanged multisig pubkeys to the latter, with each obtained from the peers' respective HD wallet keychains at the same time that the MuSig2 shares are initialised. Finally, give both Deposit Tx payout outputs _identical_ multisig spend paths with 'script_paths::deposit_payout_merkle_root', instead of being keyspend-only with empty merkle roots, using the exchanged script keys. (Also update 'TradeProtocolClient.java' to exchange the new fields.)
Now that the Deposit Tx payout outputs have a script path, we can no longer require that the Deposit inputs supplied by a peer are keyspend- only, as the payout UTXOs could be used to directly fund the next trade. So remove the optional 'internal_key' field from 'TxOutput', which is used to confirm that a prevout P2TR address has no script path, when calling 'TxOutput::estimated_input_weight'. Just assume it instead, and trust the peer not to attach an unexpectedly large witness to any of his Deposit Tx inputs, which can now have _any_ P2TR or P2WPKH address. This allows us to additionally redact the no-longer-needed internal keys from the peer's half-deposit PSBT input metadata, for better privacy, so add it to the redacted fields along with 'tap_(scripts|merkle_root)' for the inputs and 'tap_tree' for the outputs. (This requires us to set the 'trust_witness_utxo' flag in 'bdk_wallet::SignOptions' for deposit PSBT signing to succeed, which shouldn't weaken security in our case.)
| impl BMPWallet<Connection> { | ||
| pub fn next_address(&mut self, key_chain: KeychainKind) -> anyhow::Result<AddressInfo> { | ||
| // FIXME: `next_unused_address` just returns the same unused address over and over. It has | ||
| // to either be marked as used (which change isn't staged and therefore presumably never |
There was a problem hiding this comment.
Maybe the API could instead provide a new function which returns a fresh address no matter what, but this has to be under stop gap limit which means at some point the address could be the same generated.
Either way for the test we could provide a mark_as_used.
There was a problem hiding this comment.
I left a FIXME there mainly because I wasn't sure whether it was actually intended that next_address would keep returning the same address until it is marked as used (or actually used). Perhaps the method could be renamed (or a doc-comment added) to clarify, if that was in fact the intent, as "next_address" is a little ambiguous.
The BMPWallet impl of ProtocolWalletAPI currently calls Self::next_address to get new addresses for the trade protocol, so would suffer the same bug of returning duplicate addresses for the anchors & claim payouts that I fixed for protocol::psbt::TradeWallet. I think the other uses of BMPWallet::next_address in the code are fine, as the address appears to be immediately used upon return, in all the cases I could see.
| fn new_address(&mut self) -> Result<Address> { | ||
| Ok(self.next_unused_address(KeychainKind::External).address) | ||
| // For privacy, always get fresh addresses for the trade protocol. | ||
| // FIXME: Need to find a way to prevent gaps of unused addresses from growing too large. |
There was a problem hiding this comment.
Yes indeed I mentioned this below.
| Ok(self.reveal_next_address(KeychainKind::External).address) | ||
| } | ||
|
|
||
| fn new_internal_key(&mut self) -> Result<XOnlyPublicKey> { |
There was a problem hiding this comment.
Will be good if this is moved into the wallet module?
There was a problem hiding this comment.
That would require moving the entire TradeWallet trait into the wallet crate. I'm not sure whether that's the best place to put it for now or not -- I would have to give it some more thought, as it currently just has a bdk_wallet::Wallet (and a mock) impl. It's duplicating the functionality of the later-added wallet::bmp_wallet::ProtocolWalletAPI trait, so presumably at some point the two should be merged.
(I've been trying the keep the API of TradeWallet as bare-bones as possible, to facilitate mocking. I also was thinking of adding a separate trait for protocol-relevant chain notification, likely with an async interface and just mocked to begin with as well, but I haven't given that much thought yet.)
|
@ChrisSon15: I haven't added any tests for the multisig script spends yet, as that would be quite a bit more involved than just exchanging the multisig keys with the peer and adding the script paths to the Deposit outputs. I'm working on that now and am currently attempting to figure out exactly what metadata needs to be attached to the exchanged custom payout PSBT to get the two wallets to sign it with their respective keys, and then extract a signed tx. (I intend to add an integration test to |
3 participants
Add identical multisig script paths for mediated custom payouts to the two Deposit Tx payout addresses, uncommenting the provisional
multisigScriptKeyandpeersMultisigScriptKeyfields added torpc.protoin #131 for this purpose. The proto fields are already exchanged by the Bisq2 client (missing from the RPC responses and therefore defaulting to empty byte arrays), so no further client-side changes are needed to support this PR.Take the buyer/seller multisig keys from each peers' external wallet keychain, adding a
TradeWallet::new_internal_keytrait method and impls for this purpose (since I don't think it's standard to use tweaked keys within tapscripts).Also remove the now-counterproductive check that the peer's Deposit inputs are keyspend-only with no script paths, since that would prevent the payouts of previous trades from being directly used as such.
Finally, do some assorted minor cleanups, add
unused_qualificationsto the workspace rustc lints, optimise the Claim script and fix the standalone build oftestenv(which needs to specify Tokio'snetfeature in its manifest).--
Adding custom trade-closing payouts to the gRPC API and actually building, signing & broadcasting them may be better to do in a separate PR, as this PR is already getting rather big.