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

feat(base_layer): checkpoint quorum validation #4303

Merged
merged 4 commits into from
Jul 12, 2022

Conversation

mrnaveira
Copy link
Contributor

Description

Refactored the base layer committee validation for checkpoints:

  • Now it's based on HashSet of VNC public keys: one for the constitution and other for the checkpoint
    • Surprisingly, clippy warns about mutable_key_type for using PublicKey types as the keys in the HashSet. I'm not sure where the mutability comes from. I decided to ignore that rule for the validation function
  • The existing validation of members is now done by checking the difference of the public key sets
  • The new quorum validation is done by checking the intersection of the public key sets

Motivation and Context

The base layer need to validate if the quorum of an incoming checkpoint is met. The minimum quorum for checkpoints is specified in the ContractConstitution, in the checkpoint_params.minimum_quorum_required field

How Has This Been Tested?

New unit test to check both successful and failing scenarios regarding quorum conditions

sdbondi
sdbondi previously approved these changes Jul 12, 2022
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

I like the use of HashSet for ease of code understanding, but it does result in some extra clones + heap allocations which could be avoided with a for loop. I think a HashSet<&PublicKey> would also work and use less memory (allocates only n words/usize).

In any case, happy for it to go in 👍

@aviator-app aviator-app bot removed the mq-failed label Jul 12, 2022
@aviator-app aviator-app bot merged commit e1704f4 into tari-project:development Jul 12, 2022
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