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.

Sign up for GitHub

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

Jump to bottom

Added simple bandwidth accounting mechanism #5626

Merged
merged 4 commits into from
Oct 24, 2020
Merged

Added simple bandwidth accounting mechanism #5626

merged 4 commits into from
Oct 24, 2020

Conversation

devos50
Copy link
Contributor

@devos50 devos50 commented Oct 11, 2020 •
edited
Loading

This PR implements a first, simple version of the bandwidth accounting mechanism introduced in #5623. Even though the mechanism is simple, the PR has grown a bit more than I anticipated since I had to also make various GUI changes, and I had to write/improve a few tests.

I also made a few modifications to the trust screen. Specifically, we now only show the evolution of token balances. This simplified the implementation and should be less confusing for the users. Since we only have one line now, I used the Tribler orange for the pen/brush colour 👍

Schermafbeelding 2020-10-16 om 17 59 42

Fixes #5623, fixes #5255

TODO:

  • Discuss/settle on the tunnel community upgrade We will do a hard upgrade of the tunnel community for Tribler 7.6. This will include a few breaking changes in the tunnels.
  • Update the database backend to use Pony.
  • Add sequence number + timestamp to each transaction.
  • Implement a basic Gumby validation experiment (can be found here)
  • Fix GUI statistics screen.
  • Either fix the GUI trust graph or remove it (to be decided) @xoriole will update the trust screen later. I will open a new issue when this PR has been merged.
  • Complete typing usage
  • Update the current Gumby experiments

@ghost
Copy link

ghost commented Oct 11, 2020 •
edited by ghost
Loading

DeepCode's analysis on #436379 found:

  • ℹ️ 6 minor issues. 👇
  • ✔️ 2 issues were fixed.

Top issues

Description Example fixes
Undefined variable 'BandwidthTransaction' Occurrences: 🔧 Example fixes
Redefining built-in 'sum' Occurrences: 🔧 Example fixes
Redefining name 'my_key' from outer scope (line 15) Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@devos50 devos50 mentioned this pull request Oct 11, 2020
Closed
@devos50 devos50 force-pushed the bandwidth_accounting branch 2 times, most recently from 6116733 to 963ceaf Compare October 12, 2020 13:11
@devos50 devos50 mentioned this pull request Oct 12, 2020
Closed
@devos50 devos50 force-pushed the bandwidth_accounting branch 4 times, most recently from c7c788f to bb9f43b Compare October 12, 2020 16:46
@devos50 devos50 mentioned this pull request Oct 13, 2020
Merged
@devos50 devos50 force-pushed the bandwidth_accounting branch 5 times, most recently from d532049 to 20e3b24 Compare October 16, 2020 13:38
@devos50 devos50 mentioned this pull request Oct 16, 2020
Closed
@devos50 devos50 force-pushed the bandwidth_accounting branch 4 times, most recently from 0c938cc to 3374332 Compare October 17, 2020 10:26
@devos50
Copy link
Contributor Author

devos50 commented Oct 17, 2020

retest this please

@devos50
Copy link
Contributor Author

devos50 commented Oct 17, 2020

(win64 timed out)

@devos50 devos50 marked this pull request as ready for review October 17, 2020 10:35
@devos50 devos50 requested a review from grimadas October 17, 2020 10:35
ichorid
ichorid suggested changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@ichorid ichorid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could you please add a misc table to the database to identify the schema version? Alternatively, you can embed the schema ID in the filename, something like bandwidth_v1.db

src/tribler-core/tribler_core/modules/bandwidth_accounting/database.py Show resolved Hide resolved
src/tribler-core/tribler_core/modules/bandwidth_accounting/database.py Show resolved Hide resolved
src/tribler-core/tribler_core/modules/bandwidth_accounting/history.py Show resolved Hide resolved
src/tribler-core/tribler_core/modules/bandwidth_accounting/transaction.py Show resolved Hide resolved
src/tribler-core/tribler_core/modules/bandwidth_accounting/transaction.py Outdated Show resolved Hide resolved
src/tribler-core/tribler_core/modules/payout_manager.py Show resolved Hide resolved
src/tribler-core/tribler_core/modules/tests/bandwidth_accounting/test_database.py Show resolved Hide resolved
src/tribler-core/tribler_core/modules/tunnel/community/triblertunnel_community.py Show resolved Hide resolved
ichorid
ichorid suggested changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@ichorid ichorid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, perfect PR 👍

@devos50 devos50 mentioned this pull request Oct 20, 2020
Closed
@devos50 devos50 force-pushed the bandwidth_accounting branch 2 times, most recently from 1a1fe86 to 3124b32 Compare October 20, 2020 08:33
@devos50
Copy link
Contributor Author

devos50 commented Oct 20, 2020 •
edited
Loading

@ichorid @grimadas @kozlovsky I processed the feedback 👍

Note that I also have started to remove TrustChain constructs. There is currently no point in loading TrustChain, except for the market which is disabled by default. As such, I removed the TrustChain community launcher and a few configuration options. There are, however, still a few things left that would require some more refactoring and changes. For example, the variable that references the (private) key that we are using in most communities is named trustchain_keypair. We should rename that in another PR.

I also updated the identifiers of the tunnel community, after communicating with @egbertbouman. I will deploy an exit node with the new code after this PR is merged.

Finally, the trust graph in the GUI is broken after this PR is merged. @xoriole already confirmed that he will fix this later.

kozlovsky
kozlovsky previously approved these changes Oct 20, 2020
View reviewed changes
@devos50 devos50 force-pushed the bandwidth_accounting branch 2 times, most recently from f6b0845 to 8aef2ef Compare October 20, 2020 10:53
ichorid
ichorid previously approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@ichorid ichorid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, except for @kozlovsky 's remark.

@devos50
Copy link
Contributor Author

devos50 commented Oct 20, 2020

Win64 tests timeout... (we should really debug this one)

@devos50
Copy link
Contributor Author

devos50 commented Oct 20, 2020

retest this please

kozlovsky
kozlovsky previously approved these changes Oct 20, 2020
View reviewed changes
@devos50 devos50 mentioned this pull request Oct 24, 2020
Merged
@devos50 devos50 dismissed stale reviews from kozlovsky and ichorid via 436379a October 24, 2020 10:11
@sonarcloud
Copy link

sonarcloud bot commented Oct 24, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@devos50 devos50 merged commit c4ddfe5 into Tribler:devel Oct 24, 2020
@devos50 devos50 deleted the bandwidth_accounting branch October 24, 2020 10:22
This was referenced Oct 24, 2020
Closed
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ichorid ichorid ichorid approved these changes

@kozlovsky kozlovsky kozlovsky left review comments

@grimadas grimadas Awaiting requested review from grimadas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A simple mechanism for bandwidth accounting Token balance graph error
4 participants
@devos50 @ichorid @kozlovsky @grimadas