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

ics-cf-solana light-client: header verification & client update; protobuf impl #510

Open
wants to merge 21 commits into
base: hyperspace-solana
Choose a base branch
from

Conversation

vmarkushin
Copy link
Collaborator

@vmarkushin vmarkushin commented Aug 5, 2024

Implemented initial version of the client header verification and state update, and Protobuf (de)serialisation.

The library is based on cf-guest crate and reuses some functionality from there. The verification of Shreds is performed by the functions taken from Solana's codebase. Some code was copy-pasted from the repo due to absence of no-std support, mentioning the original's file source path.

The current version of the code doesn't contain proper tests - they will be added when the client is supported in hyperspace-solana.

@vmarkushin vmarkushin requested a review from mina86 August 5, 2024 18:37
Copy link
Contributor

@mina86 mina86 left a comment

Choose a reason for hiding this comment

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

If I understand correctly, Header contains the entire Solana block (i.e. all shreds)? That can be rather big, no?

light-clients/icsxx-cf-solana/src/client_def.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/client_def.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/client_def.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/header.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/header.rs Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/header.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/header.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/proto/cf-solana.proto Outdated Show resolved Hide resolved
@@ -0,0 +1,263 @@
//! File source: solana/ledger/src/shred/legacy.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m guessing we can get rid of legacy support?

Copy link
Collaborator Author

@vmarkushin vmarkushin Aug 6, 2024

Choose a reason for hiding this comment

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

It's not being use in Solana anymore? Then I think it's crucial to make sure that the encoding is still correct, which seems a bit subtle to me... But I will add a TODO for it

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I don’t know, but that would be my assumption. It’s probably something to verify.

@vmarkushin
Copy link
Collaborator Author

If I understand correctly, Header contains the entire Solana block (i.e. all shreds)? That can be rather big, no?

There is no requirement for that, but also no limit for how many shreds can be sent. I think it's reasonable to limit the amount of shreds only to be able to restore the last entry (to calculate the hash) + (potentially) some meta information, like validators change

Cargo.toml Show resolved Hide resolved
@vmarkushin vmarkushin requested a review from mina86 August 8, 2024 21:02
light-clients/icsxx-cf-solana/src/client.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/client.rs Outdated Show resolved Hide resolved
contracts/pallet-ibc/src/light_clients.rs Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/client.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/client.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/client_def.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/header.rs Outdated Show resolved Hide resolved
light-clients/icsxx-cf-solana/src/header.rs Outdated Show resolved Hide resolved
/// 3. Doesn't contain duplicates
/// 4. All shreds have the same slot
#[derive(Clone, PartialEq, Eq, Debug)]
pub struct PreCheckedShreds(Vec<Shred>);
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d say CheckedShreds is a better name. PreChecked is ambiguous and might mean that those are shreds before the check, i.e. pre being checked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're not fully checked (the signature wasn't checked at least), that's why the name is like so

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you think about PartiallyCheckedShreds?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could also be SortedShreds I suppose?

light-clients/icsxx-cf-solana/src/header.rs Show resolved Hide resolved
@vmarkushin vmarkushin requested a review from mina86 August 12, 2024 18:36
…-client-update

# Conflicts:
#	contracts/pallet-ibc/src/light_clients.rs
#	light-clients/icsxx-cf-solana/src/client_def.rs
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