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

Replace get by select.limit in BandwidthDatabase.get_latest_transaction #6821

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

drew2a
Copy link
Contributor

@drew2a drew2a commented Mar 21, 2022

This PR fixes #6802.

It seems possible to create a multiple transactions for the given pair or public_key_a and public_key_b by setting bandwidth_database.store_all_transactions to True:

if not bandwidth_database.store_all_transactions:
# Make sure to only store the latest pairwise transaction.
for tx in cls.select(
lambda c: c.public_key_a == transaction.public_key_a and
c.public_key_b == transaction.public_key_b):
tx.delete()
db.commit()
cls(**transaction.get_db_kwargs())
elif not bandwidth_database.has_transaction(transaction):
# We store all transactions and it does not exist yet - insert it.
cls(**transaction.get_db_kwargs())

That's why BandwidthDatabase.get_latest_transaction should be ready for this situation.

@drew2a
Copy link
Contributor Author

drew2a commented Mar 21, 2022

retest this please

1 similar comment
@drew2a
Copy link
Contributor Author

drew2a commented Mar 21, 2022

retest this please

@drew2a
Copy link
Contributor Author

drew2a commented Mar 21, 2022

retest this please

@drew2a
Copy link
Contributor Author

drew2a commented Mar 21, 2022

retest this please

@drew2a
Copy link
Contributor Author

drew2a commented Mar 21, 2022

Unfortunately, GUI tests are dramatically unstable, so, I'm opening the PR to review as is (with the failed GUI test).

@drew2a drew2a requested review from devos50 and kozlovsky March 21, 2022 13:48
@drew2a drew2a marked this pull request as ready for review March 21, 2022 13:48
@drew2a drew2a requested a review from a team March 21, 2022 13:48
kozlovsky
kozlovsky previously approved these changes Mar 21, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@drew2a drew2a requested a review from kozlovsky March 21, 2022 16:40
Copy link
Contributor

@devos50 devos50 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! We indeed missed this case 👍 .

By default, store_all_transactions should be set to False on the end-user side so I don't think this would affect end users under normal circumstances. We actually added the store_all_transactions option to perform network analytics and to get insights in the dynamics of bandwidth transfers (since we would like to analyse as much transactions as possible). However, a user is able enable this flag by enabling the crawler mode of the bandwidth component. Maybe this is what happened and triggered the bug.

@drew2a drew2a merged commit f68dc90 into main Mar 23, 2022
@drew2a drew2a deleted the fix/6802 branch March 23, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants