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 synchrony constraint calculation when latest block is older than approved #3269

Merged

Conversation

tgrospic
Copy link
Collaborator

Overview

When validator propose a block it must satisfy synchrony constraint which depends on reading active validators from tuple space. When LFS state is downloaded and validator latest block is older then approved block, we don't have tuple state for that block. This PR adds this detection and allow validator to propose.

Another small fix to Engine initialization. The bug manifested when LFS node received the state, LFS exporter was not enabled although the flag was set.

Please make sure that this PR:

Bors cheat-sheet:

  • bors r+ runs integration tests and merges the PR (if it's approved),
  • bors try runs integration tests for the PR,
  • bors delegate+ enables non-maintainer PR authors to run the above.

approvedBlockNumber = ProtoUtil.blockNumber(approvedBlock)
latestBlockBehindApproved = approvedBlockNumber > lastProposedBlockMeta.blockNum

allowedToPropose <- if (latestBlockBehindApproved) true.pure[F] else checkConstraint
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to just skip active validators filtering instead of allow without checking. @nzpr can we do this or this will bring the old problem again resolved in PR #3069?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgrospic I feel that using lastProposedTuplespace for fetching active validators is not correct - you have to use state that you are trying to propose on top of, so, basically, state of the main parent. Wdy think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nzpr makes sense to use main parent. Do you know why is state from latest block used now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgrospic not really, I don't see why its more preferable then main parent.

@@ -17,8 +17,8 @@
</appender>

<logger name="coop.rchain.shared.EventLogger" level="OFF" />
<logger name="coop.rchain.rspace" level="info" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enable info messages from RSpace.

@tgrospic tgrospic marked this pull request as ready for review December 16, 2020 11:16
@tgrospic tgrospic requested review from nzpr and zsluedem and removed request for nzpr December 16, 2020 11:16
@tgrospic tgrospic added the feature-branch Development parallel to dev branch label Dec 16, 2020
@tgrospic tgrospic changed the title Fix synchrony constraint calculation when latest block is older then approved Fix synchrony constraint calculation when latest block is older than approved Dec 16, 2020
@tgrospic tgrospic mentioned this pull request Dec 16, 2020
5 tasks
Copy link
Collaborator

@nzpr nzpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tgrospic tgrospic merged commit a167048 into rchain:feature/last-finalized-state Dec 17, 2020
activeValidators <- runtimeManager.getActiveValidators(mainParentStateHash)

// Validators weight map filtered by active validators only.
validatorWeightMap = lastProposedBlockMeta.weightMap.filter {
Copy link
Collaborator

@nzpr nzpr Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overlooked this, but I think this also should be main parent weight map. Not sure if this is crucial though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are using active validators from main parent and use in filter from the latest block. I'll create a fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix in PR #3274.

}
// Guaranteed to be present since last proposed block was present
seenSenders <- calculateSeenSendersSince(lastProposedBlockMeta, dag)
sendersWeight = seenSenders.toList.flatMap(validatorWeightMap.get).sum
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nzpr I'm not sure is it possible here that validatorWeightMap doesn't have validator calculated as seen sender?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nzpr nvm, this is already handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix feature-branch Development parallel to dev branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants