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

docs: rfc-0241 tari-monero atomic swap #3623

Merged
merged 5 commits into from
Jan 14, 2022

Conversation

SWvheerden
Copy link
Collaborator

Description

This document adds in RFC-0241 which describe Tari Monero atomic swaps

Motivation and Context

This is required if we want to be able to do Atomic swaps between Tari and Monero.

@philipr-za philipr-za changed the title docs: Tari Monero atomic swap docs: tari-monero atomic swap Nov 26, 2021
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did a brief read through so didn't dig into the math.

Main comment is after the merge of the two separate PRs there is a lot of repetition between the sections that can be improved. I would also say you should make it clear how the second approach is different and discuss the benefit of it.

RFC/src/RFC-0202_TariScriptOpcodes.md Outdated Show resolved Hide resolved
RFC/src/RFC-0202_TariScriptOpcodes.md Outdated Show resolved Hide resolved
RFC/src/RFC-0240_AtomicSwap.md Outdated Show resolved Hide resolved
RFC/src/RFC-0240_AtomicSwap.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SWvheerden,

Really nice so far.

As discussed:

  • We are not using MuSig sugnatures in the RFC, but aggregated Schnorr signatures.
  • The math for proving x_m_a = x_a for X_m_a != X_a and x_m_b = x_b for X_m_b != X_b is not correct.
  • Please add the Monero transaction to the transaction flow diagram and number them to enhance referencing from the text.
  • Please add proving knowledge of x_m_a and x_m_b to the alternative script, i.e. we need an ed25519 scripting option similar to the Ristretto one.
  • Please change the lapse transaction in both scripts to be claimable by either party at any time after height_2 - add another if then else branch for height_2.
  • Please arrange the alternative concept to be the primary proposed concept as it is a lot simpler and based on TariScript security.

Other remarks:

  • Some of math the rendering are failing.
  • Please verify your rendered text with Grammarly; this is easily done by with the online Grammarly by analyzing the rendered text.

Thanks.

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good @SWvheerden.

Just a few remaining comments I think need to be addressed.

RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Show resolved Hide resolved
@SWvheerden SWvheerden force-pushed the sw_xtr-xmr_rfc branch 3 times, most recently from f984b79 to fde52df Compare December 6, 2021 14:57
hansieodendaal
hansieodendaal previously approved these changes Dec 6, 2021
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - well done

Monero transaction and that everything is complete as they have agreed. If she is happy, she will provide Bob with the
pre-image to immediately claim the Tari UTXO.

### Key security
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section can probably be removed entirely

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is very informative to show this for a mixed audience, even though it might seem trivial. Many people wonder why it is safe to construct an aggregated public key where their counterparty knows the other portion of the aggregated secret key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made my viewpoints public above that I don't feel this contributes much to the article. We need to make a call about do we keep this or not.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, we can keep it. Ideally it should just be somewhere else, with a link

RFC/src/RFC-0202_TariScriptOpcodes.md Outdated Show resolved Hide resolved
RFC/src/RFC-0202_TariScriptOpcodes.md Outdated Show resolved Hide resolved
RFC/src/RFC-0202_TariScriptOpcodes.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SWvheerden, nice additions after my last review and after consideration some more comments. Thanks.

\end{aligned}
\tag{1}
$$

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a section here called "Key equivalence across different elliptic curves" or the like is warranted, similar to how "Key construction" is explained.
This is essentially a Discrete log equivalence (DLEQ) proof and should be mentioned. Reference to the Monero article https://web.getmonero.org/resources/research-lab/pubs/MRL-0010.pdf and more general use for example https://projekter.aau.dk/projekter/files/260345149/Electronic_voting_application_based_on_public_verifiable_secret_sharing.pdf paragraph 5.3.1 and https://crypto.stackexchange.com/questions/60127/key-equivalence-across-different-elliptic-curves should be made, possible with a very brief one sentence summary about the differences between the two approaches.

$$


### Commitment phase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is important to mention here why a DLEQ proof is not needed and how the script (committed to in the input script signature) fulfils that role, especially to prime the audience that expects a DLEQ proof should be present.


