[1.0.4] Finality violation tests 1.0.0 #610
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -359,4 +359,4 @@ namespace finality_proof { | |
|
|
||
| }; | ||
|
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,12 +22,18 @@ namespace savanna { | |
| return std::string(res.begin(), res.end()); | ||
| } | ||
|
|
||
| inline std::string create_strong_digest(const checksum256& digest){ | ||
| std::array<uint8_t, 32> fd_data = digest.extract_as_byte_array(); | ||
| return std::string(fd_data.begin(), fd_data.end()); | ||
| } | ||
|
|
||
| struct quorum_certificate_input { | ||
| //representation of optional strong and weak bitsets, where each bit represents the ordinal finalizer position according to canonical sorting rules of the finalizer policy | ||
| std::optional<std::vector<uint8_t>> strong_votes; | ||
| std::optional<std::vector<uint8_t>> weak_votes; | ||
|
|
||
| //string representation of a BLS signature | ||
| std::string signature; | ||
| }; | ||
|
|
||
| struct finalizer_authority_internal { | ||
|
|
@@ -120,7 +126,8 @@ namespace savanna { | |
| checksum256 _compute_root(const std::vector<checksum256>& proof_nodes, const checksum256& target, const uint64_t target_block_index, const uint64_t final_block_index){ | ||
| checksum256 hash = target; | ||
| std::vector<bool> proof_path = _get_proof_path(target_block_index, final_block_index+1); | ||
|
|
||
| check(proof_path.size() == proof_nodes.size(), "proof path size and proof nodes size mismatch"); | ||
| for (int i = 0 ; i < proof_nodes.size() ; i++){ | ||
| const checksum256 node = proof_nodes[i]; | ||
| hash = hash_pair(proof_path[i] ? std::make_pair(node, hash) : std::make_pair(hash, node)); | ||
|
|
@@ -134,54 +141,112 @@ namespace savanna { | |
| bls_g1_add(op1, op2, r); | ||
| return r; | ||
| } | ||
|
|
||
| void check_duplicate_votes(const savanna::bitset& strong_votes, const savanna::bitset& weak_votes, const finalizer_policy_input& finalizer_policy){ | ||
|
|
||
| auto fa_itr = finalizer_policy.finalizers.begin(); | ||
| auto fa_end_itr = finalizer_policy.finalizers.end(); | ||
|
|
||
| size_t index = 0; | ||
|
|
||
| while (fa_itr != fa_end_itr){ | ||
| bool voted_strong = strong_votes.test(index); | ||
| bool voted_weak = weak_votes.test(index); | ||
| check (!(voted_strong && voted_weak), "conflicting finalizer votes detected in QC"); | ||
| index++; | ||
| fa_itr++; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| uint64_t aggregate_keys(const savanna::bitset& votes, const finalizer_policy_input& finalizer_policy, bls_g1& agg_pub_key){ | ||
|
Contributor
There was a problem hiding this comment. It would be better to return a pair of the aggregate key and the total weight. |
||
| auto fa_itr = finalizer_policy.finalizers.begin(); | ||
| auto fa_end_itr = finalizer_policy.finalizers.end(); | ||
|
|
||
| bool first = true; | ||
|
|
||
| size_t index = 0; | ||
| uint64_t weight = 0; | ||
|
|
||
| while (fa_itr != fa_end_itr){ | ||
| if (votes.test(index)){ | ||
| bls_g1 pub_key = decode_bls_public_key_to_g1(fa_itr->public_key); | ||
| if (first){ | ||
| first=false; | ||
| agg_pub_key = pub_key; | ||
| } | ||
| else agg_pub_key = _g1add(agg_pub_key, pub_key); | ||
| weight+=fa_itr->weight; | ||
| } | ||
| index++; | ||
| fa_itr++; | ||
| } | ||
|
|
||
| return weight; | ||
|
|
||
| } | ||
|
|
||
| void _verify(const std::vector<std::string>& messages, const std::vector<bls_g1>& agg_pub_keys, const std::string& agg_sig){ | ||
|
|
||
| check(messages.size() == agg_pub_keys.size(), "messages vector and pub key vectors must be of the same size"); | ||
|
|
||
| for (size_t i = 0 ; i < messages.size(); i++){ | ||
| //verify signature validity | ||
| check(bls_signature_verify(agg_pub_keys[i], decode_bls_signature_to_g2(agg_sig), messages[i]), "signature verification failed"); | ||
|
Contributor
There was a problem hiding this comment. This doesn't make sense as a way to check the aggregate signature. See how Spring does it. Lines 205 to 208 in d89dc9f |
||
| } | ||
|
|
||
| } | ||
|
|
||
| //verify that the quorum certificate over the finality digest is valid | ||
| void _check_qc(const quorum_certificate_input& qc, const checksum256& finality_digest, const finalizer_policy_input& finalizer_policy, const bool count_strong_only, const bool enforce_threshold_check){ | ||
|
|
||
| check(qc.strong_votes.has_value() || qc.weak_votes.has_value(), "quorum certificate must have at least one bitset"); | ||
|
|
||
| size_t finalizer_count = finalizer_policy.finalizers.size(); | ||
|
|
||
| uint64_t weight = 0; | ||
|
|
||
| if (count_strong_only){ | ||
| bls_g1 agg_pub_key; | ||
| check(qc.strong_votes.has_value(), "required strong votes are missing"); | ||
| savanna::bitset strong_b(finalizer_count, qc.strong_votes.value()); | ||
| weight = aggregate_keys(strong_b, finalizer_policy, agg_pub_key); | ||
| _verify({create_strong_digest(finality_digest)}, {agg_pub_key}, qc.signature); | ||
|
Contributor
There was a problem hiding this comment. This only works if the weak bitset is empty. If it is not empty, then the QC signature would be an aggregate signature formed from the aggregate public key of the strong votes (calculated here) corresponding to the strong digest AND the aggregate public key to the weak votes corresponding to the weak digest. I think Spring will never bother generating a QC with mixed strong and weak votes when the strong votes are sufficient to meet the threshold. But as far as the protocol is concerned, if weak votes are present, the QC is treated as a weak QC and the aggregate signature must incorporate the effect of those weak votes. I think these contracts should not necessarily assume that and be able to handle validating the signature with |
||
| } | ||
| else { | ||
|
|
||
| if (qc.strong_votes.has_value() && qc.weak_votes.has_value()){ | ||
| //weak QC (composed of strong and weak votes) | ||
| bls_g1 strong_agg_pub_key; | ||
| bls_g1 weak_agg_pub_key; | ||
| savanna::bitset strong_b(finalizer_count, qc.strong_votes.value()); | ||
| savanna::bitset weak_b(finalizer_count, qc.weak_votes.value()); | ||
| check_duplicate_votes(strong_b, weak_b, finalizer_policy); | ||
| uint64_t strong_weight = aggregate_keys(strong_b, finalizer_policy, strong_agg_pub_key); | ||
| uint64_t weak_weight = aggregate_keys(strong_b, finalizer_policy, weak_agg_pub_key); | ||
| _verify({create_strong_digest(finality_digest), create_weak_digest(finality_digest)}, {strong_agg_pub_key, weak_agg_pub_key}, qc.signature); | ||
| weight=strong_weight+weak_weight; | ||
| } | ||
| else if (qc.weak_votes.has_value()){ | ||
| //weak QC (composed of weak votes) | ||
| bls_g1 agg_pub_key; | ||
| savanna::bitset weak_b(finalizer_count, qc.weak_votes.value()); | ||
| weight = aggregate_keys(weak_b, finalizer_policy, agg_pub_key); | ||
| _verify({create_weak_digest(finality_digest)}, {agg_pub_key}, qc.signature); | ||
| } | ||
| else { | ||
| //strong QC | ||
| bls_g1 agg_pub_key; | ||
| savanna::bitset strong_b(finalizer_count, qc.strong_votes.value()); | ||
| weight = aggregate_keys(strong_b, finalizer_policy, agg_pub_key); | ||
| _verify({create_strong_digest(finality_digest)}, {agg_pub_key}, qc.signature); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Basically you just need the three cases within the largest else branch here. Although there is a cleaner way of structuring this code See: Lines 168 to 209 in d89dc9f I don't think there is even a reason to have the |
||
| } | ||
arhag marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (enforce_threshold_check) check(weight>=finalizer_policy.threshold, "insufficient signatures to reach quorum"); | ||
|
|
||
| } | ||
|
|
||
| checksum256 get_merkle_root(const std::vector<checksum256>& leaves) { | ||
|
|
@@ -268,8 +333,8 @@ namespace savanna { | |
| }; | ||
|
|
||
| struct action_proof_of_inclusion { | ||
| uint64_t target_action_index = 0; | ||
| uint64_t final_action_index = 0; | ||
|
|
||
| action_data target; | ||
|
|
||
|
|
@@ -278,7 +343,7 @@ namespace savanna { | |
| //returns the merkle root obtained by hashing target.digest() with merkle_branches | ||
| checksum256 root() const { | ||
| checksum256 digest = action_data_internal(target).digest(); | ||
| checksum256 root = _compute_root(merkle_branches, digest, target_action_index, final_action_index); | ||
| return root; | ||
| }; | ||
| }; | ||
|
|
@@ -331,8 +396,8 @@ namespace savanna { | |
| }; | ||
|
|
||
| struct reversible_proof_of_inclusion { | ||
| uint64_t target_reversible_block_index = 0; | ||
| uint64_t final_reversible_block_index = 0; | ||
|
|
||
| block_ref_data target; | ||
|
|
||
|
|
@@ -341,7 +406,7 @@ namespace savanna { | |
| //returns the merkle root obtained by hashing target.digest() with merkle_branches | ||
| checksum256 root() const { | ||
| checksum256 digest = target.digest(); | ||
| checksum256 root = _compute_root(merkle_branches, digest, target_reversible_block_index, final_reversible_block_index); | ||
| return root; | ||
| }; | ||
| }; | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This still does not look correct to me.
If I want to check whether the third finalizer (
i == 2) is present, I would expect to look at thei/4 == 0nibble (using 0-based indexing and counting from left to right, i.e. most significant nibble to least significant nibble). Then converting that nibble into bits, I would expect thei%4 == 2bit in that nibble (using 0-based indexing and counting from MSB to LSB) to represent the presence of that finalizer.So, given a bit string of
011:I would expect the 3rd most significant bit of the first nibble to be 1. Similarly for the 2nd most significant bit of the first nibble. But, I would expect the 1st most significant bit of the first nibble to be 0. Furthermore, I would expect all other bits that are needed for padding purposes to be 0.
So the first nibble should be the hex digit
6(which has binary representation0110). And if we need an even number of nibbles in the hex string, then we can pad with a 0 nibble at the end to get the hex representation60in this case.