Log debug when receiving different height ballot

[Charleslee522]

Background

N of nodes: 4, TH: 3 When all four nodes are alive, error message is occurred every height because of fourth ballot. Consensus is already done when a node receives only three ballots.

Solution

Make this log message Debug I think receiving different height ballot is not error condition.

[Charleslee522]

Instead of logging in ISAAC, is there reason to return log funs?

@spikeekips Oh, that is also good. I just wanted to minimize code changes. Is this better? It's good I think :)

func (is *ISAAC) IsValidVotingBasis(basis voting.Basis, latestBlock block.Block) bool {
    is.RLock()
    defer is.RUnlock()

    log := is.log.New(logging.Ctx{
        "ballot-basis":        basis,
        "voting-basis":        is.LatestVotingBasis(),
        "latest-block-height": latestBlock.Height,
        "latest-block-hash":   latestBlock.Hash,
    })

    if basis.Height != latestBlock.Height {
        log.Debug(
            "voting basis is invalid",
            "reason", "basis' height is different from latest block's height",
        )

        return false
    }
    if is.isInitRound(basis) {
        return true
    }

    // Note: If we have same height but different hash,
    // consensus is probably dead
    if basis.BlockHash != latestBlock.Hash {
        log.Error(
            "voting basis is invalid",
            "reason", "basis' block hash does not match latest block's hash",
        )
        return false
    }

    lvb := is.LatestVotingBasis()

    if basis.Height == lvb.Height {
        if basis.Round <= lvb.Round {
            log.Error(
                "voting basis is invalid",
                "reason", "basis' round is <= to latest voting basis' round",
            )
            return false
        }
    }
    return true
}

[spikeekips]

@Charleslee522 I also think logging inside in ISAAC looks good.

[Charleslee522]

@spikeekips Thanks for good idea :)

[Charleslee522]

@spikeekips I applied your review :) @Geod24 @anarcher ping~~~

[Geod24]

I originally suggested not logging inside of this function because we should be able to call it without triggering side effects. As for the change, should this validation even be done if we already have consensus ?

[Charleslee522]

I originally suggested not logging inside of this function because we should be able to call it without triggering side effects.

@Geod24 In this pr change, which part is side effect? Did you mean logging?

As for the change, should this validation even be done if we already have consensus ?

Sorry, I didn't completely understand it :)

[Charleslee522]

@Geod24 ping

[Charleslee522]

@Geod24 Can I merge it? :)

[Geod24]

Did you mean logging?

Yes

Sorry, I didn't completely understand it :)

If I'm not mistaken, by the time we do this validation, we can already know we have consensus and do not need to check the 4th node ?

[Charleslee522]

Did you mean logging?

Yes

@Geod24 What is the problem with this side effect?

Sorry, I didn't completely understand it :)

If I'm not mistaken, by the time we do this validation, we can already know we have consensus and do not need to check the 4th node ?

Yes we can know that this ballot has already been got consensus. Accurately, if latestHeight is 10, and voting basis from the ballot is 9, then this means that 'the ballot is useless'. But I think it's not an error case.

[Geod24]

@Geod24 Can I merge it? :)

Yes if you feel like the other comments don't need addressing (besides the documentation)