Fraud proof naming, test & comment cleanups #3369

teor2345 · 2025-02-06T00:47:58Z

How to review this PR

The real diff starts at aa63e5d, the other commits are from the main branch. (They'll disappear once #3368 merges.)

What it does

This PR refactors some fraud proof code so it's easier to use and understand:

Use HashingFor rather than complex trait type paths (some added in Fix inconsistency of bundle digest generation and add invalid fraud proof integration test #3368)
Rename IsFalseInvalid because double negatives are confusing, and make comments clearer
Ignore test-only ignore_invalid_proof argument in production builds (added in Fix inconsistency of bundle digest generation and add invalid fraud proof integration test #3368)
Simplify some tests added in Fix inconsistency of bundle digest generation and add invalid fraud proof integration test #3368 using test setup functions

Code contributor checklist:

I have read, understood and followed contributing guide

vedhavyas · 2025-02-07T05:25:25Z

crates/sp-domains-fraud-proof/src/fraud_proof.rs

-    pub is_true_invalid_fraud_proof: bool,
+    /// If `true`, the fraud proof must prove the local bundle is invalid.
+    /// If `false`, the fraud proof must prove the remote's claimed invalid bundle is wrong.
+    pub prove_local_is_invalid_fraud_proof: bool,


local does not signify what you mean here.

What this actually means is whether we are proving if a bundle is actually invalid or prove if the invallid marked bundle is actually valid

Yeah, both TrueInvalid and FalseInvalid are attested against a (remote) ER, TrueInvalid means the fraud proof needs to prove something the ER claimed is valid is actually invalid, FalseInvalid means the fraud proof needs to prove something the ER claimed is invalid is actually valid.

Local/remote is a bit more confusing to me, because the meaning changed in different places (e.g. in the domain node when generating FP vs in the consensus node when verifying FP).

I changed it to:

use_bundle_is_invalid_fraud_proof

InvalidProofIsWrong

InvalidProofIsCorrect

ValidBundleContents

Still, I prefer the original TrueInvalid/FalseInvalid, because the BundleMismatchType is about the mismatch of the BundleValidity in the ER, while use_bundle_is_invalid_fraud_proof seems to claim a bundle is valid or not and InvalidProofIsWrong/InvalidProofIsCorrect seems to claim "there is a proof of invalidity and it is correct/wrong"

Ok, I see your point. But I'm still not quite understanding the code from the type names, the existing comments, and your explanations. So maybe we could make some improvements here?

Here's what I find confusing:

TrueInvalid is confusing English grammar, which makes it harder for me to understand what it means

is_true_invalid_fraud_proof: bool is both confusing grammar, and an unusual use of types, so it's hard for me to work out what the double negative is_true_invalid_fraud_proof: false means

BundleMismatchType is a negative, which makes BundleMismatchType::FalseInvalid a confusing double-negative

Would you be happy with these name changes?

is_true_invalid_fraud_proof -> is_{good,correct,accurate,(nothing)}_invalid_fraud_proof

TrueInvalid -> {Good,Correct,Accurate,(nothing)}Invalid

FalseInvalid -> {Bad,Wrong,Faulty}Invalid

Valid -> ValidBundleContents

So maybe we could make some improvements here?

Definitely! The above suggestion looks good to me as they reflect "the mismatch of the BundleValidity", feel free to choose one you think is the best, and thanks for being patient!

domains/client/domain-operator/src/fraud_proof.rs

NingLin-P

Rest LGTM, thanks!

NingLin-P · 2025-02-07T11:28:54Z

crates/sp-domains-fraud-proof/src/fraud_proof.rs

-    pub is_true_invalid_fraud_proof: bool,
+    /// If `true`, the fraud proof must prove the local bundle is invalid.
+    /// If `false`, the fraud proof must prove the remote's claimed invalid bundle is wrong.
+    pub prove_local_is_invalid_fraud_proof: bool,


Yeah, both TrueInvalid and FalseInvalid are attested against a (remote) ER, TrueInvalid means the fraud proof needs to prove something the ER claimed is valid is actually invalid, FalseInvalid means the fraud proof needs to prove something the ER claimed is invalid is actually valid.

Local/remote is a bit more confusing to me, because the meaning changed in different places (e.g. in the domain node when generating FP vs in the consensus node when verifying FP).

teor2345

This should be ready for review again.

Sorry for the rebase, GitHub kept saying there was a merge error when the commits applied fine in git.

teor2345

Happy to drop this change if we can't sort it out this time around. I'll take it to a ticket instead.

(The comment notes are for me for the next revision.)

domains/client/domain-operator/src/aux_schema.rs

crates/sp-domains-fraud-proof/src/verification.rs

teor2345 · 2025-02-10T10:32:49Z

crates/sp-domains-fraud-proof/src/fraud_proof.rs

-    pub is_true_invalid_fraud_proof: bool,
+    /// If `true`, the fraud proof must prove the local bundle is invalid.
+    /// If `false`, the fraud proof must prove the remote's claimed invalid bundle is wrong.
+    pub prove_local_is_invalid_fraud_proof: bool,


Ok, I see your point. But I'm still not quite understanding the code from the type names, the existing comments, and your explanations. So maybe we could make some improvements here?

Here's what I find confusing:

TrueInvalid is confusing English grammar, which makes it harder for me to understand what it means

is_true_invalid_fraud_proof: bool is both confusing grammar, and an unusual use of types, so it's hard for me to work out what the double negative is_true_invalid_fraud_proof: false means

BundleMismatchType is a negative, which makes BundleMismatchType::FalseInvalid a confusing double-negative

Would you be happy with these name changes?

is_true_invalid_fraud_proof -> is_{good,correct,accurate,(nothing)}_invalid_fraud_proof

TrueInvalid -> {Good,Correct,Accurate,(nothing)}Invalid

FalseInvalid -> {Bad,Wrong,Faulty}Invalid

Valid -> ValidBundleContents

teor2345

This should be ready for final review.

vedhavyas

LGTM apart from the suggestion. Feel free to ignore if you have a strong opinion on this

vedhavyas · 2025-02-11T05:56:16Z

crates/sp-domains-fraud-proof/src/fraud_proof.rs

-    /// Proof data of the invalid bundle
+    /// If `true`, the fraud proof must prove the bundle was correctly marked invalid.
+    /// If `false`, it must prove the bundle was marked invalid, but is actually valid.
+    pub is_good_invalid_fraud_proof: bool,


Honestly, I would prefer the previous naming itself.
Here is my thought process

is_true_invalid_fraud_proof signifies whether fraud proof is targetting a truly invalid bundle or if false signifies if targetting a proof to show incorrectly marked invalid bundle.

When it comes to is_good_invalid_fraud_proof, I'm not sure, without context, understand what good signifies here unlike TrueInvalid which brings more context self contained.

It could be that my grammer maybe off here.
I do not want to block this PR on this name change but in the past there was a similar dicussion and final result ended up being going with True Prefix since the variants in it are just 2 and easily understood

Oops, looks like this already merged.

Yep, naming things is hard, particularly when we have multiple layers of negatives: “fraud”, “invalid”, and then the bool possibly being false.

It might be there is no perfect solution here, happy to have it changed back to “truly” (which is different enough from a true bool to make it a bit easier).

teor2345 added the refactor label Feb 6, 2025

teor2345 self-assigned this Feb 6, 2025

teor2345 mentioned this pull request Feb 6, 2025

Fix inconsistency of bundle digest generation and add invalid fraud proof integration test #3368

Merged

1 task

Base automatically changed from invalid-fp-test to main February 6, 2025 15:12

teor2345 marked this pull request as ready for review February 6, 2025 23:02

teor2345 requested review from NingLin-P, vedhavyas and nazar-pc as code owners February 6, 2025 23:02

teor2345 enabled auto-merge February 6, 2025 23:02

vedhavyas reviewed Feb 7, 2025

View reviewed changes

NingLin-P reviewed Feb 7, 2025

View reviewed changes

teor2345 force-pushed the invalid-fp-test-tweaks branch from de3dd60 to 786d5fc Compare February 10, 2025 01:33

teor2345 requested review from vedhavyas and NingLin-P February 10, 2025 01:47

Use HashingFor to simplify trait paths

860d704

teor2345 force-pushed the invalid-fp-test-tweaks branch from 332eef9 to 13df0e2 Compare February 10, 2025 01:51

teor2345 commented Feb 10, 2025

View reviewed changes

teor2345 added 5 commits February 11, 2025 07:55

Rename confusing double negative IsFalseInvalid

f717568

Ignore allow_invalid_proof in production

cdb6b43

Make invalid proof generation more explicit in tests

e038c31

Simplify test code with setup functions

5aa7aed

Fix comments

2966cb0

teor2345 force-pushed the invalid-fp-test-tweaks branch from 13df0e2 to 2966cb0 Compare February 10, 2025 21:57

teor2345 commented Feb 10, 2025

View reviewed changes

teor2345 added this pull request to the merge queue Feb 11, 2025

vedhavyas approved these changes Feb 11, 2025

View reviewed changes

Merged via the queue into main with commit c8b57a9 Feb 11, 2025
8 checks passed

teor2345 deleted the invalid-fp-test-tweaks branch February 11, 2025 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fraud proof naming, test & comment cleanups #3369

Fraud proof naming, test & comment cleanups #3369

teor2345 commented Feb 6, 2025

vedhavyas Feb 7, 2025

NingLin-P Feb 7, 2025

teor2345 Feb 10, 2025

NingLin-P Feb 10, 2025

teor2345 Feb 10, 2025

NingLin-P Feb 10, 2025

NingLin-P left a comment

NingLin-P Feb 7, 2025

teor2345 left a comment

teor2345 left a comment

teor2345 Feb 10, 2025

teor2345 left a comment

vedhavyas left a comment

vedhavyas Feb 11, 2025

teor2345 Feb 11, 2025

Fraud proof naming, test & comment cleanups #3369

Fraud proof naming, test & comment cleanups #3369

Conversation

teor2345 commented Feb 6, 2025

How to review this PR

What it does

Code contributor checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NingLin-P left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

vedhavyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment