Fix fork parameters

[Varunram]

The current proposed fork code does nothing which would activate the fork defense mechanism proposed:

https://github.com/Musicoin/go-musicoin/blob/master/core/blockchain.go#L1050 calls checkChainForAttack and if the error is non zero (https://github.com/Musicoin/go-musicoin/blob/master/core/blockchain.go#L939), it prints out the error and does not act upon it. The fix would be to catch the error and return from there, so that block validation is stopped at that point.

To address this, isaac had a proposed fix in https://github.com/Musicoin/go-musicoin/commit/0d0918dee8ee02a056926e3cac13a5cfab99df6d, but it does nothing since we should error out the moment we detect that a given block is stale. On the fix itself, which seems to address the catching of errors, it overwrites the result of header verification in https://github.com/Musicoin/go-musicoin/blob/master/core/blockchain.go#L1045 to nil, and calls ValidateBody, which should not be called if header verification fails (and which would be in the proposed fix). This can be fixed by removing the proposed fix.

[masterdubs]

We don't validate the block and drop the peer. We also don't want to report the attack to the attacker. For his point of view he see nothing, so he don't know if succeed or failed.

[Varunram]

@masterdubs you don't catch the error either in the validating parameters or in the check itself, so the code as such runs the check and does not act upon it.

[GDHex]

errors.go has the err err = ErrDelayTooHigh is retuned in case of penalty you can act upon it after wards but like that it will still drop the attacking peer and reject the attackers chain

[Varunram]

@0x1337coding yes, the error is returned inside the function, when the function is called just prints the error and doesn't return from the validating function which it should.

[GDHex]

when err = errChain and yes you could return but also like that it will force the error causing the peer to drop and without a notice, the main chain will be informed though

[Varunram]

the main chain will be informed though

exactly the point for this issue, which part of the code drops the malicious block?

[Varunram]

There is no

force the error causing the peer to drop and without a notice

that is written in code like you claim.

[GDHex]

case err != nil: bc.reportBlock(block, nil, err) return i, events, coalescedLogs, err } if we pass all cases we end up here

[Varunram]

Yes, in this case, you would overwrite a bad header error which would have earlier been caught by https://github.com/Musicoin/go-musicoin/blob/master/core/blockchain.go#L1069 because you don't handle this either

    abort, results := bc.engine.VerifyHeaders(bc, headers, seals)

[GDHex]

https://github.com/Musicoin/go-musicoin/blob/c19417a9605739a5d6c99b496b48ba447abb1d72/core/blockchain.go#L1116 err = errChain could go there for a more cleaner appr

[Varunram]

It won't in the current approach because you set err to errChain. In the case that you allow it to fall through to the code pointed above, you're failing to catch the error returned from

receipts, logs, usedGas, err := bc.processor.Process(block, state, bc.vmConfig)

An easier fix is to add a case in https://github.com/Musicoin/go-musicoin/blob/c19417a9605739a5d6c99b496b48ba447abb1d72/core/blockchain.go#L1074 for errDelayTooHigh. This would allow the header validation error to be caught as well.

[GDHex]

👍 on that that would be the easy and more visible way yes

[immartian]

I like the stealthy strategy which is not only being difficult for attackers, but also giving a consistent design of modules via existing mechanism. Since it's also related to #98 , I expect this issue will be closed here and continue along that thread. Please PR your code and get it reviewed if any.