search

filecoin-project / specs-actors

DEPRECATED Specification of builtin actors, in the form of executable code.
Other
86 stars 102 forks source link

Order of removing faults is not deterministic #578

Closed Kubuxu closed 4 years ago

Kubuxu commented 4 years ago

We iterate over the map here: https://github.com/filecoin-project/specs-actors/blob/a5da78919b5055d2eadc023c6918447544cae8fc/actors/builtin/miner/miner_state.go#L607

Causing the order of fault removal to be non-deterministic which in turn causes state mismatches. See: https://gist.github.com/Kubuxu/408b35bbf42414032788c480b5f970d0

arajasek commented 4 years ago

We probably need to do an audit to make sure this doesn't happen anywhere else in actors code.

A very quick grep only pulled up this instance, which has a disclaimer about why it's okay (that probably holds, but could still afford to be moved to some ordered iterating to be safe).

Kubuxu commented 4 years ago

The issue is, that case is not okay. With gas in the mix, it has a possibility of causing a state miss-match also.

anorth commented 4 years ago

Right, the commented case is not ok, because it might abort at a different point.

whyrusleeping commented 4 years ago

aborting at a different point here isnt actually the issue, aborting always causes the same gas usage. The potential issue in the code @arajasek linked is that the difference in insertion order might cause the structure we're writing to to sharder earlier or later than it otherwise might, causing a different number of objects to have to be written. But I think thats likely not a problem either, unless this slightly suspicious put gets abused.

In any case, best to just force an ordered traversal.

Stebalien commented 4 years ago

Reopening as we need to audit all map iteration.