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

Added support for fetching grandpa justifications for arbitrary blocks #658

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ToufeeqP
Copy link
Contributor

@ToufeeqP ToufeeqP commented Sep 12, 2024

Pull Request type

  • Feature

Description

The support for storing block justifications is gated behind the grandpa-justifications feature. This feature primarily provides two functionalities:

  • A CLI option --grandpa-justification-period, allows users to set the block interval at which the node should store block justifications.
  • A grandpa_blockJustification RPC, enabling users to fetch the justification for a given block number.

Checklist

  • I have performed a self-review of my own code.
  • The tests pass successfully with cargo test.
  • The code was formatted with cargo fmt.
  • The code compiles with no new warnings with cargo build --release and cargo build --release --features runtime-benchmarks.
  • The code has no new warnings when using cargo clippy.
  • If this change affects documented features or needs new documentation, I have created a PR with a documentation update.

cli.kate_rpc_enabled,
cli.kate_rpc_metrics_enabled,
cli,
// cli.unsafe_da_sync,
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code

kate_max_cells_size,
kate_rpc_enabled,
kate_rpc_metrics_enabled,
kate_max_cells_size: cli.kate_max_cells_size,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice if we create a KateDeps here, in the same way of node_rpc::BabeDeps and node_rpc::GrandpaDeps (see some lines avobe).

@fmiguelgarcia
Copy link
Collaborator

fmiguelgarcia commented Sep 13, 2024

After deeper review I have a some of points/questions.

1. The max of grandpa-justification-period (minor)

The justification is enforced to be generated when the authority set is update, so the maximum value would be the epoch here (not i32::MAX). It means that if grandpa-justification-period is greater than epoch then nodes will generate just one justification per epoch.

2. Delays on finalization.

If the majority of the active set of validators increase the grandpa-justification-period, then we will see delays on finalization. As a side-effect, the number of forks of the network could increase, and that would also imply an increment of the memory footprint (and storage) in the node, due to the requirement of having them available until the finalization.

3. Missing Justifications for a block in the RPC.

As justifications work over sub-chains, using the current implementation of the RPC could miss some of them for a given block.
As an example, let's assume we have the following unfinalized chain A->B->C->D , and a 3 validator set. Let's also assume that validator 1 and 2 add justifications to C block, and validator 3 adds a justification to D. If you use the current implementation of the RPC on block B, you will get none. Actually, C is finalized which implies B, so you should get the justifications of C and D when you ask about B.

NOTE: Maybe a wrapper around the grandpa_proveFinality RPC could be better option.

The case is even worst when you get justifications of non-canonical chain, which could be discarded in the future.

In summary, we should review the use case of that RPC, because it could return invalid or uncompleted justification for a given block.

att. @ToufeeqP

@ToufeeqP
Copy link
Contributor Author

  1. The max of grandpa-justification-period (minor)
    The justification is enforced to be generated when the authority set is update, so the maximum value would be the epoch here (not i32::MAX). It means that if grandpa-justification-period is greater than epoch then nodes will generate just one justification per epoch.

The justification that a full node generates in this feature is not linked to justifications that must be generated on the transition block (that's mandatory & all nodes should generate that). This configuration is only for nodes on which we've enabled this.

  1. Delays on finalization.
    If the majority of the active set of validators increase the grandpa-justification-period, then we will see delays on finalization. As a side-effect, the number of forks of the network could increase, and that would also imply an increment of the memory footprint (and storage) in the node, due to the requirement of having them available until the finalization.

We won't be enabling this on validator nodes/ every RPC node for that matter. This is why we have put this feature behind a flag.

  1. Missing Justifications for a block in the RPC.
    As justifications work over sub-chains, using the current implementation of the RPC could miss some of them for a given block.
    As an example, let's assume we have the following unfinalized chain A->B->C->D , and a 3 validator set. Let's also assume that validator 1 and 2 add justifications to C block, and validator 3 adds a justification to D. If you use the current implementation of the RPC on block B, you will get none. Actually, C is finalized which implies B, so you should get the justifications of C and D when you ask about B.

This feature does not guarantee the justification of the set interval. And yes, it's only for specific use cases.

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.

2 participants