liftedinit / manifest-ledger

CosmosSDK-based blockchain ledger for the Manifest Network
Apache License 2.0
0 stars 2 forks source link

Stakeholder payout is non-deterministic #48

Closed fmorency closed 2 months ago

fmorency commented 2 months ago

The chain invariant is currently broken because, as specified in https://github.com/liftedinit/manifest-ledger/security/code-scanning/17, map iteration are non-deterministic in Go.

E.g., running

package main
import "fmt"
func main() {
    m := map[string]int{
        "one":   1,
        "two":   2,
        "three": 3,
    }
    for key, value := range m {
        fmt.Println(key, value)
    }
}

multiple times may result in a different output, e.g.,

➜  map go run ./main.go 
three 3
one 1
two 2
➜  map go run ./main.go
one 1
two 2
three 3

Those cases are currently not detected in our e2e test suite because tests are run against a single validator. I was able to easily break consensus by modifying the stakeholder payout test in https://github.com/liftedinit/manifest-ledger/pull/47. You'll need to run the test multiple times as the test doesn't always break (~1 run in 5 with 2 validators and 3 stakeholders).

A failing run will show the following in the CometBFT logs

ERR prevote step: consensus deems this block invalid; prevoting nil err="wrong Block.Header.LastResultsHash.  Expected 0D6CC242AE0E189389EAFA9F417F0A1850B1E8928FB9B866A0D9D08B65833954, got 38E94F9F95CCC034DA44BA4AB91360B7F838DD542883DF53D40528ADA511084F" height=20 module=consensus round=1

Failing run example in CI: https://github.com/liftedinit/manifest-ledger/actions/runs/8804787416/job/24166242952

I haven't time to investigate if more of the codebase is affected but I plan to do so.

CC @Reecepbcups @chalabi2

Reecepbcups commented 2 months ago

This is odd since we are not breaking early within the map, and using all values. Regardless of order

Good catch. I assume the coins need to be appended to a slice, then paid all at once rather than during each iter.