Venus: 0.5.6 fork attack - the weight of heaviest chain should reflect the most storage provided.

[taoshengshi]

Describe the bug

To Reproduce
Steps to reproduce the behavior:
Just debug on current user devnet 0.5.6.

Screenshots

This could cause the user devnet be hijacked easily.
I think this is the emergency issue for the dev net security from testing purpose.

Version information
go-filecoin 0.5.6

Additional context
NA.

[ZenGround0]

@taoshengshi thanks for filing this. From your image it is not clear what the issue is. I believe you are saying these are two different forks one with less storage than the other, but from the data all I see is that the longer chain has a higher weight which is roughly expected.

It would be very helpful to get further descriptions of the two chains. To start with: total committed sectors on each, number of blocks/nullblocks on each, height at which they forked.

[sprwig]

This problem needs to be taken seriously. When the main-net launch, if the customer can't determine whether their data is stored for a long time, they will be very anxious.

[taoshengshi]

@ZenGround0 The debug level did not trigger on in advance , so I can not identify the details in logs. You could reference below summary data.

total storage power goes down after fork attack:

[ElecRoastChicken]

Please solve the forking problem as soon as possible, the current network prevents miners from participating in the devnet!

[ZenGround0]

Thanks for your help and expressing your concern @ElecRoastChicken @sprwig @taoshengshi. This is certainly not taken lightly. It needs to be clear that we absolutely will not allow major security concerns like this to exist for mainnet launch. Please keep in mind that this is likely a protocol level concern and is potentially captured by existing work done by the research team that hasn't made it into the spec/implementation as of yet.

I'll be investigating how t2xiji234c3zomgql5k2pr77mbvagumqopou6ncri is going about increasing their weight to reorg to lower heights and understanding how this is impacting the power of other miners.

If you are reading this and think your operation might be responsible for this behavior feel free to explain what you are doing in the comments below. Regardless of whether you come forward, thanks for giving us a legitimate stress test and helping us improve the network.

[ZenGround0]

