IntersectMBO / plutus-apps

The Plutus application platform
Apache License 2.0
306 stars 214 forks source link

PLT-214 Refactored the Epoch-StakePoolDelegation indexer to the Marconi interface. #1018

Closed koslambrou closed 1 year ago

koslambrou commented 1 year ago

IMPORTANT NOTE:

Not very performant, but not clear on how to make it better. The use of LedgerState uses lots of memory. Could not fully sync on my machine as I don't have enough memory (32GB). The node by itself uses about 12GB. I have 6GB for other processes. Then, I only managed to sync the indexer at 85% before using all of my memory. I'll try again by removing some of the other processes, and see if I can complete it.

However, I think I have optimized it as much as I can (given the functions from ouroboros-network and cardano-ledger). I believe that memory usage is inevitable if we're going to continue using cardano core functions.

Edit 1:

Removed that used 6GB of different processes, and resynced again. This time, it managed to fully sync in about 24h and max RAM usage was just a bit under 16GB.

Pre-submit checklist:

eyeinsky commented 1 year ago

(Just in case noting a non-code comment from above, maybe it went unnoticed

When stopping and resuming indexing, are the stakepool sizes the same as what dbsync gives?

That is to say, the result of the indexing process after adding the resuming feature should continue matching what dbsync gives. Working on optimizing performance before that is achieved will only make it harder to fix later on.)

koslambrou commented 1 year ago

@eyeinsky Haven't compared it to db-sync. Since we have another story for that, let's do that in a future PR. I won't optimize further this PR. Is that alright?

koslambrou commented 1 year ago

@eyeinsky Applied comments. Approve PR?

koslambrou commented 1 year ago

As there is another PR pending then it would be much better to do one thing from start to finish instead of doing two half way

I mean, these are 2 separate functionality (one handles indexing, and the other is JSON-RPC querying). Makes sense to do them separately, especially for easier code reviewing. We should try to make PRs as small as possible.

My regret is not breaking this one up a bit more, as the logic is a bit complex and error-prone.

Ideas for how to test without needing to run it for the ~20h:

Good point. I'll fully sync Marconi, run some manual tests with a local db-sync on preprod and see if I get the same result on some queries. Then, I'll stop and resume, and redo the manual test.

koslambrou commented 1 year ago

@eyeinsky Ok, I'm definitely not getting the same results as db-sync. However, I'm using the same logic as before to compute the stake pool delegation. Going to dig deeper and even take a look at db-sync's implementation.

db-sync has this notion of slices in their function called genericStakeSlice. Have you looked at this?

koslambrou commented 1 year ago

@eyeinsky Ok, just confirmed that the old implementation is incorrect. My refactoring produces similar result to what we had before. However, the results are different than db-sync. They are the same for early epochs. However, at later epochs (for example epoch 32), I'm getting different stake pool delegations. That's probably what the slices of db-sync are about.

Given that this PR gives similar results to the previous implementation and that it's a refactoring PR which adds resume and rollback, I propose to:

eyeinsky commented 1 year ago

If I remember correctly, the genericStakeSlice is where from I got the idea on how to get the stake map from NewEpochState. But if the results (when stopping and resuming) match with the old indexer then I guess what was already implemented in the old indexer is maintained and there isn't more to test in that regard.

Were you comparing stakepool sizes from mainnet?

koslambrou commented 1 year ago

If I remember correctly, the genericStakeSlice is where from I got the idea on how to get the stake map from NewEpochState. But if the results (when stopping and resuming) match with the old indexer then I guess what was already implemented in the old indexer is maintained and there isn't more to test in that regard.

Yeah, most of the logic comes from there, but there's some stuff about slicing up the delegations which I don't understand, and seem to give different results from db-sync.

However, just noticed I'm not getting 100% the same results as the old implementation. The reason is because I'm getting the stake pool delegations of an epoch from the last LedgerState of that epoch, whereas the old implementation computes the stake pool delegation of an epoch from the first LedgerState of the next epoch. Do you know which one is right?

Were you comparing stakepool sizes from mainnet?

No from preprod. Does that change anything?

eyeinsky commented 1 year ago

However, just noticed I'm not getting 100% the same results as the old implementation. The reason is because I'm getting the stake pool delegations of an epoch from the last LedgerState of that epoch, whereas the old implementation computes the stake pool delegation of an epoch from the first LedgerState of the next epoch. Do you know which one is right?

