[1.0.4] Finality violation tests 1.0.0

[systemzax]

This PR adds tests to cover violation of finality rules #1, #2 and #3 and addresses issue : #91

[ericpassmore]

Note:start
category: Tests
component: Internal
summary: Tests to cover violation of finality rule.
Note:end

[arhag]

It would be better to return a pair of the aggregate key and the total weight.

[arhag]

This doesn't make sense as a way to check the aggregate signature. See how Spring does it.

spring/libraries/chain/qc.cpp

Lines 205 to 208 in d89dc9f

	// validate aggregated signature
	EOS_ASSERT( bls12_381::aggregate_verify(pubkeys, digests, sig.jacobian_montgomery_le()),
	invalid_qc_signature, "qc signature validation failed" );

[arhag]

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 count_strong_only == true even if weak votes are present in the QC.

[arhag]

Basically you just need the three cases within the largest else branch here. Although there is a cleaner way of structuring this code See:

spring/libraries/chain/qc.cpp

Lines 168 to 209 in d89dc9f

	void qc_sig_t::verify_signatures(const finalizer_policy_ptr& fin_policy,
	const digest_type& strong_digest,
	const weak_digest_t& weak_digest) const {
	const auto& finalizers = fin_policy->finalizers;
	auto num_finalizers = finalizers.size();
	// no reason to use bls_public_key wrapper
	std::vector<bls12_381::g1> pubkeys;
	pubkeys.reserve(2);
	std::vector<std::vector<uint8_t>> digests;
	digests.reserve(2);
	// utility to aggregate public keys for verification
	auto aggregate_pubkeys = [&](const auto& votes_bitset) -> bls12_381::g1 {
	const auto n = std::min(num_finalizers, votes_bitset.size());
	std::vector<bls12_381::g1> pubkeys_to_aggregate;
	pubkeys_to_aggregate.reserve(n);
	for(auto i = 0u; i < n; ++i) {
	if (votes_bitset[i]) { // ith finalizer voted
	pubkeys_to_aggregate.emplace_back(finalizers[i].public_key.jacobian_montgomery_le());
	}
	}
	return bls12_381::aggregate_public_keys(pubkeys_to_aggregate);
	};
	// aggregate public keys and digests for strong and weak votes
	if( strong_votes ) {
	pubkeys.emplace_back(aggregate_pubkeys(*strong_votes));
	digests.emplace_back(std::vector<uint8_t>{strong_digest.data(), strong_digest.data() + strong_digest.data_size()});
	}
	if( weak_votes ) {
	pubkeys.emplace_back(aggregate_pubkeys(*weak_votes));
	digests.emplace_back(std::vector<uint8_t>{weak_digest.begin(), weak_digest.end()});
	}
	// validate aggregated signature
	EOS_ASSERT( bls12_381::aggregate_verify(pubkeys, digests, sig.jacobian_montgomery_le()),
	invalid_qc_signature, "qc signature validation failed" );
	}

I don't think there is even a reason to have the count_strong_only boolean. You may just want to add a bool is_strong() const member function to quorum_certificate_input so that the IBC contract can do a separate check to see if the submitted QC is strong.

[arhag]

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 the i/4 == 0 nibble (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 the i%4 == 2 bit 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 representation 0110). 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 representation 60 in this case.

[arhag]

You still need to keep this time range conflict check: the one that verifies that the low_proof_timestamp is between the high_proof_last_claim_timestamp and the high_proof_timestamp. That is independent of the other time checks that you do below.

[arhag]

I think the time check here is trying to validate the assumptions of a valid rule 2 proof that I outline in:

First, the user must submit the contents of the leaf node in the reversible block Merkle tree (along with the proof of inclusion up to the reversible blocks root) for the block that has a timestamp greater than or equal to low_proof_timestamp and a parent_timestamp strictly less than low_proof_timestamp. Both of those inequalities must be validated otherwise the submitted proof is not correct.

But the checks on this line does not do that.

You want to make sure that (proof_of_inclusion.target.parent_timestamp < low_proof_timestamp) && (low_proof_timestamp <= proof_of_inclusion.target.timestamp) is true. If this is not true, then the proof is invalid and the action should abort.

[arhag]

You don't need to have a finality_violation boolean.

Given that you will check the low_proof_timestamp is in the interval (proof_of_inclusion.target.parent_timestamp, proof_of_inclusion.target.timestamp] earlier above, you can simplify this if else to just the following if statement:

if (proof_of_inclusion.target.timestamp == low_proof_timestamp) {
   // If the timestamp for the submitted reversible blocks leaf node is exactly equal to low_proof_timestamp, 
   // we need to compare the finality digest of the low proof block to the finality digest of the 
   // submitted reversible blocks leaf node to check that they are not the same. 
   // If they are the same, the submitted proof is not correct. 
   // But if they are different, then we know that the low proof block is not an ancestor of the high proof block.
   check(finalizer_digests.second != proof_of_inclusion.target.finality_digest, "finality digest of low proof must be different from the finality digest of the submitted reversible blocks leaf node");
}

[arhag]

All of these timestamps are still needed.

We still need to verify one of the timestamp assumptions of rule 3 violations:

bool lock_violation = 
       (high_proof_last_claim_timestamp <= low_proof_last_claim_timestamp) &&
       (low_proof_timestamp < high_proof_timestamp);
check(lock_violation, "proofs must demonstrate a lock violation");

This is independent of verifying that the target reversible block also satisfies the timestamp requirements:

bool appropriate_target_reversible_block =
       (proof_of_inclusion.target.parent_timestamp < low_proof_last_claim_timestamp) &&
       (low_proof_last_claim_timestamp <= proof_of_inclusion.target.timestamp);
check(appropriate_target_reversible_block, "proofs must include appropriate reversible target block");

[arhag]

Again no need for finality_violation boolean.

If the appropriate time checks have been done earlier, then all that is needed is to handle the case where proof_of_inclusion.target.timestamp exactly equals low_proof_last_claim_timestamp:

if (proof_of_inclusion.target.timestamp == low_proof_last_claim_timestamp) {
   // If the timestamp for the submitted reversible blocks leaf node is exactly equal to low_proof_last_claim_timestamp, 
   // we need to compare the finality digest of the block claimed by the low proof block to the finality digest of the 
   // submitted reversible blocks leaf node to check that they are not the same. 
   // If they are the same, the submitted proof is not correct. 
   // But if they are different, then we know that the block claimed by the low proof block is not an ancestor of the high proof block.
   check(low_proof.qc_block.level_3_commitments.value().latest_qc_claim_finality_digest != proof_of_inclusion.target.finality_digest, "finality digest of block claimed by low proof block must be different from the finality digest of the submitted reversible blocks leaf node");
}