test(P2P): add test for peer time sync validation by dimxy · Pull Request #2304 · GLEECBTC/komodo-defi-framework

dimxy · 2024-12-25T07:26:22Z

While doing review for peer time sync validation #2255 code I wrote a test for that.
(Attn: @onur-ozkan)

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

This reverts commit 2bfe34c.

This reverts commit c8d1165.

This reverts commit b6d6fbc.

onur-ozkan · 2024-12-26T06:02:15Z

mm2src/common/common.rs

+#[cfg(not(feature = "for-tests"))]
 pub fn get_utc_timestamp() -> i64 { Utc::now().timestamp() }

+/// get_utc_timestamp for tests allowing to add some bias to 'now'
+#[cfg(feature = "for-tests")]
+pub fn get_utc_timestamp() -> i64 {
+    Utc::now().timestamp()
+        + std::env::var("TEST_TIMESTAMP_OFFSET")
+            .map(|s| s.as_str().parse::<i64>().unwrap_or_default())
+            .unwrap_or_default()
+}
+


You can use built-in cfg(test)

I need to call it for other crates and I believe cfg(test) is in effect only for unit tests in same crate
(checked btw - test does not work with cfg(test))

there is run-docker-tests feature too

"for-tests" feature is also used already and looks more general for me (not specific for docker tests)

As far as I see you just added for-tests feature, idk how it's already there but it's fine, I will make cfg(test) working and replace all these test flags in another PR.

mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs

borngraced

one note

mm2src/common/common.rs

borngraced

lgtm 🚀

onur-ozkan

LGTM other than last nit

onur-ozkan · 2024-12-30T07:56:23Z

mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs

