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

Proper handling of bad invalid bundle ER that point to non-exist extrinsic #2287

Closed
NingLin-P opened this issue Dec 4, 2023 · 4 comments · Fixed by #2787
Closed

Proper handling of bad invalid bundle ER that point to non-exist extrinsic #2287

NingLin-P opened this issue Dec 4, 2023 · 4 comments · Fixed by #2787
Assignees

Comments

@NingLin-P
Copy link
Member

Currently, when generating the invalid bundle fraud proof, the honest operator needs to generate a proof_of_inclusion for the invalid extrinsic that is contained in the bundle:
https://github.com/subspace/subspace/blob/7ee4687c10180cfa4d3a52da809e09d53a0887de/domains/client/domain-operator/src/fraud_proof.rs#L185-L192

While this is okay for TrueInvalid bundle fraud proof, because the invalid extrinsic is pointed out by the honest operator's local ER::bundle.

In the FalseInvalid bundle fraud proof, the honest operator needs to prove the invalid extrinsic of the bad ER's ER::bundle is actually valid, thus we need to use the extrinsic index provided by the bad ER, if the bad ER provides a non-exist extrinsic index (i.e. the bad ER claim an empty bundle is invalid) then the honest operator will fail to generate the proof_of_inclusion for such extrinsic and shut down due to this failure.

cc @vedhavyas @ParthDesai

@NingLin-P
Copy link
Member Author

NingLin-P commented Dec 4, 2023

As discussed with @vedhavyas, the solution to this issue would be adding a new variant of the invalid bundle fraud proof.

Currently, we have 2 variants of the invalid bundle fraud proof in general:

  • TrueInvalid, proving extrinsic at the specific index of the bundle is invalid
  • FalseInvalid, proving the invalid extrinsic claimed by the bad ER is actually valid

We need to introduce a new variant called UnknownInvalid (better naming is surely welcomed!), proving the invalid extrinsic claimed by the bad ER does not exist at all.

The honest operator will generate this fraud proof variant if it finds:

  • A bad ER that has a mismatched ER::inboxed_bundles field
  • And the ER::inboxed_bundles[mismatched_idx] is an invalid bundle that invalid_bundle_type.extrinsic_index() >= total_tx_count, where the total_tx_count is the total number of tx contained inside the given bundle

The UnknownInvalid bundle fraud proof only needs to contain data necessary to locate the controversial bundle, i.e. bad_receipt_hash, domain_id, and bundle_index.

The consensus node (the verifier) will verify this fraud proof by extracting the total_tx_count through the host function and checking the bad ER:
ER::inboxed_bundles[bundle_index].invalid_bundle_type.extrinsic_index() >= total_tx_count

cc @vedhavyas @dariolina

@dariolina
Copy link
Member

dariolina commented Dec 4, 2023

Makes sense to me. Though it seems like there is a way to merge this case into the FalseInvalid variant, by making it check that the claimed invalid index is within bounds and only if so generate proof_of_inclusion. If invalid_bundle_type.extrinsic_index() >= total_tx_count then we send None instead of proof_of_inclusion.
Or, if we make a new type, I suggest you can extend the names for more clarity to TrueInvalidExtrinsic, FalseInvalidExtrinsic, then the new variant can be MissingInvalidExtrinsic or AbsentInvalidExtrinsic

@dariolina
Copy link
Member

I'd like to double down on making this case a FalseInvalid since we use it not only just to show that a bundle is not Invalid but also that InvalidBundleType is unjustified (i.e. incorrect type or extrinsic index), then it makes sense for a InvalidBundleType(non-existent-extrinsic-index) to fall into this category.

@NingLin-P
Copy link
Member Author

Yeah, I'm okay with reusing the FalseInvalid variant to handle the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants