[r2r] Extend swap watcher node functionality for UTXO #1496

caglaryucekaya · 2022-10-10T19:35:22Z

The second PR for the swap watcher nodes adds the following improvements as part of #1431:

State machine implementation
Refund taker payment functionality
Check if the maker payment is already spent or refunded
Wait for the takers before spending the maker payment
Keep track of running watchers using TimeCache
Error handling with ValidatePaymentError
Validate the public key of the source of the watcher message
Prevent spam attacks by validating the taker fee

…tchtower

sergeyboyko0791

Thanks for the fixes! Just a few notes

sergeyboyko0791 · 2022-10-24T11:19:40Z

mm2src/coins/solana.rs

@@ -616,6 +598,56 @@ impl SwapOps for SolanaCoin {
    fn validate_other_pubkey(&self, _raw_pubkey: &[u8]) -> MmResult<(), ValidateOtherPubKeyErr> { unimplemented!() }
 }

+#[allow(clippy::forget_ref, clippy::forget_copy, clippy::cast_ref_to_mut)]


Is this required? If I'm not mistaken, it's needed if a trait or a struct is mockable, but not sure

Could you please also check the other places where #[allow(clippy::forget_ref, clippy::forget_copy, clippy::cast_ref_to_mut)] is used.

sergeyboyko0791 · 2022-10-24T11:21:00Z

mm2src/coins/tendermint/tendermint_coin.rs

@@ -786,6 +769,56 @@ impl SwapOps for TendermintCoin {
    fn validate_other_pubkey(&self, _raw_pubkey: &[u8]) -> MmResult<(), ValidateOtherPubKeyErr> { todo!() }
 }

+#[async_trait]
+#[allow(unused_variables)]


How about marking unused variables with _ prefix?

sergeyboyko0791 · 2022-10-24T11:32:14Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

-        let duration = (self.r().data.lock_duration * 4) / 5;
-        let timeout = self.r().data.started_at + duration;
-
+        let timeout = wait_for_taker_payment_conf_until(self.r().data.started_at, self.r().data.lock_duration);


This is the reason why I thought about step_1, step_2 initially. At the spend_taker_payment we should have gotten the required taker payment confirmations already, and wait_for_taker_payment_conf_until looks a bit weird.
Let's leave it as it is, but I think it's worth to rename this function somehow.

step_1/step_2/... is not informative and possible cause for a mess in the future: if a step is added or deleted, one can have a hard time figuring out the renumbering of the steps.
But I agree that wait_for_taker_payment_conf_until is not a good match to use in spend_taker_payment. I assume an appropriate name would be like taker_payment_spend_deadline.

sergeyboyko0791 · 2022-10-24T11:35:24Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

-                self.w().taker_spends_maker_payment_preimage = preimage_hex
+            TakerSwapEvent::WatcherMessageSent(taker_spends_maker_payment, taker_refunds_payment) => {
+                self.w().taker_spends_maker_payment_preimage = taker_spends_maker_payment;
+                self.w().taker_refunds_payment = taker_refunds_payment;


Shouldn't taker_refunds_payment name similarly to taker_spends_maker_payment_preimage? I mean taker_refunds_payment_preimage.

artemii235 · 2022-10-25T08:01:14Z

@caglaryucekaya As I can see, only minor notes are left. Could you please push the current state of this PR and add leftover items to the checklist, similar to #1432 (comment)?

UPD Link to the checklist: #1431 (comment)

artemii235

Approving with one TODO for the next sprint.

artemii235 · 2022-10-25T08:20:15Z

mm2src/coins/utxo/utxo_common.rs

+        let mut attempts = 0;
+        let taker_fee_hash = H256Json::from(taker_fee_hash.as_slice());
+        loop {
+            let taker_fee_tx = match coin


Please add more validations that are done in utxo_common::validate_fee on the next iteration. As of now, a trading party can submit any transaction signed by its pubkey, and it will pass the validation. It might be not even a dex fee transaction.

Alright you can merge the current state. I was working on making the watchers periodically check if the taker already spent the maker payment, thought I could make it in this sprint but that also requires some review so it's better I leave it to the next sprint, together with the final notes here I didn't work on.

I think we should also check for the transaction time or block height since a trading party can reuse an old fee transaction signed by his pub and it will also pass validation without this check.

shamardy

Great Work! Only 2 comments for next sprint :)

shamardy · 2022-10-25T10:31:49Z

mm2src/common/time_cache.rs

@@ -271,6 +271,10 @@ where
    }

    pub fn contains(&mut self, key: &Key) -> bool { self.0.contains_key(key) }