I took the delegations from the first NewEpochState and the intent was to take the stake delegations that is used to calculate rewards for the previous epoch because by that point these can't change anymore (and this was what dbsync seemed to be doing). For any epoch that I tested the _pstakeSet field from the next epoch seemed to give the correct result (but I didn't test and compare the entire mainnet, so who knows).

But given that the results aren't matching at the moment then maybe using the first NewEpochState from the first block of the next epoch will make them match again?


To do this, using the streaming library works very well because streams work like lists: you can zip one with it's tail and you have a pair of (previousEpochNo, stakeFromNextEpoch).

koslambrou commented 1 year ago

@eyeinsky Alright, changed the implementation to get the sdd from the first LedgerState of an epoch. Getting the same results as the old implementation. Fully synced on preprod, stopped and resumed, and getting same result as the old implementation.

Can you do a last quick PR review?

To do this, using the streaming library ...

Unfortunately, I already tried using the streaming library for that. The issue were rollback handling, as we can't rollback on a previous state of a Stream, unless we kept the LedgerStates in memory with History (which I didn't want). As the indexer itself handled rollbacks, I had to do without streaming.

eyeinsky commented 1 year ago

Unfortunately, I already tried using the streaming library for that. The issue were rollback handling, as we can't rollback on a previous state of a Stream, unless we kept the LedgerStates in memory with History (which I didn't want). As the indexer itself handled rollbacks, I had to do without streaming.

It's not like there won't be any challenges at all when using streaming, you obviously need to solve rollbacks. But after you add a component that does that then at a later point in timeline it's safe to fold ledger state, have only one of it in memory, you store that as occasionally as you want etc. (E.g in Marconi.ChainIndex.Logging we have code that demonstrates how to do something every x seconds.) And also zip the stream with its tail to get (previousEpochNo, currentLedgerState).

koslambrou commented 1 year ago

It's not like there won't be any challenges at all when using streaming, you obviously need to solve rollbacks. But after you add a component that does that then at a later point in timeline it's safe to fold ledger state, have only one of it in memory, you store that as occasionally as you want etc. (E.g in Marconi.ChainIndex.Logging we have code that demonstrates how to do something every x seconds.) And also zip the stream with its tail to get (previousEpochNo, currentLedgerState).

The point of this PR is to process the LedgerState as soon as possible. I don't want to simply put the LedgerState on stream once it can't be rollbacked anymore. I wanto to process the LedgerState as soon as it's available. The logging module is a bit different as it aggregated the number of number that have occured and keep the result in a TVar. Not what I want to do.

Unless I'm misunderstanding, seems like a complex implementation and out of scope of this PR. If you think that using streaming would totality simplify the implementation, then write a detailed story about it so that we can analyse it.

eyeinsky commented 1 year ago

The point of this PR is to process the LedgerState as soon as possible. I don't want to simply put the LedgerState on stream once it can't be rollbacked anymore. I wanto to process the LedgerState as soon as it's available. The logging module is a bit different as it aggregated the number of number that have occured and keep the result in a TVar. Not what I want to do.

Unless I'm misunderstanding, seems like a complex implementation and out of scope of this PR. If you think that using streaming would totality simplify the implementation, then write a detailed story about it so that we can analyse it.

Right, but we are maybe not getting the fresh ledger state anyway because we are blocked by writing it to disk.

For things in and out of scope, just adding a writeLedgerState somewhere in the foldLedgerState would have already given us working resume capability. And the rest may have not been required, as we don't use much of the indexer framework anyway, buffer is set to 1, and we are possibly not hitting any rollbacks either, as we're writing ledger sate on every volatile block and doing those writes will stall us).

koslambrou commented 1 year ago

For things in and out of scope, just adding a writeLedgerState somewhere in the foldLedgerState would have already given us working resume capability. And the rest may have not been required, as we don't use much of the indexer framework anyway, buffer is set to 1, and we are possibly not hitting any rollbacks either, as we're writing ledger sate on every volatile block and doing those writes will stall us).

The point of this was to use the indexer framework. Might not be useful right now, but having all indexers be consistent was important. Moreover, we probably won't use LedgerState in the near future for computation that, so that implementation might change and using the Storable interface might be more useful.

I'd like to merge this ASAP. We can always rework it in the future.

koslambrou commented 1 year ago

I would break the pieces of code-changes as small as possible and merge things that (to the best of our knowledge) work, so attempting to fix resume and then refactoring the indexer to use the framework could have been better done in two steps, and perhaps using the convert-to-framework step to also evolve the framework at the same time.

Yeah... sorry about that. Totally agree.