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

feat: consensus thread improvements #812

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

kstroobants
Copy link
Contributor

@kstroobants kstroobants commented Jan 14, 2025

Fixes #667, #690, #767

What

  • Improved the crawl_snaphot for the pending queue
    • Added queue field for transactions database table so the transaction is not added again to the queue. It solves the following. Before this fix, a transaction that was being processed would have been added multiple times to the pending queue depending on how long the processing takes. Also, when there were multiple non-processed transactions in the pending queue, the crawl_snapshot would keep adding them every 5seconds. The bad thing was that the run_consensus picks every 5s a transaction from the queue so having a lot of transactions pending postpones the processing a lot (imagine n pending transactions, after 5s run_consensus processes a transaction while crawl_snapshot appends n-1 transactions to the pending queue, again after 5s run_consensus processes a transaction while crawl_snapshot appends n-2 transactions to the pending queue, ... The pending queue needs to be empty (5s * length of queue) before a newly added transaction can be processed).
    • Removed pending check in PendingState class because of queue field (this existed to filter out the duplicated transactions in the queue).
  • Created parallel tasks for appeal window processing. Each contract has an appeal window task.
  • Removed called_from_pending of PendingState because of queue field. This data member was used for leader appeals that use the PendingState and should not be picked up/processed by the exec_transaction (other thread than the appeal_window thread). Because of the queue field, the transaction will not be added to the pending queue and the appeal window will process this transaction.
  • Updated consensus test.
    • All tests use the 3 threads (crawl pending, process pending and process accepted/undetermined).
    • It removes duplicated code that was present for appeal_window.
    • It tests it more like how it is actually run.
    • It makes it possible to add tests using multiple transactions.
    • MessageHandlerMock prints the log events for better debugging when a test fails.

Why

  • To add more value to the user

Testing done

  • Tested the new feature in the studio and in the consensus test

Decisions made

  • Queue field in transactions databases table makes won't add the transaction again to the queue. Con is that transactions can get stuck when the program crashes. That was already the case for transactions in a proposing, committing or revealing state. Solution could be to make a restore function that needs to be ran before the consensus threads start to put all queue field back to False (it can also rollback all stuck transactions to pending). I will create an issue for this.
  • The appeal window uses a for loop over all the accepted/undetermined transactions to process them. Another option would have been an asyncio.queue like in the pending queue. The for loop is basically the ayncio.queue but with 0s sleep between processing the transactions. When we use a asyncio.queue, we do not have power over which appeal should be processed first. Because we do not know where the appeal_window thread is in the asyncio.queue when the appeals are submitted (transaction removed from queue -> check if appealed -> later on the transaction is again added to the end of the queue). When 2 or more transactions are appealed there are two options (the first one is implemented):
    • The transaction list is sorted on created_at, so the transaction that is created first is processed first. This will have less appeal calculations when a rollback is needed.
    • The transaction list is sorted on time of appeal, so the transaction that is appealed first is processed first. Transactions that are not appealed need to be checked for finalization in another loop. I need to create an issue for this one if it is preferred.
  • For updating the consensus test to not have the appeal window code duplicated, I added the creating of threads using the consensus/base.py code and remove the duplicated code. Another solution would have been to run an appeal function like exec_transaction for the pending transactions so no threads are used.

Checks

  • I have tested this code
  • I have reviewed my own PR
  • I have created an issue for this PR
  • I have set a descriptive PR title compliant with conventional commits

Reviewing tips

Run the studio, check the code changes.

Depends on

PR #657

User facing release notes

  • Optimization of when transactions are added to the pending queue.
  • Contracts work in parallel to process appealed transactions and finalizations.
  • The consensus mechanism is tested with threads.

kstroobants and others added 30 commits October 22, 2024 15:24
- Add N+2 validators, remove leader
- Use latest data of transaction when in pending state and not the old one from the crawler
- Write consensus data before setting status to have it updated in frontend when going to transaction info modal
- Do not deploy contract when transaction was in the undetermined state
@kstroobants kstroobants self-assigned this Jan 14, 2025
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 0% with 44 lines in your changes missing coverage. Please review.

Project coverage is 16.12%. Comparing base (b9a9226) to head (1b439c2).

Files with missing lines Patch % Lines
...ntend/src/components/Simulator/TransactionItem.vue 0.00% 34 Missing ⚠️
...tend/src/components/Simulator/TransactionsList.vue 0.00% 6 Missing ⚠️
frontend/src/views/Simulator/RunDebugView.vue 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #812      +/-   ##
==========================================
- Coverage   16.17%   16.12%   -0.06%     
==========================================
  Files         124      124              
  Lines        9707     9738      +31     
  Branches      236      236              
==========================================
  Hits         1570     1570              
- Misses       8052     8083      +31     
  Partials       85       85              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
68.5% Coverage on New Code (required ≥ 80%)
6.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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