Arrr integration WIP. #1204

artemii235 · 2022-02-01T11:15:47Z

Generate random p2p messages signing privkey for each order of a privacy coin.
Generate random keypair for HTLC redeem script of privacy coins.
Refactoring.
#927

Hide secrets before broadcasting.

shamardy

Great work! Just a few questions about unclear things to me 🙂

mm2src/lp_swap/recreate_swap_data.rs

mm2src/lp_swap/maker_swap.rs

shamardy · 2022-02-01T21:14:07Z

https://github.com/KomodoPlatform/atomicDEX-API/blob/c3408e9f997bea3448716115919bf78c3c83de4e/mm2src/lp_ordermatch.rs#L415-L418
It seems that pubkey banning will not work with arrr orders, I guess this is a tradeoff for privacy. Not sure if something can be done about this?
Also can from_peer from process_msg be used to link orders to each other? I can see that the max number of peers in the mesh is 12. So as long as the number of peers subscribed to the topic is more than 12 this shouldn't be a problem if I understand correctly. Just a thought I had and wanted to share 🙂

artemii235 · 2022-02-02T06:49:45Z

It seems that pubkey banning will not work with arrr orders, I guess this is a tradeoff for privacy. Not sure if something can be done about this?

Yes, banning won't work. For transparent addresses, we can sign p2p messages with the privkey, which address belongs to. It will allow us to check the balance to additionally validate the order in the future. It's not possible for privacy coins so yes, there will be tradeoffs for privacy. As of now, I'm unsure how we can solve it, but we will maybe have some ideas in the future 🙂

Also can from_peer from process_msg be used to link orders to each other? I can see that the max number of peers in the mesh is 12. So as long as the number of peers subscribed to the topic is more than 12 this shouldn't be a problem if I understand correctly. Just a thought I had and wanted to share

Yes, from_peer and process_msg can be used to link orders between each other. I need to check if we can modify the sender PeerId.

Gossipsub mesh size is not related to it. As per specification https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.0.md#gossipsub-the-gossiping-mesh-router, the mesh is used to transfer a message to a subset of subscribed peers to to reduce bandwidth requirements and increase decentralization and scalability. If the node is removed from another's mesh, it has to add other peers to its mesh to maintain mesh network graph completeness. It doesn't imply that some nodes will be removed from the network completely or senders' PeerIds will be hidden.

Also, in our customized gossipsub version, we form a mesh for relay nodes only, which send the messages to all subscribed "light" nodes.

artemii235 · 2022-02-02T08:31:44Z

Also can from_peer from process_msg be used to link orders to each other?

It should be possible to modify a sender, but I prefer to do it on the next iteration. Orderbook sync doesn't work properly now for these temporary keys anyway. This PR goal is to merge swap-related changes as fast as possible. Added to my checklist at #927 (comment)

sergeyboyko0791

Great progress! First batch of comments :)

sergeyboyko0791 · 2022-02-02T05:17:43Z

mm2src/coins/eth.rs

@@ -64,6 +64,7 @@ pub use ethcore_transaction::SignedTransaction as SignedEthTx;
 pub use rlp;

 mod web3_transport;
+use crate::TradePreimageResult;


Could you please move this import to:

use super::{BalanceError, TradePreimageResult};

mm2src/coins/z_coin.rs

mm2src/lp_swap/maker_swap.rs

sergeyboyko0791 · 2022-02-02T08:27:53Z

mm2src/lp_swap/maker_swap.rs

@@ -452,12 +516,29 @@ impl MakerSwap {
            },
        };

+        let taker_pubkey = taker_data.persistent_pubkey();
+        let (taker_pubkey, maker_coin_htlc_pubkey, taker_coin_htlc_pubkey) = match taker_pubkey.len() {
+            33 => (taker_pubkey.into(), None, None),


How about adding a enum that consists of taker_pubkey only by default and consists of a maker/taker htlc pubkeys pair? It will allow us to save backward compatibility and avoid errors like:

return Ok((Some(MakerSwapCommand::Finish), vec![MakerSwapEvent::NegotiateFailed( ERRL!("Unexpected persistent_pubkey field len {}", taker_pubkey.len()).into(), )]))

I agree enum will be better, will do it tomorrow.

sergeyboyko0791 · 2022-02-02T09:26:29Z

mm2src/lp_swap/maker_swap.rs

+            (None, None)
+        };
+
+        let (taker_coin_htlc_privkey, taker_coin_htlc_pubkey) = if self.taker_coin.is_privacy() {


How about adding a function that generates htlc keys pair if a coin is private. The code below is the same as for maker coin

Makes sense, will do.

sergeyboyko0791 · 2022-02-02T09:49:00Z

mm2src/lp_swap/maker_swap.rs

        };

        Ok((Some(MakerSwapCommand::Negotiate), vec![MakerSwapEvent::Started(data)]))
    }

    async fn negotiate(&self) -> Result<(Option<MakerSwapCommand>, Vec<MakerSwapEvent>), String> {
+        let mut persistent_pubkey = Vec::with_capacity(33);
+        persistent_pubkey.extend_from_slice(self.r().maker_coin_htlc_keypair.public());
+        if self.r().maker_coin_htlc_keypair != self.r().taker_coin_htlc_keypair {


If we add a enum that is either a persistent pubkey or a pair of maker and taker htlc keypairs (see the next comment), then it can perform this checking in its AEnum::with_maker_taker_htlc_keypairs constructor.

sergeyboyko0791 · 2022-02-02T10:02:23Z

mm2src/lp_swap/maker_swap.rs

@@ -717,7 +798,8 @@ impl MakerSwap {
            .validate_taker_payment(
                &self.r().taker_payment.clone().unwrap().tx_hex,
                self.taker_payment_lock.load(Ordering::Relaxed) as u32,
-                &*self.r().other_persistent_pub,
+                &*self.r().other_taker_coin_htlc_pub,
+                &**self.r().taker_coin_htlc_keypair.public(),


I think it's a good time to add a struct that consists of the method arguments.
It's easy to make a mistake, for example, pass maker and taker pubkeys in the wrong order.

I agree, will do it tomorrow.

Geth needs additional configuration to always return old transactions ethereum/go-ethereum#23569 (comment).

artemii235 · 2022-02-07T11:44:12Z

@sergeyboyko0791 It seems I resolved all remarks from the first iteration. Could you review it again, please? 🙂

shamardy

LGTM! 🔥

sergeyboyko0791

Thanks for the changes! I left a few more comments

mm2src/docker_tests.rs

mm2src/lp_swap.rs

artemii235 · 2022-02-18T12:11:52Z

@shamardy @sergeyboyko0791 It's ready for (hopefully) final review iteration.

sergeyboyko0791

Last questions and suggestions 😄

sergeyboyko0791 · 2022-02-21T12:36:37Z

mm2src/lp_swap/maker_swap.rs

+    /// Temporary pubkey used in HTLC redeem script when applicable for taker coin
+    pub taker_coin_htlc_pubkey: Option<H264Json>,
+    /// Temporary privkey used to sign P2P messages when applicable
+    /// Temporary privkey used to sign P2P messages when applicable


Will fix it today.

sergeyboyko0791 · 2022-02-21T12:45:15Z

mm2src/lp_swap/maker_swap.rs

+    fn get_my_negotiation_data(&self) -> NegotiationDataMsg {
+        let r = self.r();
+        let secret_hash = dhash160(&r.data.secret.0).take().to_vec();
+        let maker_coin_swap_contract = self.maker_coin.swap_contract_address().map_or(vec![], |addr| addr.0);


I think it should be a bit more efficient as the default vector is created if there is no contract address:

self.maker_coin.swap_contract_address().map(|addr| addr.0).unwrap_or_else(Vec::new);

Could you please consider refactoring the same lines in taker_swap.rs?

Good note, thanks!

According to https://doc.rust-lang.org/beta/std/option/enum.Option.html#method.map_or, we can also use map_or_else. It uses one line less after rustfmt 🙂

sergeyboyko0791 · 2022-02-21T13:18:13Z

mm2src/lp_swap/maker_swap.rs

+        let maker_coin_swap_contract = self.maker_coin.swap_contract_address().map_or(vec![], |addr| addr.0);
+        let taker_coin_swap_contract = self.taker_coin.swap_contract_address().map_or(vec![], |addr| addr.0);
+
+        if r.my_maker_coin_htlc_keypair != r.my_taker_coin_htlc_keypair {


I think we should use the latest version of the negotiation data.
What if we want to add or change a field in the negotiation data. Currently, I see only one way: Add NegotiationDataV2.1 and NegotiationDataV3.1 that will be used accordingly to this condition:
if r.my_maker_coin_htlc_keypair != r.my_taker_coin_htlc_keypair
I'd suggest just using NegotiationDataV3 only. On the other hand, it will break the backward compatibility with the old nodes that don't support V3 yet. Or return HtlcPubkeyData 😅
I think renaming NegotiationDataV3 to NegotiationDataPubkeysPerCoin is also ok.

NegotiationDataPubkeysPerCoin will break a naming convention that we already have (V1, V2, etc). If we need to add new fields, we will add NegotiationDataV4. It won't be mandatorily tied to if r.my_maker_coin_htlc_keypair != r.my_taker_coin_htlc_keypair, we will maybe have another condition or make it default and compatible with NegotiationDataV3. Also, when the majority of nodes are updated, we can remove the condition and make the V3 default for everyone.

According to this, I don't think we need to change the existing code now (but we will change it later).

artemii235 · 2022-02-22T04:04:16Z

@sergeyboyko0791 Could you please review it again? 🙂

sergeyboyko0791

Thanks for the changes! LGTM

# Conflicts: # mm2src/coins/eth.rs # mm2src/coins/utxo/qtum.rs # mm2src/coins/utxo/utxo_common.rs # mm2src/coins/utxo/utxo_standard.rs

artemii235 · 2022-02-22T11:42:47Z

@sergeyboyko0791 I merged the dev into my branch - there were some conflicts. Could you please take a quick look and check that I didn't break something?

sergeyboyko0791 · 2022-02-23T04:28:39Z

If I can trust a condensed version of this 2785820 merge commit, everything looks good.
I've tested UTXO activation functionality with Trezor locally and it works as expected 👍

artemii235 and others added 22 commits January 18, 2022 18:36

WIP. Async refactoring.

c90e38d

Fix Clippy.

db382a8

Fix tests.

9020f62

WIP. Removed hardcoded tx fee.

b25f8df

WIP. Temporary privkeys for z orders and swaps.

8b40d81

Move rand6 and secp256k1 from dev dependencies.

c70e7d3

Fix WASM.

021fcfc

WIP. Implementing temporary privkeys.

dc2797e

WIP. Implementing temporary privkeys.

560f14b

Fix Clippy.

60859d4

WIP. Code doesn't compile.

ab4b04a

WIP. Made first RICK/ZOMBIE swap with temp keys.

9d97820

WIP. Fixing tests compilation.

2d59a8b

WIP. Trading tests fail.

2aee5c8

WIP. Adding separate keypairs for maker and taker coin.

09005bc

WIP. Adding separate keypairs for maker and taker coin. Fix WASM.

aa67eeb

WIP. Investigating test_update_maker_order failure.

e3630e3

Merge branch 'dev' into arrr-integration

6004953

Do not broadcast swap status if one of the coin is privacy one.

5bc0202

Hide secrets before broadcasting.

Fix fmt.

848f5a3

Fix WASM.

4c939a9

Do not run check_stats_swap_status for zhtlc feature.

c3408e9

artemii235 requested review from shamardy and sergeyboyko0791 February 1, 2022 11:15

shamardy reviewed Feb 1, 2022

View reviewed changes

mm2src/lp_swap/recreate_swap_data.rs Outdated Show resolved Hide resolved

mm2src/lp_swap/recreate_swap_data.rs Outdated Show resolved Hide resolved

mm2src/lp_swap/maker_swap.rs Outdated Show resolved Hide resolved

Add m/taker_coin_htlc_pubkey fields to M/TakerSwapData.

69f00b7

sergeyboyko0791 suggested changes Feb 2, 2022

View reviewed changes

Artem Vitae added 6 commits February 2, 2022 18:05

Batch of minor review fixes.

9cd6f22

Add ValidatePaymentInput for validate_m/taker_payment.

decb2fa

Add ValidatePaymentInput for validate_m/taker_payment.

55ac6e6

Use Infura for ETH dex fee validation tests temporary.

a9acd72

Geth needs additional configuration to always return old transactions ethereum/go-ethereum#23569 (comment).

Fix fmt.

4952e12

More review fixes.

8a5076e

shamardy previously approved these changes Feb 8, 2022

View reviewed changes

sergeyboyko0791 reviewed Feb 9, 2022

View reviewed changes

mm2src/docker_tests.rs Outdated Show resolved Hide resolved

mm2src/lp_swap.rs Outdated Show resolved Hide resolved

Artem Vitae added 2 commits February 18, 2022 18:26

Add NegotiationDataV3.

176a33e

Merge remote-tracking branch 'origin/dev' into arrr-integration

b6810f6

artemii235 dismissed shamardy’s stale review via b6810f6 February 18, 2022 11:26

shamardy previously approved these changes Feb 21, 2022

View reviewed changes

sergeyboyko0791 reviewed Feb 21, 2022

View reviewed changes

Minor review fixes.

cf5bce3

artemii235 dismissed shamardy’s stale review via cf5bce3 February 22, 2022 03:51

sergeyboyko0791 previously approved these changes Feb 22, 2022

View reviewed changes

Artem Vitae added 2 commits February 22, 2022 18:18

Merge remote-tracking branch 'origin/dev' into arrr-integration

2785820

# Conflicts: # mm2src/coins/eth.rs # mm2src/coins/utxo/qtum.rs # mm2src/coins/utxo/utxo_common.rs # mm2src/coins/utxo/utxo_standard.rs

Fixes after merge.

0ffa36a

artemii235 dismissed sergeyboyko0791’s stale review via 0ffa36a February 22, 2022 11:32

artemii235 merged commit 66cab10 into dev Feb 25, 2022

artemii235 deleted the arrr-integration branch February 25, 2022 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arrr integration WIP. #1204

Arrr integration WIP. #1204

artemii235 commented Feb 1, 2022

shamardy left a comment

shamardy commented Feb 1, 2022 •

edited

Loading

artemii235 commented Feb 2, 2022

artemii235 commented Feb 2, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 Feb 2, 2022

sergeyboyko0791 Feb 2, 2022

artemii235 Feb 2, 2022

sergeyboyko0791 Feb 2, 2022

artemii235 Feb 2, 2022

sergeyboyko0791 Feb 2, 2022

sergeyboyko0791 Feb 2, 2022

artemii235 Feb 2, 2022

artemii235 commented Feb 7, 2022

shamardy left a comment

sergeyboyko0791 left a comment

artemii235 commented Feb 18, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 Feb 21, 2022

artemii235 Feb 22, 2022

sergeyboyko0791 Feb 21, 2022 •

edited

Loading

artemii235 Feb 22, 2022

artemii235 Feb 22, 2022

sergeyboyko0791 Feb 22, 2022

sergeyboyko0791 Feb 21, 2022

artemii235 Feb 22, 2022 •

edited

Loading

artemii235 commented Feb 22, 2022

sergeyboyko0791 left a comment

artemii235 commented Feb 22, 2022

sergeyboyko0791 commented Feb 23, 2022

Arrr integration WIP. #1204

Arrr integration WIP. #1204

Conversation

artemii235 commented Feb 1, 2022

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Feb 1, 2022 • edited Loading

artemii235 commented Feb 2, 2022

artemii235 commented Feb 2, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 commented Feb 7, 2022

shamardy left a comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

artemii235 commented Feb 18, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

artemii235 commented Feb 22, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

artemii235 commented Feb 22, 2022

sergeyboyko0791 commented Feb 23, 2022

shamardy commented Feb 1, 2022 •

edited

Loading

sergeyboyko0791 Feb 21, 2022 •

edited

Loading

artemii235 Feb 22, 2022 •

edited

Loading