bisq-network / bisq

A decentralized bitcoin exchange network
https://bisq.network
GNU Affero General Public License v3.0
4.64k stars 1.26k forks source link

V5 trade protocol #7105

Open stejbac opened 1 month ago

stejbac commented 1 month ago

Preliminary changes on top of the the work done by @alvasw and @HenrikJannsen on the new v5 trade protocol upgrade for bisq1 (see https://github.com/bisq-network/proposals/issues/421), recently rebased to master (3b428df - May 9, 2024).

The changes attempt to avoid an increase in the number of message rounds at the start of the protocol, by having both traders compute all the finalised staged txs excluding the claim txs (that is, both warning txs and both redirect txs) at the earliest available opportunity. I believe this is upon receipt of the first response from the maker (InputsForDepositTxResponse_v5), if enough information is provided in the first two messages, namely the fee bump output addresses and the signatures of the staged txs once the deposit txId is known. Only exchange of the staged tx signatures is necessary, rather than the txs themselves, as a single signature commits to the entire tx (minus witnesses), provided the tx is non-malleable.

For the buyer-as-taker, seller-as-maker protocol, the old DepositTxMessage from v4 is redundant in the case of SegWit-only inputs. (I think it would make sense to completely disallow non-SegWit deposit tx inputs for v5, which should probably be done for v4 as well if they're still allowed, as it gives better safely against an invalid delayed payout tx due to deposit tx malleability.) Since that message is redundant, it should be possible to achieve one fewer message rounds in that case, so that there is one less message exchanged when the buyer is the taker, instead of there being one more message exchanged, at present.

TODO: So far, I have wired up the protobuf messages but not finished adding the finalised staged tx computations to the TradeTask lists.

TODO: Organise the bundled changes into a more logical sequence of commits.

TODO: Rest of the protocol, beyond the trade start.

djing-chan commented 1 month ago

Thanks for sharing and great to see that progress!

For the buyer-as-taker, seller-as-maker protocol, the old DepositTxMessage from v4 is redundant in the case of SegWit-only inputs. (I think it would make sense to completely disallow non-SegWit deposit tx inputs for v5, ...

Agree

...which should probably be done for v4 as well if they're still allowed, as it gives better safely against an invalid delayed payout tx due to deposit tx malleability.)

Agree

Since that message is redundant, it should be possible to achieve one fewer message rounds in that case, so that there is one less message exchanged when the buyer is the taker, instead of there being one more message exchanged, at present.

I guess you refer to v4 protocol, right? Do you think the added value of reducing one round justifies the risks by changing the protocol? Specially in the light that once v5 is complete v4 will get faded out anyway (we can enforce it after some period).

stejbac commented 1 month ago

I wasn't planning to make any changes to the v4 protocol, except possibly disallowing non-Segwit inputs if they still work. But I was aiming to reduce the number of rounds for v5, in the case of the buyer-as-taker. Hopefully, I won't run into problems when adding the missing TradeTasks.

github-actions[bot] commented 2 weeks ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

HenrikJannsen commented 2 weeks ago

Still active (-> avoid stale bot close)

stejbac commented 2 days ago

I've force pushed a rebase from the latest snapshot of master, and added some further temp changes. The trade task-lists at the opening of the trade are mostly done, but need testing. The redirect tx finalisation logic also needs fixing. Then, work on the rest of the trade lifecycle can be started.

Currently, it is set up so that both traders end up with each others' finalised warning & redirect txs, in addition to their own. I'm not sure whether this poses any security issue, but should be fairly easy to change if necessary. So one could publish the peer's redirect tx after publishing one's own warning tx, to send the trade straight to arbitration. Or, less usefully, publish the peer's warning tx.