Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V5 trade protocol #7105

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from
Draft

Conversation

stejbac
Copy link
Contributor

@stejbac stejbac commented May 17, 2024

Preliminary changes on top of the the work done by @alvasw and @HenrikJannsen on the new v5 trade protocol upgrade for bisq1 (see bisq-network/proposals#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.

alvasw and others added 30 commits May 12, 2024 12:55
The factory can create, sign, and finalize the warning transaction.
The factory can create, sign, and finalize the redirection transaction.
The factory creates, signs, and finalizes the claim transaction.
…ocol to BaseSellerProtocol

Signed-off-by: HenrikJannsen <[email protected]>
…ivate and constructor protected

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
… custom code but only call super to make it more transparent which handler methods are in the protocol.

Signed-off-by: HenrikJannsen <[email protected]>
…DE_PROTOCOL_VERSION field.

Signed-off-by: HenrikJannsen <[email protected]>
…s base for later implementing the new protocol.

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Pass depositTxOutput to make it more clear what is really used.
Use long for claimDelay (to be consistent with lockTime and ScriptBuilder expected type).
Do not pass aesKey in TradeWalletService.signWarningTx as its kept private in that service class.

Signed-off-by: HenrikJannsen <[email protected]>
Using a dedicated ProcessModel implementation would cause too much changes in client code. Maybe we still refactor that later, but for now it seems its less painful to add fields and use the same class at the old and new protocol.

Signed-off-by: HenrikJannsen <[email protected]>
…to BuyerProtocol_v5, but for now we start with one variant (BuyerAsMakerProtocol_v5 and SellerAsTakerProtocol_v5)

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
HenrikJannsen and others added 3 commits May 12, 2024 12:55
TODO: check if that is expected when we do not have the full tx chain or if its caused by a bug.

Signed-off-by: HenrikJannsen <[email protected]>
@djing-chan
Copy link
Contributor

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
Copy link
Contributor Author

stejbac commented May 19, 2024

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.

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.

None yet

4 participants