+


I guess #[allow(dead_code)] can be removed from here https://github.com/KomodoPlatform/atomicDEX-API/blob/ca8e435a9e3361dd9b7190970ce1fee4a72db3cb/mm2src/common/time_cache.rs#L250-L254 Now that DuplicateCache is used.

shamardy · 2022-10-25T11:41:52Z

mm2src/coins/utxo/utxo_common.rs

+        let mut attempts = 0;
+        let taker_fee_hash = H256Json::from(taker_fee_hash.as_slice());
+        loop {
+            let taker_fee_tx = match coin


I think we should also check for the transaction time or block height since a trading party can reuse an old fee transaction signed by his pub and it will also pass validation without this check.

artemii235 · 2022-10-25T17:22:39Z

@ozkanonur @sergeyboyko0791 @borngraced Could you please check if you have something to add?
@caglaryucekaya Could you please then put all leftover review notes to the checklist here? #1431 (comment)

sergeyboyko0791

@caglaryucekaya @artemii235 I don't have more notes. Please fix the comments from the last review at the next iteration.
Anyway, the last comments are more cosmetic, so great job 🔥

borngraced

LGTM!! 👏👏👏

laruh

Looks good to me! 🚀

artemii235 · 2022-10-26T08:30:40Z

@caglaryucekaya IRIS swaps PR is merged. Could you solve git conflicts, please?

onur-ozkan

🔥

…tchtower

sergeyboyko0791

LGTM! 🔥

caglaryucekaya added 30 commits September 1, 2022 19:25

add initial swap watcher node functionality

a8d110b

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into wa…

735dc2b

…tchtower

broadcast watcher message immediately after taker payment is sent

b922e4e

rename the WatcherData struct to TakerSwapWatcherData

8127ee5

use separate SwapWatcherMsg for watchers

638f1b7

make taker send the entire spending transaction to watcher nodes

a14d31d

broadcast watcher message periodically in wait_for_taker_payment_spend

8d2e6cb

check if transaction outputs is empty in p2sh_spending_tx

39f5be2

fix minor details

a534d8d

move utxo coin generation methods to docker_tests_common

8b57858

use Mm2TestConf for watcher node tests

df84e4d

prevent running multiple watcher threads for the same swap

1f0c6a7

remove async from spawn_taker_swap_watcher

efe4294

check if htlc spend fee is greater than the transaction output

96e1aeb

add empty line at the end of docker_tests_common

bf93a96

discard prev_transaction mutability after modification

03044f1

check if transaction outputs is empty in utxo_common functions

52ecd87

use the drop_mutability macro

979d036

move all watcher related code to the swap_watcher module

d897dbf

improve the taker_swap_watchers usage

807386e

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into wa…

63ffaa9

…tchtower

remove unused types and structs

693c005

check if the transaction script has instructions

726afe3

check if the transaction has any inputs

e89f88d

fix minor stuff

c0f8c1e

make watchers validate taker payment at most once

011ba95

release taker_swap_watchers lock after inserting the uuid to the set

34dc5b7

fix the error type in instruction iterator

4958834

reorder taker_swap_watchers lock/release structure

152d507

use separate files for large json test strings

2521f43

caglaryucekaya added 5 commits October 20, 2022 02:47

add functions for wait taker/maker payment confirmation durations

2e67cc4

improve error handling

9a18733

rename check_all_inputs_signed_by_pub method

563395b

add small fixes

90f69d3

fix WatcherError naming

ca8e435

caglaryucekaya changed the title ~~[wip] Extend swap watcher node functionality for UTXO~~ [r2r] Extend swap watcher node functionality for UTXO Oct 22, 2022

sergeyboyko0791 suggested changes Oct 24, 2022

View reviewed changes

caglaryucekaya changed the title ~~[r2r] Extend swap watcher node functionality for UTXO~~ [wip] Extend swap watcher node functionality for UTXO Oct 24, 2022

artemii235 previously approved these changes Oct 25, 2022

View reviewed changes

artemii235 mentioned this pull request Oct 25, 2022

Implement swap watcher nodes. #1431

Open

caglaryucekaya changed the title ~~[wip] Extend swap watcher node functionality for UTXO~~ [r2r] Extend swap watcher node functionality for UTXO Oct 25, 2022

shamardy previously approved these changes Oct 25, 2022

View reviewed changes

sergeyboyko0791 previously approved these changes Oct 25, 2022

View reviewed changes

borngraced previously approved these changes Oct 25, 2022

View reviewed changes

laruh previously approved these changes Oct 26, 2022

View reviewed changes

onur-ozkan previously approved these changes Oct 26, 2022

View reviewed changes

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into wa…

3519e4b

…tchtower

caglaryucekaya dismissed stale reviews from onur-ozkan, laruh, borngraced, sergeyboyko0791, shamardy, and artemii235 via 3519e4b October 26, 2022 10:36

sergeyboyko0791 approved these changes Oct 26, 2022

View reviewed changes

sergeyboyko0791 merged commit 4d2071f into dev Oct 26, 2022

sergeyboyko0791 deleted the watchtower branch October 26, 2022 11:35

sergeyboyko0791 mentioned this pull request Jan 9, 2023

[r2r] Complete MetaMask #1591

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] Extend swap watcher node functionality for UTXO #1496

