spacemeshos / post

Spacemesh POST protocol implementation
MIT License
20 stars 21 forks source link

Distributed verification #256

Closed poszu closed 9 months ago

poszu commented 10 months ago

Changes to support distributed verification (https://github.com/spacemeshos/go-spacemesh/issues/5185):

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

codecov[bot] commented 10 months ago

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 :warning:
internal/postrs/proof.go 93.4% 3 Missing and 1 partial :warning:
internal/postrs/pos_verifier.go 72.7% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

poszu commented 9 months ago

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.