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

Inclusion proofs, error instead of panic #1609

Open
vmx opened this issue Jun 8, 2022 · 3 comments
Open

Inclusion proofs, error instead of panic #1609

vmx opened this issue Jun 8, 2022 · 3 comments

Comments

@vmx
Copy link
Contributor

vmx commented Jun 8, 2022

Description

Currently if we read in invalid data there is a panic:

panic_any(format!("from_repr failure at {}", i));
. The problem is that it's hard to track down where the bad data actually is. Ideally you'd know at least which sector contains invalid data (see #1604 for an example of this issue).

If an error would be returned (instead of a panic), one could then catch that error here

if inclusion_proof.root() != comm_r_last {
and return an error with the sector ID.

Acceptance criteria

The sector ID is somehow displayed if there is invalid data.

Risks + pitfalls

This also needs changes in the merkletree crate and I'm not sure how big the overall changes will be. So it might not be worth it.

Where to begin

Make

fn multi_node(&mut self, parts: &[PoseidonDomain], _height: usize) -> PoseidonDomain {
match parts.len() {
1 | 2 | 4 | 8 | 16 => shared_hash_frs(
&parts
.iter()
.enumerate()
.map(|(i, x)| {
if let Some(fr) = Fr::from_repr_vartime(x.0) {
fr
} else {
panic_any(format!("from_repr failure at {}", i));
}
})
.collect::<Vec<_>>(),
)
.into(),
arity => panic_any(format!("unsupported arity {}", arity)),
}
}
}
return Results instead of panicking and see which other changes are needed due to this.

@llifezou
Copy link

llifezou commented Jun 9, 2022

this problem may be related to special challange []uint64.

func GenerateSingleVanillaProof(
	replica PrivateSectorInfo,
	challange []uint64,
) ([]byte, error)

@storojs72
Copy link
Contributor

The problem is that it's hard to track down where the bad data actually is

That's true.

Not sure if it does relate to merkletree crate... It looks like implementation of from_repr_vartime is located in blstrs crate, isn't it? And information about sector seems to be out of scope for it.

Can't it be resolved by adding information about sector into calling code where panic is invoked? I mean in such case we need to change caller that use Poseidon hasher by adding something like that:

...
trace!("sector is {}", sector_num);
multi_node( ... );
...

and of course, Rust logging should be enabled, as it looks like it is invoked from Go high-level code.

@vmx
Copy link
Contributor Author

vmx commented Jun 9, 2022

Not sure if it does relate to merkletree crate... I

If Fr::from_repr_vartime(x.0) returns None, we could error instead of panic. But then multi_node() would need to return a Result. And that would mean that the LightAlgorithm/Algorithm trait would need to be changed, which lives in merkletree.

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

No branches or pull requests

3 participants