Elmattic/actors review F12

[elmattic]

Alex's concern is in the builtin actor's miner:

https://github.com/chainsafe/forest/blob/ccf1ac118aefe7143b0a97a671f241e052f79343/vm/actor/src/builtin/miner/mod.rs#L674-L686

Rust actors explicitly validate the original RLE within actor methods, before calling bitfield methods.

I think this is ok, because the validation is also performed by the Go code, albeit less directly:

https://github.com/filecoin-project/specs-actors/blob/55d30b231a9c91b6bc93d365d9f88ac3331dad9e/actors/builtin/miner/miner_actor.go#L870-L872

All the validation code is happening inside the Count method:

https://github.com/filecoin-project/go-bitfield/blob/04a398a6926052723d217172acee28c388c4e57e/bitfield.go#L313-L319

Which call RunValidator method that performs the validation:

https://github.com/filecoin-project/go-bitfield/blob/04a398a6926052723d217172acee28c388c4e57e/bitfield.go#L248

https://github.com/filecoin-project/go-bitfield/blob/04a398a6926052723d217172acee28c388c4e57e/rle/rleplus.go#L48-L60

On the other hand, RLE+ validation was not fully done inside BitField::from_bytes which could have resulted in chain splits.

Summary of changes
Changes introduced in this pull request:

• Add test when version is incorrect
• Fix validation rules for varint not minimally encoded
• Add test when a varint is too big (10+ bytes)
• Fix u64 overflow of total running length
• Add test when a varint takes one byte
• Add test when a varint stores zero
• Add tests related to uniqueness of encoding
• Fix invalid error code in prove_commit_aggregate

Reference issue to close (if applicable)

Closes #1276

Other information and links

A question we should ask to PL folks about uniqueness of RLE+ encoding:
https://spec.filecoin.io/appendix/data_structures/

As of today Forest, Lotus or Fuhon accept different ways to encode BitField data, is this ok?

IOW a running length of 1 can be encoded either in a Block Single, Block Short or Block Long.

Other things to mention:

Storing a u64 into a usize will not work on a 32-bit target. I'm proposing to fix this in another PR.

[noot]

looks good to me! just curious, did you find the test vectors somewhere, or did you make them yourself?

[elmattic]

looks good to me! just curious, did you find the test vectors somewhere, or did you make them yourself?

I have crafted them myself. But I think it would be nice to check what exactly the other implementations are doing. Here are the tests for Go.