Optmize GetLatestStateHeight calls

[lock9]

Is your feature request related to a problem? Please describe.

We are deploying our infrastructure using Neo Go. We noticed that the historic endpoints consume a lot of CPU resources. A few calls to historic endpoints are enough to 'break' the server. Maybe it's a configuration issue, but we have requests that stay over five minutes waiting for Neo Go.

The culprit seems to be the GetLatestStateHeight function:

neo-go/pkg/core/stateroot/module.go

Line 134 in 0d8c751

// GetLatestStateHeight returns the latest blockchain height by the given stateroot.

// GetLatestStateHeight returns the latest blockchain height by the given stateroot.
func (s *Module) GetLatestStateHeight(root util.Uint256) (uint32, error) {
	rootBytes := root.BytesBE()
	rootStartOffset := 1 + 4 // stateroot version (1 byte) + stateroot index (4 bytes)
	rootEndOffset := rootStartOffset + util.Uint256Size
	var (
		h       uint32
		found   bool
		rootKey = makeStateRootKey(s.localHeight.Load())
	)
	s.Store.Seek(storage.SeekRange{
		Prefix:    []byte{rootKey[0]}, // DataMPTAux
		Start:     rootKey[1:],        // Start is a value that should be appended to the Prefix
		Backwards: true,
	}, func(k, v []byte) bool {
		if len(k) == 5 && bytes.Equal(v[rootStartOffset:rootEndOffset], rootBytes) {
			h = binary.BigEndian.Uint32(k[1:]) // cut prefix DataMPTAux
			found = true
			return false
		}
		return true
	})
	if found {
		return h, nil
	}
	return h, storage.ErrKeyNotFound
}

Describe the solution you'd like

Create a prefix to map roots to heights, getting the roots directly, without using 'seek'. Adding this new information has a few downsides:

• One extra write
• Around ~41 extra bytes per block (prefix + root + height) = 1 + 32 + 8 (or 4?)

However, this would significantly speed up historical requests and prevent excessive CPU consumption. In practice, CPU usage became so intense that we struggled to connect to our VMs. IMHO, the performance improvement outweighs the minor increase in storage usage.

EDIT: Sorry for the confusion, I'm not sure if it's either height -> root or root -> height

Describe alternatives you've considered

I've tried a 'read cache', but that doesn't solve the issue. We also have an indexer being built.

Additional context

I implemented this feature in a local branch. It does improve the performance, but it's not fully tested. Maybe it doesn't work.

Don't forget to add labels!

• engine
• enhancement

[roman-khimov]

How exactly are you doing these calls? The only case where GetLatestStateHeight is invoked at all is when you're using state hash as parameter. But in fact you can use block index or block hash and avoid this function being called completely. In the vast majority of cases block indexes or hashes are more available to users than state root hashes.

[lock9]

We use it to get historical balances (most times). In fact, what you said is very relevant, becaus it was a bug in the frontend that was sending incorrect parameters (root hash). This was making code fall into the GetLatestStateHeight branch. This may explain why caching wasn't working as expected.

The 'problem' with this code, is that a few requests is enough to consume all the CPU available. Are these extra branches necessary? (accept the height only instead of the root)

I know that the current code was querying for the state root, and then making the historic call. If I understand correctly, the first step is skippable.

func (s *Server) getHistoricParams(reqParams params.Params) (uint32, *neorpc.Error) {
	if s.chain.GetConfig().Ledger.KeepOnlyLatestState {
		return 0, neorpc.WrapErrorWithData(neorpc.ErrUnsupportedState, fmt.Sprintf("only latest state is supported: %s", errKeepOnlyLatestState))
	}
	if len(reqParams) < 1 {
		return 0, neorpc.ErrInvalidParams
	}
	height, respErr := s.blockHeightFromParam(reqParams.Value(0))
	if respErr != nil {
		hash, err := reqParams.Value(0).GetUint256()
		if err != nil {
			return 0, neorpc.NewInvalidParamsError(fmt.Sprintf("invalid block hash or index or stateroot hash: %s", err))
		}
		b, err := s.chain.GetBlock(hash)
		if err != nil {
			stateH, err := s.chain.GetStateModule().GetLatestStateHeight(hash) <----- HERE
			if err != nil {
				return 0, neorpc.NewInvalidParamsError(fmt.Sprintf("unknown block or stateroot: %s", err))
			}
			height = stateH
		} else {
			height = b.Index
		}
	}
	return height + 1, nil
}

[roman-khimov]

Are these extra branches necessary?

Yes because we always wanted to give more options to users. Therefore we support block numbers, block hashes and state root hashes as parameters for historic calls.

Now if that's really problematic wrt resource use we may think of omitting state root hash support, although it's somewhat natural for state-related queries.

I know that the current code was querying for the state root, and then making the historic call. If I understand correctly, the first step is skippable.

That's exactly what I'm trying to convey, in many cases you don't need state root hash itself.

[lock9]

If sending an inexistent hash causes the node to read 'all the storage', it must either be optimized or disabled. The first option seems more reasonable since removing it may cause applications to break.

Do you think it's possible to optimize it without consuming more storage or using an extra write? Maybe using an in-memory bloom filter? It won't solve the issue 'permanently', but it could prevent the node from being stuck on all requests

[roman-khimov]

I doubt it can be solved without DB scheme change.