fix panic when corner case happen in RandomDelayIter #335

al8n · 2023-08-29T08:44:59Z

No description provided.

danielSanchezQ · 2023-08-29T11:40:51Z

mixnet/client/src/sender.rs

+
+#[test]
+fn test_random_delay_iter_zero_total_time() {
+    const TOTAL_DELAYS: u64 = 3;


This const could be global shared if you use for all tests.

zeegomo · 2023-08-29T12:10:06Z

mixnet/client/src/sender.rs

@@ -133,21 +133,60 @@ where
            return None;
        }

+        // corner case: 0 average delay
+        if self.avg_delay == 0 {
+            self.remaining_delays -= 1;


We can set remaining delays at line 135 and avoid putting it in every branch

zeegomo · 2023-08-29T12:11:17Z

mixnet/client/src/sender.rs

        // Calculate bounds to avoid extreme values
        let lower_bound = (self.avg_delay as f64 * 0.5) as u64;
        let upper_bound = (self.avg_delay as f64 * 1.5)
-            .min((self.remaining_time - self.remaining_delays + 1) as f64)
+            .min((self.remaining_time as f64 - self.remaining_delays as f64) + 1.0)


not sure I understand this formula

The line .min((self.remaining_time - self.remaining_delays + 1) as f64) is included to ensure that upper_bound never exceeds the maximum possible delay that can be returned without causing future delays to be zero or negative.

self.remaining_time is the total time still available for all remaining delays, and self.remaining_delays is the number of remaining delays, then the expression (self.remaining_time - self.remaining_delays + 1) calculates the maximum time we can allocate for the current delay without making future delays invalid (i.e., zero or negative).

For example, consider a case where we have remaining_time = 100 and remaining_delays = 10. If we use up 95 time units for the current delay, then we would be left with only 5-time units to distribute among the remaining 9 delays, which is not possible without using zero or negative delays.

Here's how the logic works:

self.remaining_time is the remaining time that needs to be allocated across all remaining delays.

self.remaining_delays is the number of remaining delays.

The maximum we could use for the current delay to be still able to allocate at least 1 unit of time for each of the remaining delays is (self.remaining_time - self.remaining_delays + 1).

The min function is used to choose the smaller of the two computed upper bounds (1.5 * avg_delay and (self.remaining_time - self.remaining_delays + 1)) to make sure that the program doesn't allocate too much time for the current delay.

Got it, better to put this in a comment as well (it's enought to point out that we don't want future delays to be zero)

zeegomo · 2023-08-29T12:12:22Z

mixnet/client/src/sender.rs

+        // corner case: when lower bound is greater than upper bound
+        if lower_bound >= upper_bound {
+            self.remaining_delays -= 1;
+            self.remaining_time = 0;
+            return Some(self.remaining_time);
+        }


could we set lower_bound = (self.avg_delay as f64 * 0.5).min(upper_bound) as u64; and avoid having another corner case?

mixnet/client/src/sender.rs

youngjoon-lee

Thank you!

zeegomo · 2023-08-29T12:59:35Z

mixnet/client/src/sender.rs


-        let delay = self.rng.gen_range(lower_bound, upper_bound);
+        let delay = self.rng.gen_range(lower_bound, upper_bound).floor() as u64;


is upper bound included?

No, the upper bound is exclusive.

well, what happens then if lower bound == upper bound? I think it probably panics

Ah, yes, my bad, thanks!

zeegomo · 2023-08-29T13:16:10Z

mixnet/client/src/sender.rs

-        let delay = self.rng.gen_range(lower_bound, upper_bound);
-        self.remaining_time -= delay;
-        self.remaining_delays -= 1;
+        let delay = self.rng.gen_range(lower_bound, upper_bound + 1.0).floor() as u64;


not a fan of +1, as you need to do some calculations to show that it's safe to use and it's easy to mess it up with future refactors. The new api would be better, but since we have to use an old version of rand, I suggest we use https://docs.rs/rand/0.7.3/rand/distributions/struct.Uniform.html#method.new_inclusive

Good suggestion! Thanks!

zeegomo · 2023-08-29T13:36:44Z

mixnet/client/src/sender.rs

-            as u64;
+            // guarantee that we don't exceed the remaining time and promise the delay we return is
+            // at least 1ms.
+            .min((self.remaining_time as f64 - self.remaining_delays as f64) + 1.0);


.min((self.remaining_time as f64).saturating_sub(self.remaining_delays - 1)) and I think we can remove both the avg delay and remaining time edge cases

If we remove the avg delay, then we may have extreme delays, and with higher probability delay1 > delay 2 > delay3 and etc.

nono, I mean just removing the edge case at line 139. If we craft the formula with a bit more care we can make it work in most cases without having to add checks

Ah, yeah, sorry for the misunderstanding.

I still think we need the saturating sub in .min((self.remaining_time as f64).saturating_sub(self.remaining_delays - 1))

zeegomo · 2023-08-29T14:42:27Z

mixnet/client/src/sender.rs

-            as u64;
+            // guarantee that we don't exceed the remaining time and promise the delay we return is
+            // at least 1ms.
+            .min(self.remaining_time.saturating_sub(self.remaining_delays) as f64 + 1.0);


we don't need the +1 anymore since we decrease remaining_delays before this

zeegomo · 2023-08-30T09:10:56Z

mixnet/client/src/sender.rs

+        for _ in 0..TOTAL_DELAYS {
+            assert!(delays.next().is_some());
+        }
+        assert!(delays.next().is_none());


we should probably check that the total wait is indeed 1, also a duration instead of a u64 would be much better because otherwise it's not clear what unit we are using

zeegomo · 2023-08-30T09:26:47Z

mixnet/client/src/sender.rs

+
+    #[test]
+    fn test_random_delay_iter_small_total_time() {
+        let mut delays = RandomDelayIterator::new(rand::thread_rng(), TOTAL_DELAYS, 1);


in RandomDelayIterator::new(rng, total_delays, total_time), total_time should be a Duration and not a u64

* Add `mixnode` and `mixnet-client` crate (#302) * Add `mixnode` binary (#317) * Integrate mixnet with libp2p network backend (#318) * Fix #312: proper delays (#321) * proper delays * add missing duration param * tiny fix: compilation error caused by `rand` 0.8 -> 0.7 * use `get_available_port()` for mixnet integration tests (#333) * add missing comments * Overwatch mixnet node (#339) * Add mixnet service and overwatch app * remove #[tokio::main] --------- Co-authored-by: Youngjoon Lee <taxihighway@gmail.com> * fix tests for the overwatch mixnode (#342) * fix panic when corner case happen in RandomDelayIter (#335) * Use `log` service for `mixnode` bin (#341) * Use `wire` for MixnetMessage in libp2p (#347) * Prevent tmixnet tests from running forever (#363) * Use random delay when sending msgs to mixnet (#362) * fix a minor compilation error caused by the latest master * Fix run output fd (#343) * add a connection pool * Exp backoff (#332) * move mixnet listening into separate task * add exponential retry for insufficient peers in libp2p * fix logging * Fix MutexGuard across await (#373) * Fix MutexGuard across await Holding a MutexGuard across an await point is not a good idea. Removing that solves the issues we had with the mixnet test * Make mixnode handle bodies coming from the same source concurrently (#372) --------- Co-authored-by: Youngjoon Lee <taxihighway@gmail.com> * Move wait at network startup (#338) We now wait after the call to 'subscribe' to give the network the time to register peers in the mesh before starting to publish messages * Remove unused functions from mixnet connpool (#374) * Mixnet benchmark (#375) * merge fixes * add `connection_pool_size` field to `config.yaml` * Simplify mixnet topology (#393) * Simplify bytes and duration range ser/de (#394) * optimize bytes serde and duration serde --------- Co-authored-by: Al Liu <scygliu1@gmail.com> Co-authored-by: Daniel Sanchez <sanchez.quiros.daniel@gmail.com> Co-authored-by: Giacomo Pasini <Zeegomo@users.noreply.github.com>

fix panic when corner case happen in RandomDelayIter

c4efe1d

al8n added enhancement New feature or request mixnet labels Aug 29, 2023

al8n added this to the Mixnet milestone Aug 29, 2023

al8n requested a review from youngjoon-lee August 29, 2023 08:44

al8n self-assigned this Aug 29, 2023

fix panic when corner case happen in RandomDelayIter

0826b4a

danielSanchezQ reviewed Aug 29, 2023

View reviewed changes

zeegomo requested changes Aug 29, 2023

View reviewed changes

youngjoon-lee reviewed Aug 29, 2023

View reviewed changes

mixnet/client/src/sender.rs Outdated Show resolved Hide resolved

mixnet/client/src/sender.rs Outdated Show resolved Hide resolved

fix PR comment

d3310f6

youngjoon-lee approved these changes Aug 29, 2023

View reviewed changes

al8n requested a review from zeegomo August 29, 2023 12:54

zeegomo reviewed Aug 29, 2023

View reviewed changes

fix PR comment

953be33

al8n requested a review from zeegomo August 29, 2023 13:05

zeegomo reviewed Aug 29, 2023

View reviewed changes

fix PR comment

d2f9dcb

youngjoon-lee linked an issue Aug 29, 2023 that may be closed by this pull request

Set delays when building Sphinx packets #312

Closed

zeegomo reviewed Aug 29, 2023

View reviewed changes

al8n added 2 commits August 29, 2023 21:49

fix PR comment

efa1d85

fix PR comment

e2a1e4b

zeegomo reviewed Aug 29, 2023

View reviewed changes

al8n added 2 commits August 30, 2023 00:08

fix PR comment

7f4059c

Merge branch 'mixnet' into mixnet-delay-corner-case

8b5fee8

al8n requested a review from zeegomo August 29, 2023 16:21

youngjoon-lee mentioned this pull request Aug 30, 2023

Overwatch mixnet node #339

Merged

zeegomo requested changes Aug 30, 2023

View reviewed changes

fix PR comment

13f8366

zeegomo requested changes Aug 30, 2023

View reviewed changes

fix PR comment

f657dfb

zeegomo approved these changes Aug 30, 2023

View reviewed changes

al8n merged commit 82421ce into mixnet Aug 30, 2023
2 checks passed

al8n deleted the mixnet-delay-corner-case branch August 30, 2023 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix panic when corner case happen in RandomDelayIter #335

fix panic when corner case happen in RandomDelayIter #335

al8n commented Aug 29, 2023

danielSanchezQ Aug 29, 2023

zeegomo Aug 29, 2023

zeegomo Aug 29, 2023

al8n Aug 29, 2023 •

edited

zeegomo Aug 29, 2023

zeegomo Aug 29, 2023

youngjoon-lee left a comment

zeegomo Aug 29, 2023

al8n Aug 29, 2023

zeegomo Aug 29, 2023

al8n Aug 29, 2023

zeegomo Aug 29, 2023 •

edited

al8n Aug 29, 2023

zeegomo Aug 29, 2023

al8n Aug 29, 2023 •

edited

zeegomo Aug 29, 2023 •

edited

al8n Aug 29, 2023

zeegomo Aug 29, 2023

zeegomo Aug 29, 2023

zeegomo Aug 30, 2023

zeegomo Aug 30, 2023


		let delay = self.rng.gen_range(lower_bound, upper_bound);
		let delay = self.rng.gen_range(lower_bound, upper_bound).floor() as u64;

fix panic when corner case happen in RandomDelayIter #335

fix panic when corner case happen in RandomDelayIter #335

Conversation

al8n commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al8n Aug 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngjoon-lee 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

zeegomo Aug 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al8n Aug 29, 2023 • edited

Choose a reason for hiding this comment

zeegomo Aug 29, 2023 • edited

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

al8n Aug 29, 2023 •

edited

zeegomo Aug 29, 2023 •

edited

al8n Aug 29, 2023 •

edited

zeegomo Aug 29, 2023 •

edited