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

CardanoTxOut representation #951

Merged
merged 3 commits into from
Feb 22, 2024
Merged

CardanoTxOut representation #951

merged 3 commits into from
Feb 22, 2024

Conversation

jorisdral
Copy link
Contributor

@jorisdral jorisdral commented Feb 14, 2024

Description

Reduce heap size of Cardano TxOuts by representing them as a flat sum type, instead of an NS

Comment on lines +307 to +308
--
-- TODO: can this be removed in favour of EncodeDisk and DecodeDisk?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something to think about

Copy link
Contributor

@jasagredo jasagredo Feb 19, 2024

Choose a reason for hiding this comment

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

And probably a good thing to do.

EDIT: actually I'm unsure if we could do this easily. But perhaps can be postponed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's postpone it

Comment on lines +126 to +90
type family Key l -- TODO: rename to TxIn

-- | Each @LedgerState@ instance will have the notion of a @Value@ for the
-- tables. For instance, if we only pulled out only the UTxO set from the ledger
-- state, this type would be @TxOut@ or @NS TxOut@.
type Value :: LedgerStateKind -> Type
type family Value l
type family Value l -- TODO: rename to TxOut
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a very interesting change, but it is entirely separate from what I've been doing in this PR so let's do it in a new PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And actually, is this something we want to change? Are ledgertables UTxO-specific, or would we reuse the same type for other tables like stake-related tables?

Copy link
Contributor

@jasagredo jasagredo Feb 19, 2024

Choose a reason for hiding this comment

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

Pff, this is a question I don't have an answer for. I don't know how multiple tables would work. I guess we will have several Value instances indexed by the kind of table? I don't know

Comment on lines +31 to +32
-- TODO: if we make mapkinds of kind @(k1, k2) -> Type@ instead of @k1 -> k2 ->
-- Type@, then we could reuse most of the @barbies@ machinery.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasagredo thoughts?

Copy link
Contributor

@jasagredo jasagredo Feb 19, 2024

Choose a reason for hiding this comment

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

This sounds very reasonable. I agree with this, but maybe in a follow-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

@jorisdral
Copy link
Contributor Author

ouroboros-consensus-test:consensus-test is timing out in GHA for ghc 8.10.7... weird. See: https://github.com/IntersectMBO/ouroboros-consensus/actions/runs/7899788240/job/21559954609?pr=951

@jorisdral jorisdral marked this pull request as ready for review February 16, 2024 13:44
@jorisdral
Copy link
Contributor Author

GHA is magically passing now

Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

I think in general this looks good

@jasagredo jasagredo merged commit 698126e into utxo-hd-8.2.1 Feb 22, 2024
5 checks passed
@jasagredo jasagredo deleted the jdral/cardano-tx-out branch February 22, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants