Distributed verification

[poszu]

Changes to support distributed verification (spacemeshos/go-spacemesh#5185):

• added verification modes (All, One, and Subset)
• Subset takes the number of indices to verify and a seed
  • the seed is used to initialize local randomness for selecting K3 indices (so that nodes verify different subsets)
• return the invalid index in POST proof - required for malfeasance proof

💡 There are some changes in C definitions. It's caused by improved enum definitions in the FFI on the post-rs side.

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (a09cf87) 70.7% compared to head (f559650) 71.4%.

Files	Patch %	Lines
internal/postrs/initializer.go	42.8%	4 Missing ⚠️
internal/postrs/proof.go	93.4%	3 Missing and 1 partial ⚠️
internal/postrs/pos_verifier.go	72.7%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop    #256     +/-   ##
=========================================
+ Coverage     70.7%   71.4%   +0.6%     
=========================================
  Files           29      29             
  Lines         1816    1879     +63     
=========================================
+ Hits          1285    1342     +57     
- Misses         391     396      +5     
- Partials       140     141      +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fasmat]

I'm not a fan of the proposed ABI for post-rs. I think it would be much simpler to just have 3 verify functions on the ABI level:

• C.verifyAll(...)
• C.verifyOne(index, ...)
• C.verifySubset(k3, seed, ...)

Outside of the internal package we would still have only one Verify method with functional options that just depending on the provided options calls a different C function. On the rust side I'm sure that the 3 exported functions can just call one general verify function with different parameters depending on the ABI function that was called. This makes the C interface much simpler, no need to serialize objects / structs across ABI if we can do it with simple types.

With arrays I see it similarly:

cSeed := C.CBytes(seed)
defer C.free(cSeed)

// len(seed) is arguably not needed because we can already check on the Go side that it is 32 bytes
// and return an error if it is not
C.verify(..., cSeed, len(seed), ...)

seems easier and is safer than:

cSeed := C.ArrayU8{
	ptr: (*C.uchar)(unsafe.SliceData(seed)),
	len: C.size_t(len(seed)),
	cap: C.size_t(cap(seed)),
}
C.verify(..., cSeed, ...)

I'm not even sure that the 2nd one is necessarily faster than the first one 🤔

[fasmat]

That seems a) dangerous and b) very platform / compiler specific; i.e. just because it works on windows/amd64 doesn't mean it works on macOS/arm64. I think it would be simpler and safer to just return "file n contains invalid label at offset x" as string from post-rs and then wrap it in the returned error:

Suggested change

	case C.VerifyResult_Invalid:
	result := castBytes[C.VerifyPosResult_Invalid_Body](result.anon0[:])
	return fmt.Errorf("%w: file %d contains invalid label at offset %d", ErrInvalidPos, result.file, result.offset)
	case C.VerifyResult_Invalid:
	return fmt.Errorf("%w: %s", ErrInvalidPos, C.GoString(result.error))

[poszu]

All C code is dangerous 🙃. What you propose works for verifying POS, but wouldn't work for verifying proof - we need the index there.

[fasmat]

I think the issue I have with this approach is that the return value of the verify function is essentially a void * (or at least of the field anon0 in the returned object), but I don't think it needs to be. Neither here nor in the case of verifying proof. The return value for C.verify_pos can just be (in go syntax):

type pos_result struct {
	tag    int    // the error code
	file   string // only set when tag == C.VerifyResult_Invalid
	offset int    // only set when tag == C.VerifyResult_Invalid
}

and the return value for C.verify_proof can instead be:

type proof_result struct {
	tag      int // the error code
	index_id int // only set when tag == C.VerifyResult_InvalidIndex
}

Without a generic approach the object-byte-cast wouldn't be necessary 🙂.

[fasmat]

Maybe we can just give the anon0 field a name? Then I'd already be happy 🙂

[poszu]

The header is autogenerated with https://github.com/mozilla/cbindgen. I looked but I didn't find a way to force it to name the union :(

[poszu]

I'm not a fan of the proposed ABI for post-rs. I think it would be much simpler to just have 3 verify functions on the ABI level:

• C.verifyAll(...)
• C.verifyOne(index, ...)
• C.verifySubset(k3, seed, ...)

Outside of the internal package we would still have only one Verify method with functional options that just depending on the provided options calls a different C function. On the rust side I'm sure that the 3 exported functions can just call one general verify function with different parameters depending on the ABI function that was called. This makes the C interface much simpler, no need to serialize objects / structs across ABI if we can do it with simple types.

Sure, it's possible to create 3 functions instead. I'm not sure if that's simpler, you trade 1 complexity for another.

With arrays I see it similarly:

cSeed := C.CBytes(seed)
defer C.free(cSeed)

// len(seed) is arguably not needed because we can already check on the Go side that it is 32 bytes
// and return an error if it is not
C.verify(..., cSeed, len(seed), ...)

seems easier and is safer than:

cSeed := C.ArrayU8{
	ptr: (*C.uchar)(unsafe.SliceData(seed)),
	len: C.size_t(len(seed)),
	cap: C.size_t(cap(seed)),
}
C.verify(..., cSeed, ...)

I'm not even sure that the 2nd one is necessarily faster than the first one 🤔

Avoiding a copy would be faster, but ArrayU8 is not about performance. Originally, it was required to be able to pass an object allocated on the Rust side to Go and to be able to free it afterward (hence it must contain cap). It's not required to pass data from Go to Rust - it was used as a convenience as the type was already there.

Additionally, in situations when it is needed to pass 2 slices, it's better to pass 2 objects that contain ptr + len rather than ptr0, len0, ptr1, len1 as the former is less error-prone.