[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table #1665

borngraced · 2023-02-19T22:15:22Z

resolves: #1646

Signed-off-by: borngraced <[email protected]>

sergeyboyko0791

Thank you for the great enhancement! I have one important question

sergeyboyko0791 · 2023-02-20T15:10:35Z

mm2src/mm2_main/src/lp_swap.rs

+    taker_coin_swap_contract: Vec<u8>,
+    maker_coin_htlc_pub: Vec<u8>,
+    taker_coin_htlc_pub: Vec<u8>,
+    persistent_pubkey: Vec<u8>,


Please try to avoid adding NegotiationDataV4 message. I even think this new variant might break backwards compatibility.
I personally think it's better to store maker_coin_htlc_pub as maker_pubkey and taker_coin_htlc_pub as taker_pubkey. @shamardy what do you think?

waiting for @shamardy feedback on this...

I agree with @sergeyboyko0791 here. Using persistent_pubkey also leaks private information for private ARRR swaps.

@borngraced is this PR ready for review? NegotiationDataV4 is still not removed from the code.

Oh sorry, I missed that! and yes it's ready for review.

onur-ozkan

lgtm, 1 question

onur-ozkan · 2023-02-21T08:38:37Z

mm2src/mm2_main/src/database/stats_swaps.rs

+    "ALTER TABLE stats_swaps ADD COLUMN maker_pubkey VARCHAR(255);",
+    "ALTER TABLE stats_swaps ADD COLUMN taker_pubkey VARCHAR(255);",


Can't just "ALTER TABLE stats_swaps ADD COLUMN maker_pubkey VARCHAR(255), ADD COLUMN taker_pubkey VARCHAR(255); work?

yes thanks.. it could be this also, but I followed a convention that I already used before https://github.com/KomodoPlatform/atomicDEX-API/blob/54081307cd093cce321939b618a684ad16c915a7/mm2src/mm2_main/src/database/stats_swaps.rs#L49-L52

Signed-off-by: borngraced <[email protected]>

shamardy

Good start :)
First review iteration, we just need to be all inline about implementation details at this point.

