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

server/v2/stf should not manage the storage and retrieval of header info #20510

Closed
Tracked by #20439
kocubinski opened this issue May 31, 2024 · 5 comments
Closed
Tracked by #20439
Assignees
Labels
C:server/v2 Issues related to server/v2

Comments

@kocubinski
Copy link
Member

kocubinski commented May 31, 2024

STF is stateless, the current iteration manages storing header info, and is borrowing the consensus key to do so. STF should instead depend on a header service which abstracts storage.

// TODO storing header info is too low level here, stf should be stateless.
// We should have a keeper that does this.
runtimeStore, err := state.GetWriter(appmanager.ConsensusIdentity)

@kocubinski kocubinski changed the title server/v2/stf should not manage the storage and retrieval of header info. server/v2/stf should not manage the storage and retrieval of header info May 31, 2024
@kocubinski kocubinski mentioned this issue May 31, 2024
12 tasks
@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label May 31, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Cosmos-SDK May 31, 2024
@kocubinski kocubinski added C:server/v2 Issues related to server/v2 and removed needs-triage Issue that needs to be triaged labels May 31, 2024
@tac0turtle tac0turtle self-assigned this Jun 3, 2024
@tac0turtle
Copy link
Member

closing this as it was discussed in slack

@github-project-automation github-project-automation bot moved this from 📋 Backlog to 🥳 Done in Cosmos-SDK Jun 4, 2024
@kocubinski
Copy link
Member Author

Still, this is bad design:

// TODO reserved in consensus OR need a keeper here
const headerInfoPrefix = 0x37

// setHeaderInfo sets the header info in the state to be used by queries in the future.
func (s STF[T]) setHeaderInfo(state store.WriterMap, headerInfo header.Info) error {
	// TODO storing header info is too low level here, stf should be stateless.
	// We should have a keeper that does this.
	runtimeStore, err := state.GetWriter(appmanager.ConsensusIdentity)
	if err != nil {
		return err
	}
	bz, err := headerInfo.Bytes()
	if err != nil {
		return err
	}
	err = runtimeStore.Set([]byte{headerInfoPrefix}, bz)
	if err != nil {
		return err
	}
	return nil
}

stf is arbitrarily picking a byte prefix within the consensus merkelized tree (should this even be merkelized?). There could be collisions (since that byte prefix is not reserved) if consensus changes the disk layout of its keys.

@tac0turtle
Copy link
Member

In the past store did this automatically. I'm not sure store supports this out of the box. There was a space in which commit info was stored. We should try to recreate this. Agree this is not a good design. Cc @testinginprod

@tac0turtle tac0turtle reopened this Jun 5, 2024
@github-project-automation github-project-automation bot moved this from 🥳 Done to 📋 Backlog in Cosmos-SDK Jun 5, 2024
@testinginprod
Copy link
Contributor

testinginprod commented Jun 5, 2024

(note this is my opinion)

Why should STF not store it? I do not understand, for instance, HeaderInfo is a first classi citizen of every API, it lives in BlockRequest which is also a core API: https://github.com/cosmos/cosmos-sdk/blob/main/core/app/app.go#L25 (this is just an unstructured header.Info: https://github.com/cosmos/cosmos-sdk/blob/main/core/header/service.go#L17).

So it's not something that can or cannot exist, header info will be always there, no matter how we change modules, or consensus. If this is not a correct assumption then BlockRequest should not contain header information (height, etc).

So who should store this info?

The Consensus Engine holds this info, then how does Consensus Engine plumb this into the execution context?

  • The only solution might be CometBFT passing a ConsensusMsg whose duty is to store the header info, but then every other consensus engine would need to do so because HeaderInfo is a first class citizen of all our APIs. Also which module handles the consensus msg to set the header?
  • Above seems far too complex and unneeded, considering it'd need to be replicated for all cons engines.

Regarding having this Merklized or not:

Modules need historical headers in order to server historical queries.

The "old way" of doing it would also force store to save all the headers versions in state, which is fundamentally the same as having it merklized.

It is also one less exception that store needs to handle, since saving historical queries it's ALWAYS needed there's no reason for storage implementations to specialize around this use-case.

The argument could be: why should STF save into the "Consensus" namespace, with that I agree, we can create a "_stf" namespace which is local to stf itself and cannot be claimed by other actors.

TLDR:

  • create an "stf" namespace.
  • retain the design in which STF is storing headers in state.

@tac0turtle
Copy link
Member

what is the outcome here? should we remove this or keep it?

@tac0turtle tac0turtle moved this from 📋 Backlog to 👀 Waiting / In review in Cosmos-SDK Jun 24, 2024
@github-project-automation github-project-automation bot moved this from 👀 Waiting / In review to 🥳 Done in Cosmos-SDK Jun 25, 2024
@tac0turtle tac0turtle removed this from Cosmos-SDK Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:server/v2 Issues related to server/v2
Projects
None yet
Development

No branches or pull requests

3 participants