Skip to content

Conversation

@kderme
Copy link
Contributor

@kderme kderme commented Mar 19, 2022

This removes the stake distribution from the volatile data and introduces a new way to extract slices of the stake distribution from the ledger state. The size of slices is 2000 and the start index is computed by 2000 * (height of block in epoch).

In the same way that consensus extends the ledger state and maintains the ExtLedgerState we extend it even more, in order to maintain the epoch height. The ext^2 state is stored to disk, which means there are no volatile data. I think the idea to extend the ledger state/snapshot with additional information can be useful in other places. We must be careful though, since it breaks compatibility with previous db-sync versions (it needs a resync from genesis).

Additionally, this gives us the opportunity to add a new fields epoch_height to the block table. db-sync doesn't really depend on this field, but it can be good to have.

This also implements IntersectMBO/cardano-ledger#2657 or a slight variation

Marking this as draft since I'm still syncing from genesis to test it.

@erikd
Copy link
Contributor

erikd commented Mar 20, 2022

Additionally, this gives us the opportunity to add a new fields epoch_height to the block table. db-sync doesn't really depend on this field, but it can be good to have.

What is epoch_height ? Is that not the same as epoch_no ?

epochNo Word64 Maybe sqltype=word31type
slotNo Word64 Maybe sqltype=word63type
epochSlotNo Word64 Maybe sqltype=word31type
epochHeight Word64 Maybe sqltype=word31type
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are to add this, we will need a documentation entry down below in this file.

@kderme
Copy link
Contributor Author

kderme commented Mar 20, 2022

What is epoch_height ? Is that not the same as epoch_no ?

This is the height of the block in its epoch.

@kderme kderme force-pushed the kderme/improve-stake-distr branch from 2a81508 to 1d852b4 Compare March 20, 2022 16:41
@kderme
Copy link
Contributor Author

kderme commented Mar 20, 2022

Added comments and fixed hlint. I have synced mainnet up to epoch 247 without issues. The epoch_stake is same as the released snapshots and the epoch_height seems correct.

@kderme kderme marked this pull request as ready for review March 20, 2022 16:43
@kderme kderme force-pushed the kderme/improve-stake-distr branch 2 times, most recently from c157e8a to 632020b Compare March 20, 2022 17:15
@erikd
Copy link
Contributor

erikd commented Mar 20, 2022

This is the height of the block in its epoch.

We already have something like this, its epochSlotNo. If you want to add a block No within the epoch, it should be named epochBlockNo to match epochSlotNo.

@kderme kderme force-pushed the kderme/improve-stake-distr branch 2 times, most recently from b74ef0e to 26ced03 Compare March 20, 2022 20:07
@erikd
Copy link
Contributor

erikd commented Mar 20, 2022

Also need to ask, what do we need this epochBlockNo / epochHeight for?

@kderme
Copy link
Contributor Author

kderme commented Mar 20, 2022

This epochBlockNo gives us the index of the slice to get from the ledger state. We don't need it in the block table. But since I had to compute it for each block, I added it in the db as well.

@erikd
Copy link
Contributor

erikd commented Mar 21, 2022

If its not actually needed, I wonder if its worth adding to the db. The db is already huge and is growing incredibly fast.

applyToEpochHeight _ _ EBBHeight = Height 0

stakeSliceSize :: Word64
stakeSliceSize = 2000
Copy link
Contributor

@rdlrt rdlrt Mar 21, 2022

Choose a reason for hiding this comment

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

Sorry for dropping in, but is there potential for this value to be configurable by users - via config or startup arg?

Would be good to be able to tune stake chunks from consumer pov to be larger/smaller depending on instance performance and actual amount of stake inserts by end user (maybe as advanced option)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure making this configurable is such a good idea. Its too easy for a user the configure it to a value that causes problems, but the problems will not be easily related to this value. There is also no advantage for to either making it slower or faster.

Copy link
Contributor Author

@kderme kderme Mar 22, 2022

Choose a reason for hiding this comment

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

Values less than 100 on mainnet could cause issues, since with ~21000 blocks per epoch, this gives us max 2100000 delegations. Maybe it's even worth having a sanity check that this value is bigger than size_of_delegations / expected_blocks or it could be adjusted per epoch to a big enough value if that's not the case.

Copy link
Contributor

@rdlrt rdlrt Mar 22, 2022

Choose a reason for hiding this comment

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

Right, my primary concern is that other than mainnet and testnet, there are also small networks run by community (guild network is a small one with 3600 slots/~180 blocks per epoch, and some run short-lived networks destroyed later for their own tests too) that have very low amount of blocks per epoch, but can have:

  • very low amount of stake initially , as network isn't used by many
  • very high amount of stakes as part of burst tests

If we think value of 2000 is safe enough even for small networks, we might be OK.
I understand that it could be misused, but if above becomes a concern - maybe it only be exposed as an advanced option , that not every dbsync user should touch?

Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to parameterize this value on expected blocks per epoch, but it does not make sense to expose this to users.

@kderme It would be good to think about the issue @rdlrt raises and also think about some of the testnets we run which have a very low quality factor (ie large numbers of missing blocks).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok changed this. The slice size is now max 2000 defaultEpochSliceSize. defaultEpochSliceSize is a value big enough to cover everything even if only 20% of the expected blocks are produced in the epoch.

So we could even make 2000 configurable, since if the user provides a very small value it will be orewriten. But I'd leave this for another pr.

@kderme
Copy link
Contributor Author

kderme commented Mar 22, 2022

If its not actually needed, I wonder if its worth adding to the db. The db is already huge and is growing incredibly fast.

Don't really have a strong opinion on this. Other tables are much more populated than block, but if this fields seems useless, I can remove it.

@erikd
Copy link
Contributor

erikd commented Mar 22, 2022

Yes, please remove it.

Also worthwhile seeing if the current hard coded number works correctly on some of the QA test nets (there are 2 from memory, both with pretty sparse block production). Also is it possible to have a test for this, both with sparse and dense block production?

@kderme kderme force-pushed the kderme/improve-stake-distr branch from 26ced03 to fb212bf Compare March 23, 2022 21:23
@kderme kderme force-pushed the kderme/improve-stake-distr branch 2 times, most recently from d07b9ef to 07e6463 Compare March 25, 2022 14:30
@kderme kderme force-pushed the kderme/improve-stake-distr branch from 07e6463 to af9c70e Compare March 25, 2022 15:29
@kderme kderme mentioned this pull request Apr 6, 2022
@erikd
Copy link
Contributor

erikd commented Apr 28, 2022

This was merged in PR #1094 .

@erikd erikd closed this Apr 28, 2022
@kderme kderme deleted the kderme/improve-stake-distr branch July 4, 2022 13:05
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.

4 participants