+#[test]
+fn test_peer_time_sync_validation() {
+    const TIMEOFFSET_TOLERABLE: i64 = 19;
+    const TIMEOFFSET_TOO_BIG: i64 = 21;


We can stabilize this logic a bit more here:

diff --git a/mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs b/mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs index acf60ee55..e2dcd228f 100644 --- a/mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs +++ b/mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs @@ -16,6 +16,7 @@ use common::{block_on, block_on_f01, executor::Timer, get_utc_timestamp, now_sec use crypto::privkey::key_pair_from_seed; use crypto::{CryptoCtx, DerivationPath, KeyPairPolicy}; use http::StatusCode; +use mm2_libp2p::behaviours::atomicdex::MAX_TIME_GAP_FOR_CONNECTED_PEER; use mm2_number::{BigDecimal, BigRational, MmNumber}; use mm2_test_helpers::for_tests::{check_my_swap_status_amounts, disable_coin, disable_coin_err, enable_eth_coin, enable_eth_with_tokens_v2, erc20_dev_conf, eth_dev_conf, get_locked_amount, @@ -5474,8 +5475,8 @@ fn test_approve_erc20() { #[test] fn test_peer_time_sync_validation() { - const TIMEOFFSET_TOLERABLE: i64 = 19; - const TIMEOFFSET_TOO_BIG: i64 = 21; + const TIMEOFFSET_TOLERABLE: i64 = MAX_TIME_GAP_FOR_CONNECTED_PEER - 1; + const TIMEOFFSET_TOO_BIG: i64 = MAX_TIME_GAP_FOR_CONNECTED_PEER + 1; let start_peers_with_time_offset = |offset: i64| -> (Json, Json) { let (_ctx, _, bob_priv_key) = generate_utxo_coin_with_random_privkey("MYCOIN", 10.into());

fixed in 3c6ac2a

onur-ozkan

LGTM, feel free to merge with/without the nit.

onur-ozkan · 2025-01-06T06:32:36Z

mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs

+    let timeoffset_tolerable = TryInto::<i64>::try_into(MAX_TIME_GAP_FOR_CONNECTED_PEER).unwrap() - 1;
+    let timeoffset_too_big = TryInto::<i64>::try_into(MAX_TIME_GAP_FOR_CONNECTED_PEER).unwrap() + 1;


nit: casting to i64 with TryIntos and unwraps seems unnecessary since we are only using this value to convert it into a String.

I intended to have it as i64 to indicate it's possible to emulate time deviation in both directions, if anyone needs

* dev: feat(tendermint): validators RPC (#2310) chore(CI): validate Cargo lock file (#2309) test(P2P): add test for peer time sync validation (#2304)

* dev: (35 commits) fix(crypto): allow non bip39 mnemonics storage (GLEECBTC#2312) fix(legacy_swap): check for existing maker/taker payment before timeout (GLEECBTC#2283) feat(tendermint): validators RPC (GLEECBTC#2310) chore(CI): validate Cargo lock file (GLEECBTC#2309) test(P2P): add test for peer time sync validation (GLEECBTC#2304) fix mm2_p2p dev build (GLEECBTC#2311) update Cargo.lock (GLEECBTC#2308) chore(CI): unlock wasm-pack version (GLEECBTC#2307) add `wasm` feature on WASM for timed-map (GLEECBTC#2306) replace broken rpc link (GLEECBTC#2305) chore(eth-websocket): remove some unnecessary wrappers (GLEECBTC#2291) improvement(CI): switch to proper rust caching (GLEECBTC#2303) fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls (GLEECBTC#2295) chore(ci): Update docker build for wasm (GLEECBTC#2294) chore(p2p): follow-up nits (GLEECBTC#2302) feat(p2p): ensure time synchronization in the network (GLEECBTC#2255) bump libp2p (GLEECBTC#2296) chore(adex-cli): use "Komodo DeFi Framework" name in adex_cli (GLEECBTC#2290) chore(ctx): replace gstuff constructible with oncelock (GLEECBTC#2267) don't rely on core (GLEECBTC#2289) ...

* fix peer response error log (only if error) * add test for peer time difference validation * fix linux clippy * Revert "fix linux clippy" This reverts commit 2bfe34c. * Revert "add test for peer time difference validation" This reverts commit c8d1165. * Revert "fix peer response error log (only if error)" This reverts commit b6d6fbc. * add docker test for peer time sync validation * remove unneeded test println * use Mm2TestConf::seednode * review(borngraced): refactor get_utc_timestamp with test code * use const in peer time sync test

dimxy added 2 commits December 25, 2024 12:08

fix peer response error log (only if error)

b6d6fbc

add test for peer time difference validation

c8d1165

dimxy added the improvement: tests label Dec 25, 2024

fix linux clippy

2bfe34c

dimxy changed the title ~~feat(tests) add test for peer time sync validation~~ feat(tests): add test for peer time sync validation Dec 25, 2024

dimxy changed the title ~~feat(tests): add test for peer time sync validation~~ test(P2P): add test for peer time sync validation Dec 25, 2024

dimxy added the status: pending review label Dec 25, 2024

onur-ozkan reviewed Dec 25, 2024

View reviewed changes

mm2src/mm2_p2p/src/behaviours/atomicdex.rs Outdated Show resolved Hide resolved

mm2src/mm2_p2p/src/behaviours/atomicdex.rs Outdated Show resolved Hide resolved

dimxy added 5 commits December 25, 2024 19:50

Revert "fix linux clippy"

cfa6586

This reverts commit 2bfe34c.

Revert "add test for peer time difference validation"

ef71c7d

This reverts commit c8d1165.

Revert "fix peer response error log (only if error)"

5bd0d96

This reverts commit b6d6fbc.

add docker test for peer time sync validation

bdba931

remove unneeded test println

e735d26

onur-ozkan reviewed Dec 26, 2024

View reviewed changes

mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs Outdated Show resolved Hide resolved

use Mm2TestConf::seednode

5be3e5c

onur-ozkan previously approved these changes Dec 26, 2024

View reviewed changes

borngraced suggested changes Dec 26, 2024

View reviewed changes

mm2src/common/common.rs Show resolved Hide resolved

review(borngraced): refactor get_utc_timestamp with test code

6eaa3cf

dimxy dismissed onur-ozkan’s stale review via 6eaa3cf December 29, 2024 20:06

borngraced previously approved these changes Dec 29, 2024

View reviewed changes

onur-ozkan previously approved these changes Dec 30, 2024

View reviewed changes

use const in peer time sync test

3c6ac2a

dimxy dismissed stale reviews from onur-ozkan and borngraced via 3c6ac2a January 5, 2025 10:16

onur-ozkan approved these changes Jan 6, 2025

View reviewed changes

onur-ozkan merged commit 1eb4bf7 into dev Jan 7, 2025

onur-ozkan deleted the peer-time-diff-test branch January 7, 2025 08:07

dimxy pushed a commit that referenced this pull request Jan 8, 2025

Merge branch 'dev' into fix-utxo-tx-fee-for-change

455403c

* dev: feat(tendermint): validators RPC (#2310) chore(CI): validate Cargo lock file (#2309) test(P2P): add test for peer time sync validation (#2304)

shamardy mentioned this pull request Apr 16, 2025

chore(release): v2.4.0-beta #2345

Merged

		let timeoffset_tolerable = TryInto::<i64>::try_into(MAX_TIME_GAP_FOR_CONNECTED_PEER).unwrap() - 1;
		let timeoffset_too_big = TryInto::<i64>::try_into(MAX_TIME_GAP_FOR_CONNECTED_PEER).unwrap() + 1;

Conversation

dimxy commented Dec 25, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants