validate p2p messages and topics #1755

onur-ozkan · 2023-04-13T13:04:37Z

Applies the following checks:

Validate topics if they are mixed or not.
Do early return if the message data is not valid (since no point to iterate over and over on the invalid message)
~~Avoid decoding messages that have more than 25 topics~~
Break the loop right after processing any of SWAP_PREFIX, WATCHER_PREFIX, TX_HELPER_PREFIX topic.

laruh

Thanks for this fix!
Just a note for the first view and question: Why did you choose 25 topics limit?

mm2src/mm2_main/src/lp_network.rs

onur-ozkan · 2023-04-13T14:17:52Z

Why did you choose 25 topics limit?

25 topics can't hurt, and it's quite flexible value for any case I think.

borngraced

Great work! got a question

mm2src/mm2_main/src/lp_network.rs

mm2src/gossipsub/src/protocol.rs

mm2src/mm2_main/src/lp_network.rs

mm2src/gossipsub/src/protocol.rs

mm2src/mm2_main/src/lp_network.rs

rozhkovdmitrii · 2023-04-14T14:59:08Z

mm2src/mm2_main/src/lp_ordermatch.rs

@@ -90,7 +90,8 @@ pub use lp_bot::{start_simple_market_maker_bot, stop_simple_market_maker_bot, St

 #[path = "lp_ordermatch/my_orders_storage.rs"]
 mod my_orders_storage;
-#[path = "lp_ordermatch/new_protocol.rs"] mod new_protocol;
+#[path = "lp_ordermatch/new_protocol.rs"]
+pub(crate) mod new_protocol;


It would be more clear it did not stick up, explained before

Can you clarify this, didn't understand it

Now it can be rolled back because stick up logic has been hidden

mm2src/mm2_main/src/lp_swap.rs

mm2src/mm2_main/src/lp_swap/swap_watcher.rs

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan · 2023-04-17T11:02:31Z

mm2src/coins/tendermint/tendermint_coin.rs

@@ -92,7 +92,7 @@ const ABCI_REQUEST_PROVE: bool = false;
 /// 0.25 is good average gas price on atom and iris
 const DEFAULT_GAS_PRICE: f64 = 0.25;
 pub(super) const TIMEOUT_HEIGHT_DELTA: u64 = 100;
-pub const GAS_LIMIT_DEFAULT: u64 = 100_000;
+pub const GAS_LIMIT_DEFAULT: u64 = 125_000;


https://nyancat.iobscan.io/#/txs/ADA2A1B69E98A3E0BBF2A7183D8DC81C3F194E972A0C13312369B01039F168F3

Could you document(either here in the PR comments or in the changelog) why you are changing this value?

Because HTLCs requiring more gas than our limit lately. Since gas prices can be updated from the coins configuration, this should never be problem even if we make it 200_000.

Signed-off-by: ozkanonur <work@onurozkan.dev>

shamardy

Thanks a lot for the fix! Only one comment for now.

mm2src/gossipsub/src/protocol.rs

Signed-off-by: ozkanonur <work@onurozkan.dev>

shamardy

Just some minor comments :)

mm2src/mm2_main/src/lp_network.rs

mm2src/mm2_main/src/lp_ordermatch.rs

shamardy · 2023-04-18T14:49:01Z

@ozkanonur please merge with latest dev that fixes the failing trade_test_electrum_and_eth_coins test

…x-p2p-stack

Signed-off-by: ozkanonur <work@onurozkan.dev>

…ss_maker_order_updated

shamardy

🔥

shamardy · 2023-04-19T13:59:08Z

@rozhkovdmitrii can you please check if your comments have been fixed or not?
@borngraced @laruh will you be doing another review?

laruh

LGTM! Thanks!

borngraced

Great work and well done 🙌🏼 🔥

…rialized

rozhkovdmitrii · 2023-04-19T18:10:04Z

mm2src/mm2_main/src/lp_ordermatch.rs

+        .find_order_by_uuid_and_pubkey(&uuid, &from_pubkey)
+        .ok_or_else(|| MmError::new(OrderbookP2PHandlerError::OrderNotFound(uuid)))?;
+    order.apply_updated(&updated_msg);
+    drop_mutability!(order);


Does it require to drop mutability explicitly?

drop_mutability! is done to ensure that the order cannot be accidentally modified later in this part of the code.

* optimize p2p network layer Signed-off-by: ozkanonur <work@onurozkan.dev> * update default gas limit for tendermint Signed-off-by: ozkanonur <work@onurozkan.dev> * rollback topic limit Signed-off-by: ozkanonur <work@onurozkan.dev> * remove unused arg from `lp_ordermatch::process_msg` Signed-off-by: ozkanonur <work@onurozkan.dev> * use OrderbookP2PHandlerResult for process_orders_keep_alive and process_maker_order_updated * return one error for failed SwapMsg, SwapStatus deserialization * fix process_swap_msg in wasm to return error if SwapMsg can't be deserialized * remove additional space in SwapMsg, SwapStatus decode error message * roll back crate visibility for new_protocol mod --------- Signed-off-by: ozkanonur <work@onurozkan.dev> Co-authored-by: shamardy <shamardy@yahoo.com>

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan added the under review label Apr 13, 2023

