Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ConsensusContext.CreateContext() #3824

Open
greymistcube opened this issue Jun 13, 2024 · 0 comments
Open

Fix ConsensusContext.CreateContext() #3824

greymistcube opened this issue Jun 13, 2024 · 0 comments

Comments

@greymistcube
Copy link
Contributor

There are numerous issues related to this problem.

  • CreateContext() can create a Context with a wrong ValidatorSet
    if (_blockChain.Tip.ProtocolVersion
    < BlockMetadata.SlothProtocolVersion)
    {
    validatorSet = _blockChain
    .GetWorldState(_blockChain[Height - 1].StateRootHash)
    .GetValidatorSet();
    }
    • CreateContext() can be called with any height that is greater than or equal to Height, meaning it is impossible to know the ValidatorSet for given height if height is greater than Height.
  • HandleMessage() can lock the entire ConsensusContext
    public bool HandleMessage(ConsensusMsg consensusMessage)
    {
    long height = consensusMessage.Height;
    if (height < Height)
    {
    _logger.Debug(
    "Discarding a received message as its height #{MessageHeight} " +
    "is lower than the current context's height #{ContextHeight}",
    height,
    Height);
    return false;
    }
    lock (_contextLock)
    {
    if (!_contexts.ContainsKey(height))
    {
    _contexts[height] = CreateContext(height);
    }
    _contexts[height].ProduceMessage(consensusMessage);
    return true;
    }
    }
    • When ConsensusMsgs with "future" heights are received, it gets funneled to HandleMessage(), and this would result in a lock until the first call to GetNextStateRootHash() is resolved.

It is hard to know the full extent of the problem as this may cause issues on very non-trivial edge cases. The issue is farther exacerbated by having the value of Height becoming further asynchronous with the state of BlockChain the ConsensusContext is referencing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant