feat(commp): add binary unmarshall/marshall

[hannahhoward]

Goals

Support binary marshalling of hash calculation state, so that you can pause/resume a calculation. This is supported on sha256 and the simd version used in this package.

It's certainly a bunch of complexity so no pressure to merge, but we needed it.

Also I'm a long time contributor but should I really still have commit privileges to all the filecoin-project repos? Cause maybe there are people like me who are now hostile actors? or people who could get hacked?

Implementation

• modify the channel to handle an abstract message type
• implement a system to extract the current twinfold state
• implement a handling a large buffer when there is already state in the queues
• implement marshall/unmarshal with a cbor gen struct

For discussion

I'm pretty sure this is the first time ANYONE has worked on this repo in a minute and the code is complicated. I recommend some additional docs and/or knowledge transfer. happy to pass on what I learned, @ribasushi is off to better pastures.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 58.46995% with 76 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.17%. Comparing base (fe57ba8) to head (ca84f1d).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/serialized_gen.go	32.94%	29 Missing and 28 partials ⚠️
commp.go	84.94%	9 Missing and 5 partials ⚠️
internal/gen/main.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   41.58%   46.17%   +4.58%     
==========================================
  Files           3        5       +2     
  Lines         392      444      +52     
==========================================
+ Hits          163      205      +42     
+ Misses        225      202      -23     
- Partials        4       37      +33     

Files with missing lines	Coverage Δ
internal/gen/main.go	0.00% <0.00%> (ø)
commp.go	73.75% <84.94%> (+4.68%)	⬆️
internal/serialized_gen.go	32.94% <32.94%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rvagg]

Permissions question is WIP (slow) here filecoin-project/github-mgmt#47, hard because there's basically never been a clean-up and we have so many teams contributing in the org so it's a bit tedious pulling all those threads together and figuring out canonical lists.

Will try and get someone, maybe me, to review this soon. Sadly the changes are big enough that it's going to be an investment.

[hannahhoward]

we don't need a merge... it's more just contribute back :).

but also: what concerns me is I have access to push and merge this PR without any additional approval, and that says to me a bunch of people have my access, and I can't imagine what someone with my level access could do to the network if they pushed and merged a PR to a core hashing algorithm that was malicious, either cause they had a grudge or their account got hacked. I know a lot of hashing is in rust code but I bet people have access there too.

[snissn]

kudos to attemps to clean up the tooling around commp! It's both fairly complex and fairly straightforward - complex because there's a lot of intricate details - simple because it's "just hashing something". Glad to see any activity towards improvement!

[ribasushi]

I am very much around, though the first time I heard about this ticket/effrot is from having me mentioned in a roundabout way as seen in the screenshot above.

Why the passive-aggressiveness?

[rvagg]

@ribasushi if you're willing to review this then I'd very much appreciate it. I too imagined that you'd moved on enough to not want to be bothered with this but it would be great if you could.

[ribasushi]

Functionally this works, and the implementation seems safe. The slowdown of the "tower rails" is about 2.5% which while not great is within acceptable range

Difficult to hit approve, because while the implementation works, it is not... ideal. Leaving notes in no particular order, take-it-or-leave-it.

• The interface{}-ification of the channel structure is unfortunate: you can seimply use nil or []byte{} (len 0) to signify a request for "soft-collapse"
• The dance around enquing/dequing the holds is pointless: just move the entire hold list to the state struct as [MaxLayers][]byte
• quadsEnqueued isn't actually useful internally, it is a leftover from an optimization pass. Kill it, and therefore remove it from serialization state
• The use of a cbor encoder is overkill. Simply lay out the contents as:
  • uint32 bitfield signifying which of the MaxLayers are populated
  • a popcount(bitfield) long list of [32]byte
  • trailing buffer if any
  • if you are worried about short-reads: throw a checksum/hash at the end
• there is a lot of code duplication of short (thus inlineable) functions