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

[DONOTMERGE] move chain config to validated state #1492

Merged
merged 14 commits into from
Jun 17, 2024
Merged

Conversation

imabdulbasit
Copy link
Contributor

@imabdulbasit imabdulbasit commented May 21, 2024

Closes #1475

Move ChainConfig to ValidatedState to allow updates

@@ -54,7 +54,8 @@ impl BlockPayload for Payload<TxTableEntryWord> {
txs: impl IntoIterator<Item = Self::Transaction>,
instance_state: &Self::Instance,
) -> Result<(Self, Self::Metadata), Self::Error> {
let payload = Payload::from_txs(txs, &instance_state.chain_config)?;
// TODO: ????
let payload = Payload::from_txs(txs, &instance_state.genesis_state.chain_config)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sveitser @jbearer This doesn't look right. Do we need to add ValidatedState type to this trait and pass that as the parameter to from_transactions

Copy link
Member

Choose a reason for hiding this comment

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

Ugh yes we do (or equivalent). But the bigger problem is this means the builder will need to be aware of the ValidatedState at each point. This is fundamental because a parameter of block building (max size) is now being made variable.

In theory this is fine, the builder can compute the state by replaying each proposal it sees, but I don't know if it currently has all the information it needs from HotShot to call validate_and_apply_header. We'll need to discuss this with @move47 and @nyospe

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe we do, and I'm not sure it will be trivial to add.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we maintain the state as suggested, we will need a mechanism to recover when we miss a proposal...

Copy link
Member

Choose a reason for hiding this comment

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

That part is built into the sequencer implementation of ValidatedState. validate_and_apply_header will automatically fetch missing state as needed as long as the InstanceState contains a valid peer URL

@jbearer
Copy link
Member

jbearer commented May 22, 2024

It occurs to me that we will need catch up for this as well. The ValidatedState will need to have a ResolvableChainConfig, not a ChainConfig, so that we can implement from_header by just taking the Commitment<ChainConfig> from the header if the full ChainConfig is not available. Then we'll need catchup for validated state:

  • As a heuristic, if the Commitment<ChainConfig> matches the genesis ChainConfig from instance_state, we can just use that. This will work in almost all cases.
  • In the case where our node was out of sync while an upgrade was happening, we might have a ValidatedState with a Commitment<ChainConfig> that doesn't match any complete ChainConfig that is known to us. In this case we would have to fetch the complete ChainConfig from a peer via an endpoint like GET /catchup/:view/:height/chain-config or maybe GET /catchup/chain-config/:commitment

@imabdulbasit
Copy link
Contributor Author

as discussed with @jbearer
The from_transactions function will be async. We will pass an additional parameter, ValidatedState.

ResolvableChainConfig contains either a full ChainConfig or just a commitment.

  • The commitment in ResolvableChainConfig exactly matches the instance's ChainConfig (genesis). In this case, we simply return the instance's ChainConfig.
  • The ResolvableChainConfig contains a full ChainConfig.
  • The commitment from ResolvableChainConfig does not match the node instance's chain configuration. In this case, we need to perform a catchup.

@imabdulbasit imabdulbasit force-pushed the abdul/move-chain-config branch from 4b3d92f to cfd2662 Compare May 28, 2024 12:28
@imabdulbasit imabdulbasit changed the title [DRAFT] move chain config to validated state [DONOTMERGE] move chain config to validated state Jun 3, 2024
@jbearer jbearer merged commit f0cf3ef into main Jun 17, 2024
13 of 15 checks passed
@jbearer jbearer deleted the abdul/move-chain-config branch June 17, 2024 14:12
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.

Move ChainConfig to ValidatedState
3 participants