I'm going to draft up a gist exploring the issue in more detail. The tl;dr of my current hypothesis is that a miner with enough power can mine their own private chain (low weight because lots of null blocks), slash everyone else's power away (because their posts aren't on the private chain) and then mine every single block (higher weight than main chain which naturally has some null blocks).

This scenario is a large motivator of the weighting function spec's recent improvements. Particularly the P term punishes null blocks more harshly and the X term rewards total storage more strongly. go-filecoin is lagging behind the spec and does not have these terms.

We'll be validating this hypothesis and writing this up in more detail. We'll also be evaluating upgrading the protocol to use the spec's weighting function in the near future to bring back availability to the alphanet.

[ridewindx]

@ZenGround0 I believe that the fork attack will be mitigated after the weighting function in specs is correctly implemented. But the formula has no mathematical proof, so we don't know how much security it has. Most of all, the convergence rate or finality of chain should be taken seriously.

[ridewindx]

Expect:
A <- B <- C <- null <- D
---------- C1 <- null <- null //-- fork give up

Now:
A <- B <- C <- null <- D
---------- C1 <- null <- null <- null <- D1 //-- fork win

After weighting function in specs is completely implemented:
A <- B <- C <- null <- D
---------- C1 <- null <- null <- null <- null <- null <- null <- D1 //-- fork still win

Would this situation happen?

[taoshengshi]

Expect:
A <- B <- C <- null <- D
---------- C1 <- null <- null //-- fork give up

Now:
A <- B <- C <- null <- D
---------- C1 <- null <- null <- null <- D1 //-- fork win

After weighting function is implemented:
A <- B <- C <- null <- D
---------- C1 <- null <- null <- null <- null <- null <- null <- D1 //-- fork still win

Would this situation happen?

@ridewindx hmm,...I do NOT think it is this situation, but is good example to reveal the root cause of weight computation in current implementation - that is, null or n blocks(any blocks but not all) in the reorg TipSet do NOT reflect all miners's power, and therefore publish weight accumulation in parent weight - so ,this make attacker could quickly accumulate weight and at the reorg point‘s attacker 's TipSet have more proportion in weight . I guess @ZenGround0 's Particularly the P term punishes null blocks more harshly and the X term rewards total storage more strongly. express the same meaning.

[ridewindx]

@taoshengshi It seems that you don't get the meaning of my figure above. I mean adjusting the coefficients of the weighting function may not solve the problem completely. The security and correctness of the weighting should be formulated and proved in math.

[ridewindx]

In my opinion, it would be worth a try that the number of consecutive null blocks should be limited.

For example, do not allow the big power miner to generate consecutive null blocks beyond a generated real block of some small power miner:

A <- B <- C <- null <- D // small-power miner generated a valid block
---------- C1 <- null <- null <- null <- null <- null <- null <- D1 //-- fork should be failed
------------------------- |
------------------------- since this null block is beyond the valid block D

This trick is succinct and obvious, so actually it needs no mathematical proof.
Moreover, it fully respects small power miners' work. Miner's power should be measured fairly in each individual block height, but not in several consecutive blocks.

[taoshengshi]

@ridewindx yeap,Now It is more clear on your illustration. There is tickets validation in function validateMining (consensus/expected.go:299) but no tickets validation in chain rerog . and I suggest to take more scenarios into validation at the point of reorg.

but I just agree in some what sense on coefficients adjusting of @ZenGround0 's statement, I am saying the extended meaning of Particularly the P term punishes null blocks more harshly and the X term rewards total storage more strongly. Actually I mean the algorithm should be changed to reflect all miners's power, that is main issue should be addressed(of course should be formulated and proved in math).

// Compute parent weight.
    parentW, err := ts.ParentWeight()
    if err != nil {
        return uint64(0), err
    }

    w, err := types.FixedToBig(parentW)
    if err != nil {
        return uint64(0), err
    }
    // Each block in the tipset adds ECV + ECPrm * miner_power to parent weight.
    totalBytes, err := c.PwrTableView.Total(ctx, pSt, c.bstore)
    if err != nil {
        return uint64(0), err
    }
    floatTotalBytes := new(big.Float).SetInt(totalBytes.BigInt())
    floatECV := new(big.Float).SetInt64(int64(ECV))
    floatECPrM := new(big.Float).SetInt64(int64(ECPrM))
    for _, blk := range ts.ToSlice() {
        minerBytes, err := c.PwrTableView.Miner(ctx, pSt, c.bstore, blk.Miner)
        if err != nil {
            return uint64(0), err
        }
        floatOwnBytes := new(big.Float).SetInt(minerBytes.BigInt())
        wBlk := new(big.Float)
        wBlk.Quo(floatOwnBytes, floatTotalBytes)
        wBlk.Mul(wBlk, floatECPrM) // Power addition
        wBlk.Add(wBlk, floatECV)   // Constant addition
        w.Add(w, wBlk)
    }

look into above code, put what I said again and attach two picture to illustrate:
that is, null or n blocks(any blocks but not all) in the reorg TipSet do NOT reflect all miners's power, and therefore publish weight accumulation in parent weight - so ,this make attacker could quickly accumulate weight (do selfish mining off the main chain) and at the reorg point‘s attacker 's TipSet have more proportion in weight .

weight increase 110 for each block(do selfish mining off the main chain):

weight increase 48.429 for each block(main chain):

[ZenGround0]

@merced getting the weight function up to spec is probably the quickest viable fix out there. Unfortunately the team is gathering for a week of planning right now so updating the network is on pause as we don't want to hastily ship something in the next few hours that we won't have time to vet and or fix even worse bugs introduced during the fix. I 100% agree it is a bad thing that we aren't shipping a fix sooner but this is best we can do under constraints.

@ridewindx I love that you are asking these questions. I too am not convinced the weighting function is yet where it needs to be (this is part of the reason we didn't chose to implement it in go-filecoin as we were hoping to take in all improvements at once). I will take the time to read through and internalize your scenarios in detail and raise these concerns. Also know that in the future bringing things like this up to @sternhenri directly in the filecoin-project/specs or filecoin-project/consensus repos is the most direct way to get us to address them.

[sternhenri]

Thanks! I'm indeed at your disposal @merced, @ridewindx.

The idea you suggest with regards to consecutive nulls is indeed one we've had (and specc'd out). In fact, the form it'll take will be penalizations based on distance from expected number of blocks (and not just null blocks which is a specific case within that).

Feel free to check out work done here on this and comment accordingly!

Henri

[anorth]

We'll attempt a protocol upgrade to replace the weight function: #3540