Doing atomic swaps with Monero is more complicated and requires a cryptographic dance to complete as Monero does not
implement any form of HTLC's or the like. This means that when doing an atomic swap with Monero, most of the logic will
have to be implemented on the Tari side. Atomic swaps between Monero and bitcoin have been implemented by the [Farcaster project](https://github.com/farcaster-project/RFCs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably Bitcoin should be capitalized throughout.

@stringhandler stringhandler changed the title docs: tari-monero atomic swap docs: RFC-0241 tari-monero atomic swap Dec 8, 2021
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just something I realized looking at this revision but I believe that Ed25519 is actually referring to the EdDSA signature scheme and we have been using it to describe the ECC private-public key cryptography. I believe it is more correct to refer to those points as Curve25519 points rather than Ed25519 points. It is arguable whether the phrase Ed25519 public key is correct or not depending on if it is referring to the public key in a signature or not but this is not always the case?

I just wanted to check that everywhere we are referring to Ed25519 we are talking about a signature rather than just a key.

RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor corrections/notes as I was reading through this

@@ -325,6 +325,28 @@ push 0.
Identical to [`CheckSig`](#checksigmsg), except that nothing is pushed to the stack if the signature is valid, and the
operation fails with `VERIFY_FAILED` if the signature is invalid.

##### ConvertToRistrettoPoint,

Pop the top element, converts it to a Ristretto point, and pushes this to the stack.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear what the source element would be for this conversion. Of the stack items allowed (Number, Hash, Commitment, PublicKey, Signature) non really fit as inputs to this op, are you proposing a 32-byte blob or would this be a conversion from PublicKey? Hash is 32 bytes but I would say it isn't appropriate for this use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I realized this, and for now, the script mentions Hash as it can be used as 32 bytes. Left his out for now till we can discuss what route we want forward with this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct approach in my view would be to define a new StackItem type RistrettoScalar(RisrettoSecretKey)

Suggested change
Pop the top element, converts it to a Ristretto point, and pushes this to the stack.
Posp the top element, which must be a valid Ristretto scalar, calculates the corresponding Ristretto point and pushes this to the stack.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yup that makes sense

Comment on lines +68 to +76
Doing atomic swaps with Monero is more complicated and requires a cryptographic dance to complete as Monero does not
implement any form of HTLC's or the like. This means that when doing an atomic swap with Monero, most of the logic will
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tari-Monero atomic swaps are significantly more involved than BTC atomic swaps as Monero does not
implement scripting that allows for any form of HTLCs or similar. 

RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
refunds slower, but it should be safer in giving more time to finalize this.

Allowing both to claim the Tari after the second lock height is, on face value, a safer option. This can be done by enabling
either party to claim the script with the lapse transaction. The counterparty can then claim the Monero. However, this
Copy link
Member

@sdbondi sdbondi Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
either party to claim the script with the lapse transaction. The counterparty can then claim the Monero. However, this
either party to claim the Tari funds with the lapse transaction. The counterparty can then claim the Monero. However, this

### Detail

This method relies purely on TariScript to enforce the exposure of the private Monero aggregate keys. Based on [Point Time Lock Contracts](https://suredbits.com/payment-points-part-1/), the script forces the spending party to supply their Monero
private key part as input data to the script, evaluated via the operation Ed25519Point. This TariScript operation will
Copy link
Member

@sdbondi sdbondi Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private key part as input data to the script, evaluated via the operation Ed25519Point. This TariScript operation will
private key part as input data to the script, evaluated using the `ToEd25519Point` operation. This TariScript operation will

RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
RFC/src/RFC-0241_AtomicSwapXMR.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments up to and not including the BTC-XTR section

@@ -325,6 +325,28 @@ push 0.
Identical to [`CheckSig`](#checksigmsg), except that nothing is pushed to the stack if the signature is valid, and the
operation fails with `VERIFY_FAILED` if the signature is invalid.

##### ConvertToRistrettoPoint,

Pop the top element, converts it to a Ristretto point, and pushes this to the stack.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct approach in my view would be to define a new StackItem type RistrettoScalar(RisrettoSecretKey)

Suggested change
Pop the top element, converts it to a Ristretto point, and pushes this to the stack.
Posp the top element, which must be a valid Ristretto scalar, calculates the corresponding Ristretto point and pushes this to the stack.

RFC/src/RFC-0202_TariScriptOpcodes.md Outdated Show resolved Hide resolved
@@ -48,7 +46,7 @@ technological merits of the potential system outlined herein.

## Goals

The aim of this Request for Comment (RFC) is to describe how an Atomic swap will be created.
This Request for Comment (RFC) aims to describe how an Atomic swap will be created.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created.. where? between which parties / chains?

RFC/src/RFC-0240_AtomicSwap.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good. I especially like the discussion about Key equivalence and additional security changes to method 1; addition of the DLEQ proof, addition of the blinding factor to the Monero private key and corresponding changes to the script.

I think Key equivalence needs some grammatical and punctuation/spacing changes.

hansieodendaal
hansieodendaal previously approved these changes Dec 14, 2021
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stringhandler stringhandler changed the title docs: RFC-0241 tari-monero atomic swap docs: rfc-0241 tari-monero atomic swap Jan 1, 2022
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFCs are still in draft anyway, so I don't mind merging this, but before I do, I would like the following:

  1. Add a link to the Github discussion about RFC 0241
  2. Remove the changes to RFC 0202 and rather mention that a new opcode must be created in RFC 0241. The changes to 0202 can be split into a new PR for future tracking

@SWvheerden
Copy link
Collaborator Author

Moved the changes to TariScript to #3686

SWvheerden and others added 2 commits January 11, 2022 10:30
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
Co-authored-by: Philip Robinson <simian@tari.com>
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are ready to merge this RFC and we will discuss further via the Discussion and chat channels.

@stringhandler stringhandler dismissed their stale review January 14, 2022 15:00

Changes have been made

@aviator-app aviator-app bot merged commit 58aa914 into tari-project:development Jan 14, 2022
@SWvheerden SWvheerden deleted the sw_xtr-xmr_rfc branch January 17, 2022 07:05
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.

6 participants