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.

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

[light-client-integration] support for light client state signature #879

Merged
merged 30 commits into from
Jan 9, 2024
Merged

[light-client-integration] support for light client state signature #879

merged 30 commits into from
Jan 9, 2024

Conversation

mrain
Copy link
Contributor

@mrain mrain commented Dec 19, 2023 •
edited
Loading

closes: #801

What does this PR do

  • Watches the hotshot event stream, if a LeafDecided event happens, it triggers the light_client_signature_hook to generate a new state signature
  • SystemContextHandle is now wrapped in SequencerContext, where the later includes a light client state signature pool that contains the most recent 100 signatures
  • Adds a endpoint for fetching the light client state signature. It's enabled when http option is available.

jbearer
jbearer reviewed Dec 19, 2023
View reviewed changes
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

Question: is it ok for a node to choose not to run the state signature API? In other words isn't it considered malicious behavior for a node not to respond to this query? I'm wondering if we should make the Http option required and always run at least the state signature API.

.docker-compose.yaml.swp Outdated Show resolved Hide resolved
sequencer/api/state_signature.toml Outdated Show resolved Hide resolved
sequencer/src/api/signature_pool.rs Outdated Show resolved Hide resolved
sequencer/src/enriched_handle.rs Outdated Show resolved Hide resolved
sequencer/src/enriched_handle.rs Outdated Show resolved Hide resolved
sequencer/src/enriched_handle.rs Outdated Show resolved Hide resolved
sequencer/src/api.rs Outdated Show resolved Hide resolved
sequencer/src/api/options.rs Outdated Show resolved Hide resolved
sequencer/src/api/endpoints.rs Outdated Show resolved Hide resolved
sequencer/src/api/endpoints.rs Outdated Show resolved Hide resolved
@mrain
Copy link
Contributor Author

mrain commented Dec 19, 2023

Question: is it ok for a node to choose not to run the state signature API? In other words isn't it considered malicious behavior for a node not to respond to this query? I'm wondering if we should make the Http option required and always run at least the state signature API.

This is worth discussing. IMHO
First, a light client state prover should be able to collect the signatures, at least from all honest nodes. If there's no data(signature) availability guarantee/mechanism, then yes, honest nodes must respond to these queries.
Second, signature collection doesn't have to be through this Http. This is just an option that works, if it's okay to make Http a default option.

cc @jbearer @chancharles92 @alxiong

@philippecamacho
Copy link
Contributor

Do we plan to write tests for this feature in another PR?

philippecamacho
philippecamacho reviewed Dec 22, 2023
View reviewed changes
sequencer/src/state_signature.rs Outdated
let new_light_client_state = LightClientState::<BaseField> {
view_number: leaf.get_view_number().get_u64() as usize,
block_height: leaf.get_height() as usize,
block_comm_root: BaseField::default(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a TODO here as well right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just created 2 new issues. #923, #924

@mrain
Copy link
Contributor Author

mrain commented Dec 22, 2023

Do we plan to write tests for this feature in another PR?

I'm writing tests now. Dealing with more problems :(

@mrain mrain marked this pull request as ready for review December 22, 2023 17:07
jbearer
jbearer reviewed Jan 8, 2024
View reviewed changes
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

This looks good, but are we going to change it to POST signatures to another service, rather than just providing a query API for signatures?

sequencer/src/api.rs Outdated Show resolved Hide resolved
sequencer/src/api.rs Outdated Show resolved Hide resolved
sequencer/src/context.rs Outdated Show resolved Hide resolved
sequencer/src/context.rs
handle: SystemContextHandle<TYPES, I>,

/// Index of this sequencer node
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to use this for something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not from my side, it was one of the returns in the previous init_node

Copy link
Member

Choose a reason for hiding this comment

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

Huh. If we're not using it I guess it's fine to delete it. Not a big deal though

sequencer/src/context.rs Outdated Show resolved Hide resolved
@mrain
Copy link
Contributor Author

mrain commented Jan 8, 2024

This looks good, but are we going to change it to POST signatures to another service, rather than just providing a query API for signatures?

Yes we are. But I think it's better to keep this query API, maybe as a backup in the future.

@mrain mrain mentioned this pull request Jan 8, 2024
Closed
6 tasks
jbearer
jbearer reviewed Jan 8, 2024
View reviewed changes
sequencer/src/context.rs Outdated
@@ -25,13 +30,20 @@ pub struct SequencerContext<TYPES: NodeType, I: NodeImplementation<TYPES>> {
state_signatures: Arc<RwLock<StateSignatureMemStorage>>,
}

impl<TYPES: NodeType, I: NodeImplementation<TYPES>> SequencerContext<TYPES, I> {
impl<N: network::Type> Clone for SequencerContext<N> {
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is implemented manually because the derived implementation requires N: Clone? You can use Derivative to get around this:

#[derive(Derivative)]
#[derivative(Clone(bound = ""))]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Derivative works! ddd5882

sequencer/src/context.rs Outdated Show resolved Hide resolved
jbearer
jbearer approved these changes Jan 8, 2024
View reviewed changes
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

Looks good!

@mrain mrain merged commit f1cd62b into main Jan 9, 2024
9 checks passed
@mrain mrain deleted the cl/light-client branch January 9, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbearer jbearer jbearer approved these changes

@alxiong alxiong Awaiting requested review from alxiong

@nomaxg nomaxg Awaiting requested review from nomaxg

@ImJeremyHe ImJeremyHe Awaiting requested review from ImJeremyHe ImJeremyHe is a code owner

@sveitser sveitser Awaiting requested review from sveitser sveitser is a code owner

@philippecamacho philippecamacho Awaiting requested review from philippecamacho philippecamacho is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[light-client-integration] support to light client state prover
3 participants
@mrain @philippecamacho @jbearer