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

Incremental storage of Cardano transactions #1619

Merged
merged 26 commits into from
Apr 9, 2024
Merged

Incremental storage of Cardano transactions #1619

merged 26 commits into from
Apr 9, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Apr 8, 2024 •
edited by dlachaume

Content

This PR optimize the retrieval of cardano transactions. Before each time a Cardano Transaction signed entity type was signed the whole cardano chain was parsed. Now we only parse the immutables that was not parsed before, with a parameter that allow to re-parse previous immutables in case of a previous failure.

There's several internal mithril-common api changes to handle that:

  • Remove TransactionStore trait: persistence is now defined by the nodes themselves.
  • Add TransactionsImporter trait to handle both parsing and (possibly if the node need it) persistence.
  • Add from_immutable parameter to TransactionParser trait: an optional lower bound, if None it tell the parser to parse everything from the start.

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
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Comments

We simplified some signatures that were using a CardanoDbBeacon but only needed it's immutable_file_number part.

Issue(s)

Relates to #1591

Alenar and others added 21 commits April 4, 2024 12:26
@Alenar
Extract importer code from the signable builder into a method
5c6af1e
@Alenar
Define TransactionImporter trait and do a first naive impl
2682947
@Alenar
Implement and wire a TransactionsImporter in both aggregator and si…
063f722 
…gner
@Alenar
Move TransactionStore trait from common to signer & aggregator
ab75f90 
As they will diverge in the future.
@Alenar
Test actual behaviour in signer TransactionStore
d13db99
@Alenar
Implement naive cache
4207d0c 
If data are stored in db returns it and don't parse/store anything
@Alenar
Transaction importer now handle partial import
3f0278c 
When data are partially stored in the database.
@Alenar
Add a test to check if database order and parsed order are consistent
cced3ac
@Alenar
Split get from db highest beacon & transactions in two
80adb16
@Alenar
Refactor import: retrieve all data from db after updating it
015ff69 
Before it would aggregate result from the not update db and the parser.
@Alenar
Implement get_up_to to signer cardano transactions repository
a56e42b
@Alenar
Implement get_highest_beacon to signer cardano transactions repository
fa449e0
@Alenar
Add rescan offset to signer TransactionsImporter
1fd9475 
This allow to rescan immutables in case of a previous partial scan
likely when this scan failed.
@Alenar
Simplify rescan offset computation & testing
1ceb5b0
@Alenar
Clarify TransactionImporter::import signature
329b3ad 
The beacon parameter wasn't clearly expressing that it was an upper bound.
@Alenar
Don't use ServiceBuilder to build the sqliteconnection in unit tests
b646831
@Alenar
Test the repository 'get tx up to' method instead of the trait implem
580b38b 
Since multiple traits can define it
@Alenar
Synchronise TransactionsImporter change from signer to aggregator
fd345f4
@Alenar
Add log to TransactionsImporter when the database is up to date
eb5c95f
@Alenar
Make transaction repository handle the store in chunk
bf7d1dd 
Since it's a sqlite limitation, it's the repository job to handle that.
@dlachaume
Handle lower bound in Transactions parser
396ed9e
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2024 •
edited

Test Results

    3 files  ± 0     42 suites  ±0   8m 34s ⏱️ -48s
  962 tests +16    962 ✅ +16  0 💤 ±0  0 ❌ ±0 
1 056 runs  +16  1 056 ✅ +16  0 💤 ±0  0 ❌ ±0 

Results for commit ed4d46d. ± Comparison against base commit 336c070.

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-sanchonet April 8, 2024 10:14 — with GitHub Actions Inactive
@Alenar Alenar self-assigned this Apr 8, 2024
@dlachaume
Minor changes after review
baf3e08 
- Fix comments typo
- Rename function name
- Change `from` type to `ImmutableFileNumber` in `parse_and_store_transactions_not_imported_yet` signature
jpraynaud
jpraynaud approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Alenar Alenar merged commit b9e810f into main Apr 9, 2024
41 checks passed
@Alenar Alenar deleted the ensemble/1591/incremental-storage-of-cardano-tx branch April 9, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfauvel sfauvel sfauvel approved these changes

@jpraynaud jpraynaud jpraynaud approved these changes

@dlachaume dlachaume Awaiting requested review from dlachaume

Assignees

@Alenar Alenar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Alenar @sfauvel @jpraynaud @dlachaume