shamardy · 2023-02-28T11:30:05Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

 const TAKER_PAYMENT_SPEND_SEARCH_INTERVAL: f64 = 10.;

 pub const TAKER_SUCCESS_EVENTS: [&str; 11] = [
    "Started",
    "Negotiated",
    "TakerFeeSent",
-    "TakerPaymentInstructionsReceived",
+    "TakerPaymentInstructionscReceived",


You seem to have changed this event unintentionally

shamardy · 2023-02-28T14:04:31Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

@@ -1702,6 +1704,7 @@ impl MakerSwapStatusChanged {
 #[derive(Debug, Default, PartialEq, Serialize, Deserialize)]
 pub struct MakerSavedSwap {
    pub uuid: Uuid,
+    pub my_persistence_pub: Option<H264Json>,


Can you please explain why you added my_persistence_pub to MakerSavedSwap/TakerSavedSwap. Isn't it available in TakerSwapData/MakerSwapDatahttps://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L513 https://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L143 I really just think we should add maker_coin_htlc_pubkeyand taker_coin_htlc_pubkey to the DB as is, there should be 4 pubkeys in this case (maker_pubkey_for_maker_coin, maker_pubkey_for_taker_coin, taker_pubkey_for_maker_coin, taker_pubkey_for_taker_coin), please note that these 4 can be different when using HD wallet functionality, we can get @sergeyboyko0791 input on this.

@cipig which exact pubkey do you parse from the json files? I can see that in stats_swaps in seednodes there will be either a MakerSavedSwap or TakerSavedSwap so I guess you use my_persistence_pub and one of maker_coin_htlc_pubkey/taker_coin_htlc_pubkey from MakerNegotiationData/TakerNegotiationData

@shamardy, with the help of Sergey's input, I got this context

There can be multiple cases. Let's consider the Maker side only:

Maker sends NegotiationDataV1 or NegotiationDataV2 where NegotiationDataV1/2::persistent_pubkey is considered as maker_pubkey (its persistent pubkey that identifiers him uniquely)

Maker sends NegotiationDataV3 where NegotiationDataV3::maker_coin_htlc_pub and NegotiationDataV3::taker_coin_htlc_pub are the same. If the maker_coin and taker_coin tickers are not ARRR (or other private coin), we can consider NegotiationDataV3::m/taker_coin_htlc_pub as maker_pubkey (its persistent pubkey that identifiers him uniquely)

Maker sends NegotiationDataV3 where NegotiationDataV3::maker_coin_htlc_pub and NegotiationDataV3::taker_coin_htlc_pub are different. In that case we probably should try to figure out what coin is private in the swap, and what coin is not. The private coin pubkey should have a random m/taker_coin_htlc_pub that is unique between all swaps. But the public coin should have the same public key as the persistent_pubkey

and also the data in

pub my_persistence_pub: Option<H264Json>,

Is the persistence_pub from T/MakerSwap
https://github.com/KomodoPlatform/atomicDEX-API/blob/a6f9a0ea383d6f87f2157bb6598a76eb7fa10071/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L464

As we discussed DM, there can be the case when it's better not to share the persistent public key. For example, when you exchange two private coins (like ARRR).
But on the other hand, I am not sure if maker_coin_maker_htlc_pubkey, maker_coin_taker_htlc_pubkey, taker_coin_maker_htlc_pubkey, taker_coin_taker_htlc_pubkey pubkeys can be useful as they can be different, and we can't recognise a node that performed the swaps.

Is the persistence_pub from T/MakerSwap

@borngraced what I meant is that you can use the one from these events https://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L1538
https://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L635 instead of adding it to MakerSavedSwap/TakerSavedSwap

As we discussed DM, there can be the case when it's better not to share the persistent public key. For example, when you exchange two private coins (like ARRR).
But on the other hand, I am not sure if maker_coin_maker_htlc_pubkey, maker_coin_taker_htlc_pubkey, taker_coin_maker_htlc_pubkey, taker_coin_taker_htlc_pubkey pubkeys can be useful as they can be different, and we can't recognise a node that performed the swaps.

@smk762 @ca333 We need your inputs on this, it all comes down to if we want to track one pubkey for all swap operations related to a node, or track the pubkeys related to the used HD account for the swap. This can be considered another issue different from the current one. As for the current one I think we should just use the pubkeys @cipig used to parse from the json files.

Perhaps we can use the PeerID for private coins?
The reason for the request was to gather stats on most active users (and able to delineate maker/taker activity), so for HD perhaps a single identifier for the node would be best.

Signed-off-by: borngraced <[email protected]>

shamardy

Next iteration! We need a unit test added to make sure that the right pubkeys are saved.

shamardy · 2023-03-06T18:00:51Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

+                swap_pubkeys.maker = Some(started.my_persistent_pub);
+                swap_pubkeys.taker = started.maker_coin_htlc_pubkey;


Aren't those 2 pubkeys the maker's? can you please add a unit test to test that the right pubkeys are saved to DB? you should use 3 nodes for the test (taker/maker/seednode) and use stats_swap_status RPC to check that the right pubkeys are saved to the seednode's DB.

shamardy · 2023-03-06T18:01:39Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+                swap_pubkeys.maker = started.maker_coin_htlc_pubkey;
+                swap_pubkeys.taker = Some(started.my_persistent_pub);


Same for those 2 pubkeys, aren't they both the taker's?

@shamardy this is ready for another round of review!

Signed-off-by: borngraced <[email protected]>

# Conflicts: # mm2src/mm2_main/src/lp_swap/maker_swap.rs # mm2src/mm2_main/src/lp_swap/taker_swap.rs

shamardy

Next iteration! Probably the last one :)

mm2src/mm2_main/src/lp_swap/maker_swap.rs

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

Signed-off-by: borngraced <[email protected]>

shamardy

Final review :)
Just some minor stuff left, otherwise everything else looks good.

mm2src/mm2_main/src/lp_swap.rs

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

Signed-off-by: borngraced <[email protected]>

shamardy

Thank you for your patience on this PR. LGTM!

* add change logs for PRs merged to dev only * remove {version} - {date} and add dev branch instead * add ibc transfer change logs * add lightning PR #1655 to change logs * add changelog for #1706 * add changelog for #1694, #1665 --------- Reviewed-by: laruh, caglaryucekaya <[email protected]>

borngraced added 2 commits February 18, 2023 00:23

saved dev state

430b2a9

Signed-off-by: borngraced <[email protected]>

include pubkeys in stats_swaps.rs

4bf3833

Signed-off-by: borngraced <[email protected]>

borngraced requested review from caglaryucekaya, shamardy and onur-ozkan February 19, 2023 22:15

borngraced self-assigned this Feb 19, 2023

borngraced added 4 commits February 19, 2023 23:16

remove println

630a9fe

Signed-off-by: borngraced <[email protected]>

Merge branch 'dev' into add-pubkeys-to-stats-swap

b9a233e

refactor get_swap_pubkeys -> swap_pubkeys

c860d15

Signed-off-by: borngraced <[email protected]>

change ADD_MAKER_TAKER_PUBKEYS types

12b2b41

Signed-off-by: borngraced <[email protected]>

sergeyboyko0791 suggested changes Feb 20, 2023

View reviewed changes

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Feb 20, 2023

borngraced mentioned this pull request Feb 20, 2023

[release] 1.0.0-beta #1576

Merged

onur-ozkan previously approved these changes Feb 21, 2023

View reviewed changes

remove NegotiationDataV4

0c642a2

Signed-off-by: borngraced <[email protected]>

borngraced dismissed onur-ozkan’s stale review via 0c642a2 February 24, 2023 01:26

Merge branch 'dev' into add-pubkeys-to-stats-swap

6b057da

borngraced changed the title ~~[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [wip] Include maker/taker pubkeys in MM2.db stats_swaps table Feb 24, 2023

minor changes — ready for review

dc612e9

Signed-off-by: borngraced <[email protected]>

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Feb 25, 2023

borngraced added 2 commits February 26, 2023 23:50

remove is_private_coin fn

f39c52d

Signed-off-by: borngraced <[email protected]>

remove NegotiationDataV4

76d0929

Signed-off-by: borngraced <[email protected]>

shamardy requested changes Feb 28, 2023

View reviewed changes

remove my_persistent_pub from maker/taker_swap

4aa8539

Signed-off-by: borngraced <[email protected]>

borngraced changed the title ~~[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [wip] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 1, 2023

borngraced added 2 commits March 3, 2023 17:07

remove my_persistent_pub from maker/taker_swap

ef088e3

Signed-off-by: borngraced <[email protected]>

fix unit tests

a581dfb

Signed-off-by: borngraced <[email protected]>

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 3, 2023

shamardy requested changes Mar 6, 2023

View reviewed changes

borngraced changed the title ~~[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [wip] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 7, 2023

save dev state — stats_swap swap coin pubkeys unit test

fe856bf

Signed-off-by: borngraced <[email protected]>

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 8, 2023

borngraced added 4 commits March 8, 2023 02:25

finish impl unit test

275bf27

Signed-off-by: borngraced <[email protected]>

remove #[ignore]

012c2a3

Signed-off-by: borngraced <[email protected]>

Merge remote-tracking branch 'origin/dev' into add-pubkeys-to-stats-swap

526ffa0

# Conflicts: # mm2src/mm2_main/src/lp_swap/maker_swap.rs # mm2src/mm2_main/src/lp_swap/taker_swap.rs

Merge remote-tracking branch 'origin/dev' into add-pubkeys-to-stats-swap

12ec822

shamardy requested changes Mar 10, 2023

View reviewed changes

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

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Outdated Show resolved Hide resolved

borngraced and others added 2 commits March 12, 2023 06:02

minor changes

5f5a3ea

Signed-off-by: borngraced <[email protected]>

remove unused

f40f582

Signed-off-by: borngraced <[email protected]>

shamardy reviewed Mar 13, 2023

View reviewed changes

mm2src/mm2_main/src/lp_swap.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Outdated Show resolved Hide resolved

undo unit tests changes

0e3f647

Signed-off-by: borngraced <[email protected]>

shamardy approved these changes Mar 14, 2023

View reviewed changes

shamardy merged commit 5faec02 into dev Mar 14, 2023

shamardy deleted the add-pubkeys-to-stats-swap branch March 14, 2023 12:31

shamardy added a commit that referenced this pull request Mar 14, 2023

add changelog for #1694, #1665

dd3a42d

ca333 mentioned this pull request Mar 27, 2023

[release] 1.0.1-beta #1721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table #1665

[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table #1665

borngraced commented Feb 19, 2023

sergeyboyko0791 left a comment

sergeyboyko0791 Feb 20, 2023

borngraced Feb 21, 2023

shamardy Feb 23, 2023

shamardy Feb 27, 2023

borngraced Feb 27, 2023

onur-ozkan left a comment

onur-ozkan Feb 21, 2023

borngraced Feb 21, 2023

shamardy left a comment

shamardy Feb 28, 2023

shamardy Feb 28, 2023

borngraced Feb 28, 2023 •

edited

Loading

sergeyboyko0791 Feb 28, 2023

shamardy Feb 28, 2023

shamardy Feb 28, 2023

smk762 Feb 28, 2023

shamardy left a comment

shamardy Mar 6, 2023

borngraced Mar 7, 2023

shamardy Mar 6, 2023

borngraced Mar 8, 2023

shamardy left a comment

shamardy left a comment

shamardy left a comment

		"ALTER TABLE stats_swaps ADD COLUMN maker_pubkey VARCHAR(255);",
		"ALTER TABLE stats_swaps ADD COLUMN taker_pubkey VARCHAR(255);",

		swap_pubkeys.maker = Some(started.my_persistent_pub);
		swap_pubkeys.taker = started.maker_coin_htlc_pubkey;

		swap_pubkeys.maker = started.maker_coin_htlc_pubkey;
		swap_pubkeys.taker = Some(started.my_persistent_pub);

[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table #1665

[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table #1665

Conversation

borngraced commented Feb 19, 2023

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

onur-ozkan left a 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

borngraced Feb 28, 2023 • 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

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

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

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

borngraced Feb 28, 2023 •

edited

Loading