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

Prune consumed tx_out #1398

Closed
wants to merge 9 commits into from
Closed

Prune consumed tx_out #1398

wants to merge 9 commits into from

Conversation

kderme
Copy link
Contributor

@kderme kderme commented May 1, 2023

Description

This is a prototype for #1397. It doesn't correctly handles rollbacks or migrations, but it allows to benchmark syncing and querying the feauture. It builds on top of #1389

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu (which can be run with scripts/fourmolize.sh
  • Self-reviewed the diff

Migrations

  • The pr causes a breaking change of type a,b or c
  • If there is a breaking change, the pr includes a database migration and/or a fix process for old values, so that upgrade is possible
  • Resyncing and running the migrations provided will result in the same database semantically

If there is a breaking change, especially a big one, please add a justification here. Please elaborate
more what the migration achieves, what it cannot achieve or why a migration is not possible.

@@ -146,6 +146,8 @@ insertBlock syncEnv cblk applyRes firstAfterRollback tookSnapshot = do
Generic.fromBabbageBlock (ioPlutusExtra iopts) (getPrices applyResult) blk
insertEpoch details
lift $ commitOrIndexes withinTwoMin withinHalfHour
when (unBlockNo blkNo `mod` 21600 == 0) $ do
Copy link
Contributor

@Cmdv Cmdv May 1, 2023

Choose a reason for hiding this comment

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

think that's meant to be 2160 not 21600.

I ended up using something like bellow rather than hard coding this value:

getSecurityParam :: SyncEnv -> Word64
getSecurityParam syncEnv =
  case envLedgerEnv syncEnv of
    HasLedger hle -> getSecurityParameter $ leProtocolInfo hle
    NoLedger nle -> getSecurityParameter $ nleProtocolInfo nle          

not sure that would be of use 🤷

Copy link
Contributor Author

@kderme kderme May 2, 2023

Choose a reason for hiding this comment

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

21600, or 10 * securityParameter, is the expected number of blocks per epoch on mainnet, so by doing this periodicaly every 21600 it's done roughly once per epoch.
Initially I wanted to do this pruning at the first block of the epoch, however the block itself doesn't carry this information, it requires the ledger state and we have options that disable it. So this was the easiest check I can make to have a realistic benchmark just by looking at the block itself (no additional state).

Copy link
Contributor

Choose a reason for hiding this comment

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

ah cool, so we can only rollback 2160 but the length of an epoch is normally (10 * 2160) 👍

I thought the securityParameter param was ascertained from the config value inside the shelly-genesis.json file.
In my example would I be wrong in doing like that?

@rdlrt
Copy link
Contributor

rdlrt commented May 8, 2023

Apologies as it's not very clear to me, will the delete 'cascade' the references for tx_out from ma_tx_out as well (as it has a max of 900 1-to-many mapping from tx_out)?

@kderme
Copy link
Contributor Author

kderme commented May 17, 2023

Apologies as it's not very clear to me, will the delete 'cascade' the references for tx_out from ma_tx_out as well (as it has a max of 900 1-to-many mapping from tx_out)?

Indeed this is not done in this pr yet, though it should. I was running benchmarks with disable-multiassets and related flags to see the performance diff related to tx_out.

@rdlrt
Copy link
Contributor

rdlrt commented May 17, 2023

Gotcha - thanks. Just wanted to be sure it's not missed 🙂

@kderme
Copy link
Contributor Author

kderme commented May 30, 2023

Superseded by #1416

@kderme kderme closed this May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants