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

Do not HTR the state when checking for optimistic mode #12143

Merged

potuz merged 35 commits into develop from is-optimistic-htr

Mar 20, 2023

Contributor

rkapka commented Mar 16, 2023 •

edited

Loading

What type of PR is this?

Optimization

What does this PR do? Why is it needed?

Several users complained about out-of-memory issues happening when issuing API requests. This is most likely because of how we check for optimistic status. Currently we calculate the state root every time inside IsOptimistic, which is very inefficient. In tghis PR we move to an algorithm which does not calculate HTR on the state. The algorithm was designed by @potuz


          initial impl

de952c9

rkapka changed the title ~~initial impl~~ Do not HTR the state when checking for optimistic mode

rkapka commented

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              				return false, errors.Wrapf(err, "could not get block roots for slot %d", slotNumber)
+              			}
+              			if len(roots) == 0 {
+              				// TODO: What to do here?

Contributor Author

rkapka Mar 16, 2023

Not sure what to do in this case

Contributor

potuz Mar 17, 2023

my suggestion above removes this problem

nisdas reviewed

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              	if st.Slot() == headSt.Slot() {
+              		return optimisticModeFetcher.IsOptimistic(ctx)
+              	}
+              	r, err := st.LatestBlockHeader().HashTreeRoot()

Member

nisdas Mar 17, 2023

doesn't work this way for the reasons we have discussed earlier.

Contributor

potuz Mar 17, 2023

you do not need to do this HTR. All you care is if the block is canonical or not. Here the cheapest thing to do is to pass database and use database.BlockRootsBySlot or BlocksBySlot

potuz requested changes

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              			if !optimistic {
+              				return false, nil
+              			}
+              			headSt, err := stateFetcher.State(ctx, []byte("head"))

Contributor

potuz Mar 17, 2023

please use chainInfo.HeadSlot()

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              			if primitives.Slot(slotNumber) == headSt.Slot() {
+              				return optimisticModeFetcher.IsOptimistic(ctx)
+              			}
+              			finalizedSlot, err := slots.EpochStart(headSt.FinalizedCheckpoint().Epoch)

Contributor

potuz Mar 17, 2023

please use chainInfo.FinalizedCheckpt() and move this check before the head check above.

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              			if primitives.Slot(slotNumber) <= finalizedSlot {
+              				return false, nil
+              			}
+              			_, roots, err := database.BlockRootsBySlot(ctx, primitives.Slot(slotNumber))

Contributor

potuz Mar 17, 2023

You can avoid this database lookup in this case by using chainInfo.ForkChoicer().AncestorRoot(ctx, headRoot, slot) and then checking for optimistic status of that root. Be careful with the forkchoice locks.

Contributor Author

rkapka Mar 17, 2023

Be careful with the forkchoice locks.

Do I need to explicitly do anything?

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              				return false, errors.Wrapf(err, "could not get block roots for slot %d", slotNumber)
+              			}
+              			if len(roots) == 0 {
+              				// TODO: What to do here?

Contributor

potuz Mar 17, 2023

my suggestion above removes this problem

beacon-chain/rpc/eth/helpers/sync.go Outdated

Comment on lines 127 to 137

+              			for _, r := range roots {
+              				canonical, err := chainInfo.IsCanonical(ctx, r)
+              				if err != nil {
+              					return false, errors.Wrapf(err, "could not check canonical status")
+              				}
+              				if canonical {
+              					return optimisticModeFetcher.IsOptimisticForRoot(ctx, r)
+              				}
+              			}
+              			// No block is canonical, return true
+              			return true, nil

Contributor

potuz Mar 17, 2023

My suggestion above removes this block.

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              ) (bool, error) {
+              	st, err := stateFetcher.State(ctx, stateId)
+              	if err != nil {
+              		return false, errors.Wrap(err, "could not fetch justified state")

Contributor

potuz Mar 17, 2023

Why this error message?

Contributor Author

rkapka Mar 17, 2023

I forgot to change this when extracting the function

beacon-chain/rpc/eth/helpers/sync.go Outdated

Comment on lines 153 to 159

+              	headSt, err := stateFetcher.State(ctx, []byte("head"))
+              	if err != nil {
+              		return false, errors.Wrap(err, "could not fetch head state")
+              	}
+              	if st.Slot() == headSt.Slot() {
+              		return optimisticModeFetcher.IsOptimistic(ctx)
+              	}

Contributor

potuz Mar 17, 2023

you are grabbing a state only to check that the incoming id has the same state slot. You can use if you want chainInfo.HeadSlot() but also you should move this check (which is very unlikely) down the path.

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              	if st.Slot() == headSt.Slot() {
+              		return optimisticModeFetcher.IsOptimistic(ctx)
+              	}
+              	r, err := st.LatestBlockHeader().HashTreeRoot()

Contributor

potuz Mar 17, 2023

you do not need to do this HTR. All you care is if the block is canonical or not. Here the cheapest thing to do is to pass database and use database.BlockRootsBySlot or BlocksBySlot


          review feedback

b4af356

rkapka commented

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              	}
+              	if len(roots) == 0 {
+              		// TODO: What to do here?
+              		return false, errors.New("no block")

Contributor Author

rkapka Mar 17, 2023

We now have this issue here @potuz

rkapka marked this pull request as ready for review

March 17, 2023 10:39

rkapka requested a review from a team as a code owner

March 17, 2023 10:39

rkapka requested review from prestonvanloon, saolyn and nisdas

March 17, 2023 10:39


          fix tests

22b4427

potuz requested changes

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              			if err != nil {
+              				return false, errors.Wrap(err, "could not get head state's finalized slot")
+              			}
+              			if primitives.Slot(slotNumber) <= finalizedSlot {

Contributor

potuz Mar 17, 2023

Please move this check above the head one.

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              			}
+              			r, err := chainInfo.ForkChoicer().AncestorRoot(ctx, bytesutil.ToBytes32(stateId), primitives.Slot(slotNumber))
+              			if err != nil {
+              				return false, errors.Wrap(err, "could not get ancestor root")

Contributor

potuz Mar 17, 2023

on errors is probably better to return true instead of false.

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              			if primitives.Slot(slotNumber) <= finalizedSlot {
+              				return false, nil
+              			}
+              			r, err := chainInfo.ForkChoicer().AncestorRoot(ctx, bytesutil.ToBytes32(stateId), primitives.Slot(slotNumber))

Contributor

potuz Mar 17, 2023

how is stateId a block root here? you need to pass the headroot here.


          review feedback

fd5da7f

rkapka force-pushed the is-optimistic-htr branch from 86a639c to fd5da7f Compare

March 17, 2023 15:39

rkapka added 4 commits

March 17, 2023 16:48


          Merge branch 'develop' into is-optimistic-htr

32d3506


          some improvements

af3798b


          tests and small improvements

bf2245a

gzl

adb6333

potuz requested changes

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go Outdated

+              		return true, errors.Wrapf(err, "could not get block roots for slot %d", st.Slot())
+              	}
+              	if len(roots) == 0 {
+              		return false, errors.New("no blocks returned from the database")

Contributor

potuz Mar 17, 2023

better to return true here

beacon-chain/rpc/eth/helpers/sync.go Outdated

Comment on lines 152 to 173

+              	for _, r := range roots {
+              		canonical, err := chainInfo.IsCanonical(ctx, r)
+              		if err != nil {
+              			return true, errors.Wrapf(err, "could not check canonical status")
+              		}
+              		if canonical {
+              			optimistic, err := optimisticModeFetcher.IsOptimistic(ctx)
+              			if err != nil {
+              				return true, errors.Wrap(err, "could not check optimistic status")
+              			}
+              			if !optimistic {
+              				return false, nil
+              			}
+              			finalizedSlot, err := slots.EpochStart(chainInfo.FinalizedCheckpt().Epoch)
+              			if err != nil {
+              				return true, errors.Wrap(err, "could not get head state's finalized slot")
+              			}
+              			if st.Slot() <= finalizedSlot {
+              				return false, nil
+              			}
+              			return true, nil
+              		}

Contributor

potuz Mar 17, 2023

This block does not check what you want. Suppose you get roots = [r1, r2] with r1 canonical and r2 not, both past the finalized version. The node is not optimistic. In this situation you would return false.

Now suppose that the block with root r2 has stateRoot precisely the state you queried for. In this case you should have returned true.

What you need to do here is to requests the blocks, not the roots. And then check if the stateRoot matches the given id, and in that case (only in that case) check if it is canonical or not, etc...

rkapka force-pushed the is-optimistic-htr branch from ff3a8bd to c6c866f Compare

March 17, 2023 18:20


          one more review

1a4b3ef

rkapka force-pushed the is-optimistic-htr branch from c6c866f to 1a4b3ef Compare

March 17, 2023 18:22

potuz added 6 commits

March 17, 2023 15:42


          fix test

85570af


          fix other test

d453acc


          get the roots instead of hashing them

2387d9a


          fix comment


          fix justified case

08d5109


          fix all tests

1587a3d


          Fix is_not_finalized_when_head_is_optimistic but reviewing the logic …

b0b7776

…first

potuz previously approved these changes

View reviewed changes


          Fix is_not_finalized_when_head_is_optimistic

f83de8c

terencechain dismissed potuz’s stale review via

f83de8c

March 18, 2023 16:26

rkapka force-pushed the is-optimistic-htr branch from 9527a83 to 289d164 Compare

March 18, 2023 16:49


          better root tests

1090deb

rkapka force-pushed the is-optimistic-htr branch from 289d164 to 1090deb Compare

March 18, 2023 16:50

terencechain reviewed

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go

Comment on lines +90 to +96

+              			optimistic, err := optimisticModeFetcher.IsOptimistic(ctx)
+              			if err != nil {
+              				return true, errors.Wrap(err, "could not check optimistic status")
+              			}
+              			if !optimistic {
+              				return false, nil
+              			}

Member

terencechain Mar 18, 2023

Why not move this to before strconv.ParseUint(stateIdString, 10, 64)?

Member

terencechain Mar 18, 2023

Very smart check btw

beacon-chain/rpc/eth/helpers/sync.go Outdated

Comment on lines 116 to 119

+              			r, err := chainInfo.ForkChoicer().AncestorRoot(ctx, bytesutil.ToBytes32(headRoot), primitives.Slot(slotNumber))
+              			if err != nil {
+              				return true, errors.Wrap(err, "could not get ancestor root")
+              			}

Member

terencechain Mar 18, 2023

This is very smart

Contributor Author

rkapka Mar 18, 2023

This is Potuz's idea

terencechain reviewed

View reviewed changes

beacon-chain/rpc/eth/helpers/sync.go

+              	case "head":
+              		return optimisticModeFetcher.IsOptimistic(ctx)
+              	case "genesis", "finalized":
+              		return false, nil

Member

terencechain Mar 18, 2023

Why wouldn't we check finalized?

fcp := chainInfo.FinalizedJustifiedCheckpoint()
		if fcp == nil {
			return true, errors.New("received nil finalized checkpoint")
		}
		return optimisticModeFetcher.IsOptimisticForRoot(ctx, bytesutil.ToBytes32(fcp.Root))

Contributor Author

rkapka Mar 18, 2023 •

edited

Loading

finalized block (which corresponds to finalized state) can't be optimistic, as said on Slack


          move optimistic check before parsing root

f6fd393

terencechain previously approved these changes

View reviewed changes


          check for last validated checkpoint

a0feab7

potuz dismissed terencechain’s stale review via

a0feab7

March 18, 2023 18:04

potuz and others added 5 commits

March 18, 2023 16:29


          add right check for finalized

6355c69


          fix finalized tests

63d3bc9


          Merge branch 'develop' into is-optimistic-htr

316edd6


          removed impossible condition

a63fde4


          fix TestGetSyncCommitteeDuties

7ffae21

rkapka added the Blocked label

Contributor Author

rkapka commented Mar 20, 2023

Blocked by #12162

potuz added 2 commits

March 20, 2023 15:53


          Merge remote-tracking branch 'origin/develop' into is-optimistic-htr

d51958d


          Use Ancestor from chaininfo

b4cfa55

terencechain previously approved these changes

View reviewed changes

potuz removed the Blocked label


          fix test

potuz dismissed terencechain’s stale review via

March 20, 2023 19:20

terencechain approved these changes

View reviewed changes


          Merge branch 'develop' into is-optimistic-htr

c8c47bf

potuz merged commit 0cb46eb into develop

delete-merged-branch bot deleted the is-optimistic-htr branch

March 20, 2023 20:04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

terencechain terencechain approved these changes

potuz potuz left review comments

prestonvanloon Awaiting requested review from prestonvanloon prestonvanloon is a code owner automatically assigned from prysmaticlabs/core-team

saolyn Awaiting requested review from saolyn saolyn is a code owner automatically assigned from prysmaticlabs/core-team

nisdas Awaiting requested review from nisdas nisdas is a code owner automatically assigned from prysmaticlabs/core-team

Labels

API Ready For Review