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

Enhance Cardano transactions rollbacks #1917

Merged
merged 6 commits into from
Sep 11, 2024
Merged

Enhance Cardano transactions rollbacks #1917

merged 6 commits into from
Sep 11, 2024

Conversation

jpraynaud
Copy link
Member

@jpraynaud jpraynaud commented Sep 5, 2024 •
edited
Loading

Content

This PR includes some enhancements to the handling of Cardano transactions rollbacks:

  • Enhanced logging for easier spotting of roll-backs and distinction between buffer rollbacks and full rollbacks (with storage impact)
  • Fixed a bug that caused panics with the following cases:
    • The rollback is done on a block number not recorded in the Cardano transaction store (now targets closest block number above slot number)
    • The rollback does not impact the Cardano transactions store (no record to erase)

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Issue(s)

Relates to #1840
Closes #1929

@jpraynaud jpraynaud self-assigned this Sep 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 5, 2024 •
edited
Loading

Test Results

    4 files  ±0     53 suites  ±0   9m 17s ⏱️ -9s
1 252 tests +2  1 252 ✅ +2  0 💤 ±0  0 ❌ ±0 
1 463 runs  +2  1 463 ✅ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 9eae92d. ± Comparison against base commit aff35a8.

This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both. 
mithril-persistence ‑ database::repository::cardano_transaction_repository::tests::repository_get_block_number_by_slot_number

mithril-persistence ‑ database::query::cardano_transaction::get_cardano_transaction::tests::with_highest_block_number_below_slot_number
mithril-persistence ‑ database::repository::cardano_transaction_repository::tests::remove_rolled_back_transactions_and_block_range_by_slot_number
mithril-persistence ‑ database::repository::cardano_transaction_repository::tests::repository_get_closest_block_number_by_slot_number

♻️ This comment has been updated with latest results.

@jpraynaud jpraynaud force-pushed the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch from f330e1a to 733fc67 Compare September 5, 2024 15:20
@jpraynaud jpraynaud force-pushed the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch from 733fc67 to 88fdac2 Compare September 6, 2024 08:03
@jpraynaud jpraynaud force-pushed the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch from 88fdac2 to f4a7779 Compare September 6, 2024 08:28
@jpraynaud jpraynaud marked this pull request as ready for review September 6, 2024 09:48
sfauvel
sfauvel approved these changes Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

just some comments on the form in the tests

internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs Show resolved Hide resolved
internal/mithril-persistence/src/database/query/cardano_transaction/get_cardano_transaction.rs Show resolved Hide resolved
internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs Show resolved Hide resolved
@jpraynaud jpraynaud force-pushed the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch from f4a7779 to 1f2b0b6 Compare September 10, 2024 15:39
@jpraynaud jpraynaud force-pushed the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch from 1f2b0b6 to 87a56a9 Compare September 10, 2024 15:55
@jpraynaud jpraynaud force-pushed the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch from 87a56a9 to a9b3324 Compare September 10, 2024 16:09
sfauvel
sfauvel approved these changes Sep 11, 2024
View reviewed changes
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM

internal/mithril-persistence/src/database/query/cardano_transaction/get_cardano_transaction.rs Show resolved Hide resolved
@jpraynaud
fix: remove rolled-back transactions edge cases
b280774 
Does not trigger an error when no block exists above the slot number,
also find the closest block above the slot number if rolled back block does not have transaction.
dlachaume
dlachaume reviewed Sep 11, 2024
View reviewed changes
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

Just two minor comments but LGTM 👍 . I'm waiting for the modification of the SQL condition before approving.

@jpraynaud
chore: apply review comments
ab1d016
@jpraynaud
docs: update CHANGELOG
95a5329
@jpraynaud
chore: bump crates versions
9eae92d 
- 'mithril-persistence' from '0.2.25' to '0.2.26'
- 'mithril-aggregator' from '0.5.61' to '0.5.62'
- 'mithril-common' from '0.4.50' to '0.4.51'
- 'mithril-signer' from '0.2.180' to '0.2.181'.
@jpraynaud jpraynaud force-pushed the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch from a9b3324 to 9eae92d Compare September 11, 2024 09:29
dlachaume
dlachaume approved these changes Sep 11, 2024
View reviewed changes
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jpraynaud jpraynaud merged commit f083934 into main Sep 11, 2024
41 of 45 checks passed
@jpraynaud jpraynaud deleted the jpraynaud/1840-enhance-cardano-transactions-rollbacks branch September 11, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alenar Alenar Alenar approved these changes

@sfauvel sfauvel sfauvel approved these changes

@dlachaume dlachaume dlachaume approved these changes

Assignees

@jpraynaud jpraynaud

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic on rollback on slot number not recorded in the Cardano transactions store
4 participants
@jpraynaud @Alenar @sfauvel @dlachaume