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

refactor: lightclient verification flags #2097

Merged
merged 37 commits into from
May 8, 2024

Conversation

kingpinXD
Copy link
Contributor

@kingpinXD kingpinXD commented Apr 29, 2024

Description

  • Refactor MsgUpdateVerificationFlags into MsgEnableHeaderVerification and MsgDisableHeaderVerification .
  • Refactor Verification flags to use individual chain IDs instead of consensus mechanisms such as ETH /BTC
  • Rename Verifaction flags to BlockHeaderVerification which stores a list of chains

It does not add a migration script , as the flags can be enable once the upgrade rolls out

Closes: #2083

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 91.83673% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 72.33%. Comparing base (6d80018) to head (35a0cd3).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2097      +/-   ##
===========================================
+ Coverage    72.30%   72.33%   +0.03%     
===========================================
  Files          247      250       +3     
  Lines        13921    14041     +120     
===========================================
+ Hits         10066    10157      +91     
- Misses        3394     3423      +29     
  Partials       461      461              
Files Coverage Δ
pkg/chains/chain.go 96.11% <100.00%> (ø)
pkg/chains/chains.go 100.00% <100.00%> (ø)
x/lightclient/genesis.go 100.00% <100.00%> (ø)
x/lightclient/keeper/block_header.go 94.11% <100.00%> (ø)
x/lightclient/keeper/block_header_verification.go 100.00% <100.00%> (ø)
...tclient/keeper/grpc_query_header_enabled_chains.go 100.00% <100.00%> (ø)
...er/msg_server_disable_block_header_verification.go 100.00% <100.00%> (ø)
...per/msg_server_enable_block_header_verification.go 100.00% <100.00%> (ø)
x/lightclient/keeper/proof.go 69.23% <100.00%> (-30.77%) ⬇️
x/lightclient/types/block_header_verification.go 100.00% <100.00%> (ø)
... and 10 more

@kingpinXD kingpinXD marked this pull request as ready for review April 30, 2024 05:05
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Refactor storage for Verification flags to use chain-id as index

Why adding this indexation instead of simply keep an array of chain IDs in a singleton?

VerificationFlags {
  ChainIDs []int64
}

It's easier to maintain, we can put most of the logic in types in pure function and easier to migrate
In general we should avoid adding indexation when necessary since this complexify the state. Here it would not be necessary, the rollout of chain header supported would be slow so the array would not continously grow unlike cctxs and it wouldn't be a performance issue to iterate the array to detect enabled chains

(the variable should also be renamed from flags since we don't have a a predifined list of values to set/unset but can be considered for another PR)

@kingpinXD
Copy link
Contributor Author

Having an index makes the update much easier, especially when we consider that the input is a list of chain IDs.
From migration perspective I think both options are similar , we would either iterate before or after fetching the value.
In my opinion , maintaining the index is simpler and easier to manage in terms of get and set values. Let me know if you still strongly feel about keeping the singleton.

I agree regarding the names

@lumtis
Copy link
Member

lumtis commented Apr 30, 2024

Having an index makes the update much easier, especially when we consider that the input is a list of chain IDs.
From migration perspective I think both options are similar , we would either iterate before or after fetching the value.
In my opinion , maintaining the index is simpler and easier to manage in terms of get and set values. Let me know if you still strongly feel about keeping the singleton.

Using a singleton simplifies the interaction and representation of the store with a single entrypoint. This structure is like parameters of a module, it's not a ever-growing array of data where indexing is critical, I see it as a simpler model to use a self-containing structure like we do for similar structures.
Updating might be more complex but is easy to encapsulate

list.Add(chainID)
list.Remove(chainID)
list.Exist(chainID)

The algo would be:

list := store.GetList()
for chainID := range chainIDs
  list.Add(chainID)
store.SaveList(list)

Finally the query would be simplified, the ZetaClient would directly get a list of chainIDs that are enabled instead of a list of VerificationFlag object that can contains false

@kingpinXD kingpinXD requested a review from lumtis May 3, 2024 15:55
Copy link
Contributor

@skosito skosito left a comment

Choose a reason for hiding this comment

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

lgtm, just couple small comments

@kingpinXD kingpinXD merged commit 372c9ec into develop May 8, 2024
21 checks passed
@kingpinXD kingpinXD deleted the refactor-lightclient-verification-flags branch May 8, 2024 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change VerificationFlags to hold a list ChainID->bool
3 participants