-
Notifications
You must be signed in to change notification settings - Fork 538
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
feat: native light client state verification #630
base: main
Are you sure you want to change the base?
feat: native light client state verification #630
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments and questions.
genesis_root, | ||
forks, | ||
execution_state_proof, | ||
} = serde_cbor::from_slice(&inputs).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to handle this unwrap()
into a new Error
value.
) | ||
.is_ok(); | ||
|
||
apply_update(&mut store, update); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will apply the update
even if it was invalid.
Do we want this behavior? Should not we raise up an error in that case (short-circuit)?
&forks, | ||
) | ||
.is_ok(); | ||
apply_finality_update(&mut store, &finality_update); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same questions here about applying invalid update.
let execution_state_branch_nodes: Vec<Node> = execution_state_proof | ||
.execution_state_branch | ||
.iter() | ||
.map(|b| Node::try_from(b.as_ref()).unwrap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about unwrap
to Error
|
||
is_valid = is_valid | ||
&& is_valid_merkle_branch( | ||
&Node::try_from(execution_state_proof.execution_state_root.as_ref()).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about unwrap
to Error
.
let finalized_header_root: [u8; 32] = store | ||
.finalized_header | ||
.hash_tree_root() | ||
.unwrap().as_ref().try_into().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about unwrap
.
finalized_header_root: H256::from(finalized_header_root), | ||
execution_state_root: H256::from(execution_state_root), | ||
finalized_slot: store.finalized_header.slot.as_u64(), | ||
participation: store.current_max_active_participants.try_into().unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about unwrap
let current_sync_committee_hash: U256 = store | ||
.current_sync_committee | ||
.hash_tree_root() | ||
.unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about unwrap
.
if let Some(mut next_sync_committee) = store.next_sync_committee { | ||
let next_period = period + 1; | ||
let stored_next_sync_committee_hash = SyncCommitteeHashes::<T>::get(next_period); | ||
let next_sync_committee_hash: [u8; 32] = next_sync_committee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about unwrap
.
@@ -368,7 +383,7 @@ pub mod pallet { | |||
.expect("Rotate verification key should be valid at genesis."); | |||
RotateVerificationKey::<T>::set(Some(rotate_verification_key)); | |||
|
|||
SyncCommitteePoseidons::<T>::insert(self.period, self.sync_committee_poseidon); | |||
SyncCommitteeHashes::<T>::insert(self.period, self.sync_committee_hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mainnet genesis will be different using this new binary even if we use the same chain spec.
I am not sure about side-effect of that.
Inside the Vector pallet, instead of verifying a proof of light client logic, we natively implement the logic at a node level. This mirrors SP1 Telepathy's approach. The Rust-based logic built on top of Helios allows for seamless integration with Avail's pallet.
Additionally, this approach dramatically simplifies the operator. Instead of fetching specialized inputs and generating proofs, we simply retrieve inputs for Helios and relay that to the chain.
Modifications
We've optimized storage by directly using the canonical Ethereum consensus state's sync committee hash, eliminating the need for Poseidon hashes.
The updated verification logic is inside the
fulfill_call
function ofpallets/vector/src/lib.rs
.Operator
The new operator is based on SP1 Telepathy's operator. The logic remains the same, with placeholders for reading and writing to Avail using
avail-subxt
instead of a smart contract on Ethereum.For reference, I've created a fork of the SP1 Telepathy operator with added comments highlighting the lines that need to be changed: https://github.com/xavierdmello/avail-telepathy-operator
Testing
We've adapted a few tests into
tests_new.rs
, which replaces the currenttests.rs
. You can try the new ones by runningcargo test test_fulfill
inside of./pallets/vector/
. We generated test inputs using SP1 Telepathy (instructions here), which outputs bytes that should be placed inside./pallets/vector/examples