onur-ozkan requested review from caglaryucekaya, ca333, Alrighttt and shamardy April 13, 2023 13:04

onur-ozkan force-pushed the fix-p2p-stack branch 2 times, most recently from e4f1849 to 0f3196c Compare April 13, 2023 13:36

onur-ozkan requested review from borngraced and laruh April 13, 2023 13:48

onur-ozkan force-pushed the fix-p2p-stack branch 2 times, most recently from 2a65949 to b259d56 Compare April 13, 2023 14:08

laruh requested changes Apr 13, 2023

View reviewed changes

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

onur-ozkan force-pushed the fix-p2p-stack branch from b259d56 to e520117 Compare April 13, 2023 14:16

borngraced requested changes Apr 14, 2023

View reviewed changes

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

rozhkovdmitrii reviewed Apr 14, 2023

View reviewed changes

onur-ozkan force-pushed the fix-p2p-stack branch 2 times, most recently from 1c62cc7 to 911bc31 Compare April 16, 2023 22:31

optimize p2p network layer

fe38f18

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan force-pushed the fix-p2p-stack branch from 911bc31 to fe38f18 Compare April 17, 2023 09:58

onur-ozkan commented Apr 17, 2023

View reviewed changes

update default gas limit for tendermint

0da7d9e

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan force-pushed the fix-p2p-stack branch from 42f1bf4 to 0da7d9e Compare April 17, 2023 11:32

shamardy requested changes Apr 17, 2023

View reviewed changes

mm2src/gossipsub/src/protocol.rs Outdated Show resolved Hide resolved

rollback topic limit

292614f

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan force-pushed the fix-p2p-stack branch from 4d2cacc to 292614f Compare April 17, 2023 21:46

shamardy reviewed Apr 18, 2023

View reviewed changes

mm2src/mm2_main/src/lp_network.rs Show resolved Hide resolved

mm2src/mm2_main/src/lp_network.rs Show resolved Hide resolved

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

onur-ozkan added 2 commits April 18, 2023 19:12

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

fe283db

…x-p2p-stack

remove unused arg from lp_ordermatch::process_msg

86ee2b5

Signed-off-by: ozkanonur <work@onurozkan.dev>

use OrderbookP2PHandlerResult for process_orders_keep_alive and proce…

87cd284

…ss_maker_order_updated

shamardy previously approved these changes Apr 18, 2023

View reviewed changes

laruh previously approved these changes Apr 19, 2023

View reviewed changes

borngraced previously approved these changes Apr 19, 2023

View reviewed changes

return one error for failed SwapMsg, SwapStatus deserialization

240506f

shamardy dismissed stale reviews from borngraced, laruh, and themself via 240506f April 20, 2023 00:03

shamardy added 2 commits April 20, 2023 02:27

fix process_swap_msg in wasm to return error if SwapMsg can't be dese…

1233e9b

…rialized

remove additional space in SwapMsg, SwapStatus decode error message

24e5f56

rozhkovdmitrii reviewed Apr 20, 2023

View reviewed changes

shamardy assigned onur-ozkan and shamardy Apr 20, 2023

shamardy added the bug fixing week 1 label Apr 20, 2023

roll back crate visibility for new_protocol mod

3fbe579

rozhkovdmitrii approved these changes Apr 22, 2023

View reviewed changes

onur-ozkan merged commit 547a30a into dev Apr 22, 2023
18 of 20 checks passed

onur-ozkan deleted the fix-p2p-stack branch April 22, 2023 16:11

onur-ozkan added a commit that referenced this pull request Apr 24, 2023

improve p2p stack (#1755)

e21c9d6

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan added a commit that referenced this pull request Apr 26, 2023

improve p2p stack (#1755) (#1753)

c5fbee7

Signed-off-by: ozkanonur <work@onurozkan.dev>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate p2p messages and topics #1755

validate p2p messages and topics #1755

onur-ozkan commented Apr 13, 2023 •

edited

laruh left a comment

onur-ozkan commented Apr 13, 2023

borngraced left a comment

rozhkovdmitrii Apr 14, 2023

onur-ozkan Apr 14, 2023

rozhkovdmitrii Apr 20, 2023

shamardy Apr 20, 2023

onur-ozkan Apr 17, 2023

Alrighttt Apr 22, 2023

onur-ozkan Apr 22, 2023

shamardy left a comment

shamardy left a comment

shamardy commented Apr 18, 2023

shamardy left a comment

shamardy commented Apr 19, 2023

laruh left a comment

borngraced left a comment

rozhkovdmitrii Apr 19, 2023

shamardy Apr 20, 2023

validate p2p messages and topics #1755

validate p2p messages and topics #1755

Conversation

onur-ozkan commented Apr 13, 2023 • edited

laruh left a comment

Choose a reason for hiding this comment

onur-ozkan commented Apr 13, 2023

borngraced 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

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Apr 18, 2023

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Apr 19, 2023

laruh left a comment

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan commented Apr 13, 2023 •

edited