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 appeal success should rollback all newer transactions to pending #768

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

Conversation

kstroobants
Copy link
Contributor

@kstroobants kstroobants commented Dec 19, 2024

Fixes #662

What

  • Added start and stop event for async tasks for each contract based on address.
  • Added boolean flag to see if the task is still running for each contract based on address.
  • For each contract when a rollback is needed, I empty the pending queue and I put the status of the newer transactions to pending.
  • Same applies for accepted and undetermined transactions.
  • Add order by created_at in get_pending_transactions() query to process the oldest first.
  • The validator set is kept when rollbacked. This means that if a transaction has an increased validator set because of an appeal and that transaction need to be rollbacked to pending then it keeps the increased validator set (confirmed with Kiril).

Why

  • Future transactions depend on each other. If one needs to go back to pending because of a successful appeal then the transactions that came after need to be rolled back.
  • Creates value for the user.

Testing done

  • Tested the new feature in the localhost

Decisions made

  • I use asyncio.Event() to stop and start the tasks. Other option was to use a boolean flag.
  • With while is_pending_queue_task_running() in rollback_transactions() I let the processing of the current transaction finish (until accepted or undetermined state) by the pending queue processor task to return it back to pending in a clean way. If it needs to be faster we can add a check for the stop event in the state transition while loop of exec_transaction() to stop after the current state transition so it doesn't need to go until accepted/undetermined but we need to see if it doesn't break anything.
  • I empty the pending queue as I assume all transactions in the pending queue were send after the appealed accepted/undetermined transaction. So the _crawl_snapshot will get the rolled back transactions and the pending transactions that were already in the queue. Other option is to store the created_at of the appealed transaction in the consensusalgorithm class and only process if the current selected pending transaction's created_at is the same as the stored value. This will filter the newer that we present in the queue and they will be added at the end of the queue by the _crawl_snapshot.
  • The leader appeal is processed by the appeal_window thread, implemented in PR feat: appeals undetermined transactions #657. Otherwise the code will be confusing when a leader appeal should be rollbacked when a validator appeal is successful (to select the correct validator set).
  • I rollback all newer transactions for successful appealed accepted and undetermined transaction to the pending state. Other option is to not rollback when the contract state was not changed.

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

Check code changes and try it.

User facing release notes

A leader appeal success and validator appeal success will rollback all newer transactions to the pending status.

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 Dec 19, 2024
@kstroobants kstroobants marked this pull request as draft December 19, 2024 11:19
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

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

Project coverage is 18.16%. Comparing base (add6a9e) to head (df3f7e4).
Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
...ntend/src/components/Simulator/TransactionItem.vue 0.00% 23 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     #768      +/-   ##
==========================================
- Coverage   19.55%   18.16%   -1.40%     
==========================================
  Files         129      129              
  Lines       10006    10104      +98     
  Branches      319      302      -17     
==========================================
- Hits         1957     1835     -122     
- Misses       7964     8184     +220     
  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
37.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@kstroobants kstroobants marked this pull request as ready for review December 20, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consensus: appeal success should rollback all newer transactions to pending
1 participant