[r2r] Extend swap watcher node functionality for UTXO #1496

caglaryucekaya commented Oct 10, 2022 •

edited

Loading

sergeyboyko0791 left a comment

sergeyboyko0791 Oct 24, 2022

sergeyboyko0791 Oct 24, 2022

sergeyboyko0791 Oct 24, 2022

sergeyboyko0791 Oct 24, 2022

artemii235 Oct 24, 2022

sergeyboyko0791 Oct 24, 2022

artemii235 commented Oct 25, 2022 •

edited

Loading

artemii235 left a comment

artemii235 Oct 25, 2022

caglaryucekaya Oct 25, 2022

shamardy Oct 25, 2022

shamardy left a comment

shamardy Oct 25, 2022

shamardy Oct 25, 2022

artemii235 commented Oct 25, 2022 •

edited

Loading

sergeyboyko0791 left a comment •

edited

Loading

borngraced left a comment

laruh left a comment

artemii235 commented Oct 26, 2022

onur-ozkan left a comment

sergeyboyko0791 left a comment

		@@ -271,6 +271,10 @@ where
		}

		pub fn contains(&mut self, key: &Key) -> bool { self.0.contains_key(key) }

[r2r] Extend swap watcher node functionality for UTXO #1496

[r2r] Extend swap watcher node functionality for UTXO #1496

Conversation

caglaryucekaya commented Oct 10, 2022 • edited Loading

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

artemii235 commented Oct 25, 2022 • edited Loading

artemii235 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

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 commented Oct 25, 2022 • edited Loading

sergeyboyko0791 left a comment • edited Loading

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

artemii235 commented Oct 26, 2022

onur-ozkan left a comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

caglaryucekaya commented Oct 10, 2022 •

edited

Loading

artemii235 commented Oct 25, 2022 •

edited

Loading

artemii235 commented Oct 25, 2022 •

edited

Loading

sergeyboyko0791 left a comment •

edited

Loading