Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Finalize the latest epoch #1082
Finalize the latest epoch #1082
Changes from all commits
b5bdaad
c083aa1
f26af23
b081918
f765b51
7dbf515
aa7a2f7
b496602
47edbaa
5393210
149fb96
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I changed
DepositExists()
toGetActiveFinalizers().empty()
is becauseDepositExists()
has a bit different meaning. It checks that we have deposits in current and previous dynasty. However,GetActiveFinalizers
checks that there is a finalizer in current or previous dynasty. If finalizer has deposited in one of the dynasties, it can vote and participate in finalization. So, if we keptDepositExists()
we have "race-condition" were at the same to systems are in place, instant justification and votes. By changing this, we have always 1 mechanism, either instant justification or voting.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit misleading that this variable is named
to_be_finalized
but we finalize the next epochm_current_epoch - 1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name was changed from
prev_justified
to this after discussing in the following thread #1082 (comment)I am fine to change it again if we have a consensus :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gnappuraz's comment doesn't actually make sense to me.
prev_justified
seems to be closer to the purpose of this variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should't
GetCheckpoint(prev_justified)
become finalized as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the change proposed here: https://github.com/dtr-org/unit-e/pull/1083/files#diff-924113d2ba8b7131c0af6bb2fe3c1d75R1088 it's finalized, but I suggest keeping data consistent as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frolosofsky I think it's OK to keep previous epoch only justified. It's nice that we can look back and see if we reached finalization at that point or not. Otherwise, after we reached finalization, we should go through all checkpoints until the current one and mark them finalized, but I don't think it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, honestly. You've already rewritten comments about justified->finalized because they were wrong, and why not change that in the code? Having #1083 merged will strictly limit amount of checkpoints.
(nit and up to you, I understand that this code works)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment wasn't wrong but I changed as it seemed confusing.
Eventually, I'll delete all checkpoints, but since it's harder to do, we have them. My point is that every FinalizationState (or checkpoints while we still have them) contains the information which was valid/up to date at the time this state was created/(considered the current one), and we shouldn't override old data (previous states) when we started to create new ones. Currently, this is true (I spot only 1 case when this rule is violated in FinalizationState but will fix it separately).
I believe that relying on this immutability in this particular case will give us a safer code as it easier to maintain/less risky to unsynchronize. For instance, let's say we made old checkpoints
is_finalized=true
but then if we retrieveFinalizationState
for these checkpoints, they won't haveis_finalized=true
flag.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is the revert from a0267c3
I misunderstood the meaning of this function as we didn't have tests for reward (I could adjust the code that the whole deposit would be burnt and all tests would pass). But now there is a test that covers finalizer reward so I return this block back.