Eva improvements - Githubissues

KoningR commented 1 year ago

This PR introduces 3 improvements to EVA:

Every EVA instance now has an optional argument encrypt = true to enable encryption of all EVA packets.
The (de)serialization of EVA acknowledgement packets has been improved to convert integers directly to bytes instead of converting them to string literals. This saves space and time (+- 97% faster serialization, 95% faster deserialization).
Periodic checking of scheduled transfers is now @Synchronized to prevent random race conditions that become prevalent in low-latency scenarios (e.g. when checking for new transfers every <10 ms).

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 76.47% and project coverage change: +0.04 :tada:

Comparison is base (0623f77) 58.32% compared to head (7f45901) 58.36%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #71 +/- ## ============================================ + Coverage 58.32% 58.36% +0.04% - Complexity 697 699 +2 ============================================ Files 107 107 Lines 4487 4487 Branches 634 634 ============================================ + Hits 2617 2619 +2 + Misses 1538 1534 -4 - Partials 332 334 +2 ``` | [Impacted Files](https://codecov.io/gh/Tribler/kotlin-ipv8/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tribler) | Coverage Δ | | |---|---|---| | [.../java/nl/tudelft/ipv8/messaging/eva/EVAProtocol.kt](https://codecov.io/gh/Tribler/kotlin-ipv8/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tribler#diff-aXB2OC9zcmMvbWFpbi9qYXZhL25sL3R1ZGVsZnQvaXB2OC9tZXNzYWdpbmcvZXZhL0VWQVByb3RvY29sLmt0) | `69.41% <72.54%> (ø)` | | | [ipv8/src/main/java/nl/tudelft/ipv8/Community.kt](https://codecov.io/gh/Tribler/kotlin-ipv8/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tribler#diff-aXB2OC9zcmMvbWFpbi9qYXZhL25sL3R1ZGVsZnQvaXB2OC9Db21tdW5pdHkua3Q=) | `74.80% <75.00%> (+0.79%)` | :arrow_up: | | [...ain/java/nl/tudelft/ipv8/messaging/eva/Transfer.kt](https://codecov.io/gh/Tribler/kotlin-ipv8/pull/71?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tribler#diff-aXB2OC9zcmMvbWFpbi9qYXZhL25sL3R1ZGVsZnQvaXB2OC9tZXNzYWdpbmcvZXZhL1RyYW5zZmVyLmt0) | `96.90% <100.00%> (ø)` | | ... and [4 files with indirect coverage changes](https://codecov.io/gh/Tribler/kotlin-ipv8/pull/71/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tribler) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tribler). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tribler)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

rmadhwal commented 1 year ago

I took a look at the changes and I'm curious if this change could cause issues if two incompatible EvaProtocols try to communicate with each other. i.e. one has encryption enabled and one doesn't ?

Maybe the network packet should indicate that the contents are encrypted so in the decryption step regardless of if the EvaProtocol uses encryption on its messages, an encrypted message can be decrypted ?

Also using "Synchronized" with coroutines is considered an antipattern, if you could make a test to replicate the race condition perhaps you might want to use a coroutine Mutex instead or channels which would suspend instead of blocking the coroutine :)