EVA Protocol improvements

Tribler / kotlin-ipv8

P2P communication library for Android

62 stars 27 forks source link

EVA Protocol improvements #63

Closed rmadhwal closed 1 year ago

rmadhwal commented 2 years ago

This PR addresses issues with the EVA protocol dicussed in https://github.com/Tribler/trustchain-superapp/issues/101

The implemented changes are:

Change of block size from 1200 to 600 and timeout increased to 20s to compensate for the smaller transfers. I'm not entirely sure about the numbers and the only reason I have for setting them this way is that they worked constitently in my experiments, if there is a better method for determining these numbers, please let me know
Packet Loss fixes and Write Request retries as implemented in https://github.com/Tribler/tribler/pull/6831 (I did not implement the Future and Shutdown functionality since unlike python I'm not aware of a use case for this yet)

The changes worked fine in my local testing using the EVA protocol with Freedom of Computing, please let me know if you require an APK to verify them

@synctext @drew2a

synctext commented 1 year ago

@InvictusRMC @rmadhwal Can we fix this PR please? The initial EVA message is currently not re-sent. Does this fix open "must haves" for EVA?

InvictusRMC commented 1 year ago

@synctext yes, I have asked @KoningR whether he was aware of this PR. As he also has a PR open with changes to EVA, I hope he can ensure his changes work with these.

rmadhwal commented 1 year ago

@synctext yes, in my testing this seemed to work consistently with medium sized files (3 - 10 MB)

synctext commented 1 year ago

@rmadhwal : as you proposed this change back in April 2022: lets merge this PR first.

@KoningR please review and comment this PR. Is there a conflict with your recent PR work? Your proposed PR with "+- 97% faster serialization, 95% faster deserialization" is impressive :rocket: Please test master with .APK on real phone and make sure we don't break things too badly for the 63 enrolled Blockchain students.

rmadhwal commented 1 year ago

@InvictusRMC there's a discrepancy between the version of Kotlin this patch was pushed on and its current version causing the tests to fail since TestCoroutineScope seems to have been deprecated after 1.6, I'll push a new patch with the fixed tests

InvictusRMC commented 1 year ago

@InvictusRMC there's a discrepancy between the version of Kotlin this patch was pushed on and its current version causing the tests to fail since TestCoroutineScope seems to have been deprecated after 1.6, I'll push a new patch with the fixed tests

Thank you; I appreciate it. Meanwhile, I pushed some suppression so that the build at least passes 😄