Use quorums N-MAX_REORG_BLOCKS_POST_HF12 for checkpointing #721
Conversation
Nodes on an alternative chain will generate quorums reflecting those on the alternate chain. Since checkpointing prevents reorgs up to MAX_REORG_BLOCKS_POST_HF12, use quorums exactly that many blocks old to ensure that at any point, all nodes will be working in quorums that are consistent between each other.
| @@ -311,15 +308,19 @@ namespace service_nodes | |||
| case quorum_type::checkpointing: | |||
| { | |||
| m_last_checkpointed_height = std::max(start_voting_from_height, m_last_checkpointed_height); | |||
|
|
|||
| for (m_last_checkpointed_height += (m_last_checkpointed_height % CHECKPOINT_INTERVAL); | |||
There was a problem hiding this comment.
This initial mutating addition doesn't look right. If the last checkpoint height is something that isn't already a multiple of CHECKPOINT_INTERVAL (say 100003) then this is going to add 3 and give 100006 which still isn't a proper checkpoint height. And if it is a multiple of CHECKPOINT_INTERVAL already (say 100000) then it seems as though it is going to add 0 and end up processing a checkpoint that has already been handled?
I think this should instead be: m_last_checkpointed_height += (CHECKPOINT_INTERVAL - m_last_checkpointed_height % CHECKPOINT_INTERVAL), but perhaps I'm missing something.
There was a problem hiding this comment.
I've made this error with modulo so many times now, when will I learn.
| @@ -197,7 +194,7 @@ namespace service_nodes | |||
| break; | |||
|
|
|||
| m_obligations_height = std::max(m_obligations_height, start_voting_from_height); | |||
| for (; m_obligations_height < (height - REORG_SAFETY_BUFFER_IN_BLOCKS); m_obligations_height++) | |||
| for (; m_obligations_height < (height - MAX_REORG_BLOCKS); m_obligations_height++) | |||
There was a problem hiding this comment.
I like the "safety buffer" name much better. The "max reorg" name implies that this is somehow a limit, but as far as I can see it isn't (and mustn't be!): it is merely the target maximum implied by a checkpointing mechanism working in normal times. It is still possible for checkpoints to be missed and for this maximum to be exceeded; it's a rare case that shouldn't normally happen, but it is something we need to handle (even if suboptimally).
There was a problem hiding this comment.
You're right, I forgot about missed checkpoints leading to > the usual number of blocks reorganisable.
| @@ -311,15 +308,19 @@ namespace service_nodes | |||
| case quorum_type::checkpointing: | |||
| { | |||
| m_last_checkpointed_height = std::max(start_voting_from_height, m_last_checkpointed_height); | |||
|
|
|||
| for (m_last_checkpointed_height += (m_last_checkpointed_height % CHECKPOINT_INTERVAL); | |||
There was a problem hiding this comment.
I've made this error with modulo so many times now, when will I learn.
| @@ -197,7 +194,7 @@ namespace service_nodes | |||
| break; | |||
|
|
|||
| m_obligations_height = std::max(m_obligations_height, start_voting_from_height); | |||
| for (; m_obligations_height < (height - REORG_SAFETY_BUFFER_IN_BLOCKS); m_obligations_height++) | |||
| for (; m_obligations_height < (height - MAX_REORG_BLOCKS); m_obligations_height++) | |||
There was a problem hiding this comment.
You're right, I forgot about missed checkpoints leading to > the usual number of blocks reorganisable.
Doy-lee
force-pushed
the
CheckpointFromTailMinusNBlocks
branch
from
f18d765
to
6205a88
Compare
Nodes on an alternative chain will generate quorums reflecting those on
the alternate chain. Since checkpointing prevents reorgs up to
MAX_REORG_BLOCKS_POST_HF12, use quorums exactly that many blocks old to
ensure that at any point, all nodes will be working in quorums that are
consistent between each other.
This will cause a fork most likely on testnet. @jagerman