Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EVA Protocol improvements #63

Merged
merged 4 commits into from
Mar 13, 2023
Merged

EVA Protocol improvements #63

merged 4 commits into from
Mar 13, 2023

Conversation

rmadhwal
Copy link
Contributor

This PR addresses issues with the EVA protocol dicussed in Tribler/trustchain-superapp#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 Improve the EVA protocol: devos50 edition tribler#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
Copy link
Member

synctext commented Mar 6, 2023

@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?

@synctext synctext requested a review from InvictusRMC March 6, 2023 08:45
@InvictusRMC
Copy link
Member

@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
Copy link
Contributor Author

rmadhwal commented Mar 9, 2023

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

@synctext
Copy link
Member

synctext commented Mar 9, 2023

@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 🚀
Please test master with .APK on real phone and make sure we don't break things too badly for the 63 enrolled Blockchain students.

@InvictusRMC InvictusRMC merged commit ac31dab into Tribler:master Mar 13, 2023
@rmadhwal
Copy link
Contributor Author

rmadhwal commented Mar 13, 2023

@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
Copy link
Member

@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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants