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

zoneconcierge: query inclusion proofs and add them to ProofEpochSealed #243

Merged
merged 14 commits into from
Dec 13, 2022

Conversation

SebastianElvis
Copy link
Member

Fixes BM-303

This PR implements the functionalities for querying inclusion proofs to Cosmos SDK's underlying DB. The query is done by using the sdk.Querier interface implemented by Cosmos SDK's underlying DB (in type store.CommitMultiStore). This PR also contains a fuzz test for it.

Given this functionality, this PR implements the inclusion proofs for the epoch metadata and the validator set in `ProofEpochSealed.

Future works

  • Fuzz tests for verifying inclusion proofs of epoch metadata and validator set (need to simulate epoching, checkpointing and zoneconcierge altogether)
  • Solidify verifier of inclusion proofs, specifically verifying the inclusion of both key and value. Currently we only verify the key.

Copy link
Contributor

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

Great work! Only minor comments:

)

func getEpochInfoKey(epochNumber uint64) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be cleaner if we move this function to x/zoneconcierge/types/keys.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is possible. However, my concern is that if we do this, then the function has to be public to any module that imports zoneconciergetypes. This might be too open. I thus chose to make this function and the below getValSetKey as private functions. If other modules under zoneconcierge need to use them in the future, I will move them to x/zoneconcierge/types/keys.go. 👍

return proof, nil
}

func getValSetKey(epochNumber uint64) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

return epochInfoKey
}

func (k Keeper) ProveEpochInfo(ctx sdk.Context, epoch *epochingtypes.Epoch) (*tmcrypto.ProofOps, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that ctx is not used. Do we still want to keep it?

return valSetKey
}

func (k Keeper) ProveValSet(ctx sdk.Context, epoch *epochingtypes.Epoch) (*tmcrypto.ProofOps, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, ctx not used

// ProveEpochSealed proves an epoch has been sealed, i.e.,
// - the epoch's validator set has a valid multisig over the sealer header
// - the epoch's validator set is committed to the sealer header's last_commit_hash
// - the epoch's metadata is committed to the sealer header's last_commit_hash
func (k Keeper) ProveEpochSealed(ctx sdk.Context, epochNumber uint64) (*types.ProofEpochSealed, error) {
var (
proof *types.ProofEpochSealed = &types.ProofEpochSealed{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
proof *types.ProofEpochSealed = &types.ProofEpochSealed{}
proof = &types.ProofEpochSealed{}

} else { // BLS sig has a valid quorum
require.Greater(t, subsetPower, valSet.GetTotalPower()*1/3)
require.NoError(t, err)
require.True(t, sdkerrors.IsOf(zctypes.ErrInvalidMerkleProof, err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
require.True(t, sdkerrors.IsOf(zctypes.ErrInvalidMerkleProof, err))
require.ErrorIs(t, err, zctypes.ErrInvalidMerkleProof)

@@ -92,9 +94,30 @@ func FuzzProofEpochSealed(f *testing.F) {
if numSubSet <= numVals*1/3 { // BLS sig does not reach a quorum
require.LessOrEqual(t, subsetPower, uint64(numVals*1/3))
require.Error(t, err)
require.False(t, sdkerrors.IsOf(zctypes.ErrInvalidMerkleProof, err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
require.False(t, sdkerrors.IsOf(zctypes.ErrInvalidMerkleProof, err))
require.NotErrorIs(t, err, zctypes.ErrInvalidMerkleProof)

@SebastianElvis SebastianElvis merged commit 2fbf785 into dev Dec 13, 2022
@SebastianElvis SebastianElvis deleted the zoneconcierge-metadata-inclusion-proofs branch December 13, 2022 10:10
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