Skip to content

LSM trees initial integration #1572

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
Merged
merged 5 commits into from
Oct 9, 2025
Merged

LSM trees initial integration #1572

merged 5 commits into from
Oct 9, 2025

Conversation

@jasagredo
Copy link
Contributor

@jasagredo jasagredo commented Jun 26, 2025 •
edited
Loading

Description

Integrate the LSM trees as a new backend for the LedgerDB.

@jasagredo jasagredo self-assigned this Jun 26, 2025
@jasagredo jasagredo moved this to 🏗 In progress in Consensus Team Backlog Jun 26, 2025
@geo2a geo2a mentioned this pull request Jun 30, 2025
Closed
4 tasks
jasagredo
jasagredo commented Jul 16, 2025
View reviewed changes
@jasagredo jasagredo changed the base branch from main to coot/cardano-diffusion-integration July 17, 2025 10:30
@jasagredo jasagredo force-pushed the js/lsm1 branch 6 times, most recently from 4eaa608 to 97d30af Compare July 17, 2025 13:54
geo2a
geo2a reviewed Jul 17, 2025
View reviewed changes
@jasagredo jasagredo marked this pull request as ready for review July 17, 2025 15:52
@coot coot force-pushed the coot/cardano-diffusion-integration branch from 37619a9 to d62dc42 Compare July 18, 2025 09:23
@jasagredo jasagredo changed the base branch from coot/cardano-diffusion-integration to main July 21, 2025 09:53
@jasagredo jasagredo force-pushed the js/lsm1 branch 2 times, most recently from dad593f to 26e6455 Compare July 21, 2025 11:43
geo2a
geo2a reviewed Jul 21, 2025
View reviewed changes
@jasagredo jasagredo mentioned this pull request Jul 22, 2025
Open
@jasagredo jasagredo force-pushed the js/lsm1 branch 2 times, most recently from 12a1acb to 4119afa Compare July 29, 2025 10:55
@jasagredo jasagredo mentioned this pull request Sep 4, 2025
Open
@jasagredo jasagredo force-pushed the js/lsm1 branch 3 times, most recently from 4ccdfb9 to 30c149e Compare September 5, 2025 09:56
dnadales
dnadales approved these changes Sep 29, 2025
View reviewed changes
@jasagredo jasagredo mentioned this pull request Oct 1, 2025
Merged
@jasagredo jasagredo force-pushed the js/lsm1 branch 2 times, most recently from 81c8c09 to 074e656 Compare October 2, 2025 10:30
jasagredo and others added 4 commits October 2, 2025 16:21
@jasagredo
Implement LSM-trees based ledger tables handles in LedgerDB V2
9ec9127
@jasagredo
Update to mempack-0.2
9da3c08
@jasagredo
Update Ledger dependencies
4af1ec6
@jasagredo @hamishmack
Fix cabal-docspec in Hydra
a0d614e 
An extra version of `text` is in the plan.json as a dependency of a
`custom-setup` or `build-tool-depends`. Using `exactDeps = true` prevent those
from getting into the plan.json.

Co-authored-by: Hamish Mackenzie <Hamish.K.Mackenzie@gmail.com>
bladyjoker
bladyjoker reviewed Oct 7, 2025
View reviewed changes
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2/LSM.hs
Comment on lines +115 to +120
error $
unlines
[ "There was an error deserializing a TxOut from the LSM backend."
, "This will likely result in a restart-crash loop."
, "The error: " <> show err
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not thrilled to see error here. Still looking to understand where the fromTxoutBytes is used from perhaps it's possible to gracefully handle the error condition without calling error?

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2/LSM.hs
Copy link
Contributor

Choose a reason for hiding this comment

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

Looked at from fromTxInBytes and fromTxOutBytes and error calls therein. Couple of questions:

  1. Should we introduce module level Error type and handle such conditions gracefully?
data Error = TxOutDecodingError SomeError | TxInDecodingError SomeError | InternalError Text

Even though we're calling from functions in IOLike m, it would be very useful to have structured error types such that we can eventually also provide machine readable errors in agreed upon format etc.

  1. What's our take on error generally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general we don't like error but for unrecoverable situations like this one it is somewhat OK. We cannot handle such an error in any meaningful way so we just have to crash. Having a module level Error type would help us with readability of the error though...

@jasagredo jasagredo added this pull request to the merge queue Oct 9, 2025
Merged via the queue into main with commit 31a1bef Oct 9, 2025
17 checks passed
@jasagredo jasagredo deleted the js/lsm1 branch October 9, 2025 14:33
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Consensus Team Backlog Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@geo2a geo2a geo2a left review comments

@amesgen amesgen amesgen left review comments

@jorisdral jorisdral jorisdral left review comments

@fraser-iohk fraser-iohk fraser-iohk left review comments

@dnadales dnadales dnadales approved these changes

@bladyjoker bladyjoker bladyjoker approved these changes

@nfrisby nfrisby Awaiting requested review from nfrisby nfrisby is a code owner

Assignees

@jasagredo jasagredo

Labels

None yet

Projects

Consensus Team Backlog
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jasagredo @jorisdral @dnadales @bladyjoker @geo2a @amesgen @fraser-iohk