From 5e95017cf172725dd1e650b8b14059937a0f6d8e Mon Sep 17 00:00:00 2001 From: Bui Quang Minh Date: Wed, 4 Sep 2024 11:08:26 +0700 Subject: [PATCH] consortium-v2: only check the current block in GetBestParentBlock In commit 94bcf95e230b ("fix: miner does not mine its inturn block"), we make the miner go backward in the chain to find if it can create an inturn block which makes the chain has high difficulty instead of just creating the new block on the current block. However, after finality is introduced, this scenario can happen Current chain 10 <- 11 (diff: 3) <- 12 (diff: 3) And the miner can create block 11 with difficulty 7. However, block 12 has enough finality vote to justify block 11 already. So if the miner still tries to create block 11 with difficulty 7, that block will be rejected because the old chain has higher justified block number (11 vs maximum 10) and this miner does not participate to make a new block in this scenario. This is undesirable as this miner can be viewed as down even though it is still online. PR https://github.com/axieinfinity/ronin/pull/422 tries to address the above issue but it reverts the purpose of commit 94bcf95e230b when the TODO is not resolved. This commit tries to simplify the logic of GetBestParentBlock to resolve the issue. GetBestParentBlock does not go backward and tries to revert more than 1 block anymore, it only tries to revert 1 block (i.e. the current block) if possible. With the above example, the miner only tries to revert block 12 if it can create a new block 12 with difficult 7 and does not try to revert block 11. It works with the assumption that in case the current block 12 with difficulty 3 justify block 11, it means there are enough finality votes for block 11 and the newly created block 12 with difficulty 7 can justify block 11 too. So justified block number in the newly created chain is the same as the old one but the new one will have higher difficulty. --- consensus/consortium/v2/consortium.go | 31 ++++++++++++--------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/consensus/consortium/v2/consortium.go b/consensus/consortium/v2/consortium.go index 7a4a11252..efad3e053 100644 --- a/consensus/consortium/v2/consortium.go +++ b/consensus/consortium/v2/consortium.go @@ -1474,33 +1474,30 @@ func (c *Consortium) readSignerAndContract() ( return c.val, c.signFn, c.signTxFn, c.contract } -// GetBestParentBlock goes backward in the canonical chain to find if the miner can -// create a chain which has more difficulty than current chain. In case the miner -// cannot create a better chain, this function returns the head block of current -// canonical chain. +// GetBestParentBlock looks at the current block to see if the miner can create +// higher difficulty block than the current block. If it can, GetBestParentBlock +// returns the the parent of current block and true. Otherwise, this function +// returns current block and false. func (c *Consortium) GetBestParentBlock(chain *core.BlockChain) (*types.Block, bool) { - signer, _, _, _ := c.readSignerAndContract() - currentBlock := chain.CurrentBlock() - block := currentBlock - prevBlock := chain.GetBlockByHash(block.ParentHash()) - diffculty := block.Difficulty().Int64() - for diffculty < diffInTurn.Int64() { - snap, err := c.snapshot(chain, block.NumberU64()-1, block.ParentHash(), nil) + if currentBlock.Difficulty().Int64() < diffInTurn.Int64() { + snap, err := c.snapshot(chain, currentBlock.NumberU64()-1, currentBlock.ParentHash(), nil) if err != nil { return currentBlock, false } // Miner can create an inturn block which helps the chain to have - // greater diffculty + // higher diffculty + signer, _, _, _ := c.readSignerAndContract() if snap.supposeValidator() == signer { if !snap.IsRecentlySigned(signer) { - return prevBlock, true + parentBlock := chain.GetBlockByHash(currentBlock.ParentHash()) + // This must never happen, still check for safety + if parentBlock == nil { + return currentBlock, false + } + return parentBlock, true } } - - block = prevBlock - prevBlock = chain.GetBlockByHash(block.ParentHash()) - diffculty += block.Difficulty().Int64() } return currentBlock, false