Finalize the latest epoch #1082

kostyantyn · 2019-05-07T16:56:44Z

When we have two consecutive justifications, finalize the latest one instead of the previous one.

Rationale
Let's say we have these two consecutive justified links.

                J
source epoch ------- target hash                                          
|                              |
e2[101, .., 150] e3[151, ... 200] e4[201, ... 250]
                 |                              |
                 source epoch ------- target hash                                          
                                 J

In Casper the finalized epoch would be e4 and in our case it's e3. However, we proved in feature_fork_choice_forked_finalize_epoch.py that reorging e4, in this case, is not possible anymore as it will revert finalization for e3. Because of that we introduced FinalizationState::GetCheckpointHeightAfterFinalizedEpoch as a point of "not going back". However, if we count e4 as finalized, we can drop this extra entity. Moreover, we used to have such a convention and we changed but as it turned out wasn't a good idea.

Aditional fix
CalculateWithdrawAmount was broken as it didn't correctly take reward into account which led to a shrunk amount over time. We didn't have tests to catch it. Now we have finalizationstate_calculate_withdraw_amount_tests.cpp test that tests this part and shows that the amount is properly increasing.

However the finalization_withdraw.py still shows that after withdraw the balance is not larger than the initial deposit (it's actually equal to initial deposit - fee). The reason is that the reward for finalizers is not implemented and I'll continue working on it in the following PR.

Resolves #1092

Signed-off-by: Kostiantyn Stepaniuk [email protected]

src/esperanza/finalizationstate.cpp

src/Makefile.test.include

src/esperanza/finalizationstate.cpp

src/test/esperanza/finalizationstate_calculate_withdraw_amount_tests.cpp

src/test/esperanza/finalizationstate_utils.cpp

test/functional/rpc_preciousblock.py

kostyantyn · 2019-05-08T12:56:35Z

src/esperanza/finalizationstate.cpp

    m_reward_factor = 0;
  }

+  IncrementDynasty();
+
+  if (GetActiveFinalizers().empty()) {


The reason I changed DepositExists() to GetActiveFinalizers().empty() is because DepositExists() 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 kept DepositExists() 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.

kostyantyn · 2019-05-08T12:59:22Z

src/esperanza/finalizationstate.cpp

@@ -199,7 +199,7 @@ ufp64::ufp64_t FinalizationState::GetCollectiveRewardFactor() {
 }

 bool FinalizationState::DepositExists() const {
-  return m_cur_dyn_deposits > 0;
+  return m_cur_dyn_deposits > 0 && m_prev_dyn_deposits > 0;


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.

frolosofsky

ConceptACK. I think this proposal moves us closer to initial Casper idea.

I went through the changes, most of them are monotonous one-lines that replaces numbers, so I could miss something.

frolosofsky · 2019-05-08T12:54:03Z

src/esperanza/finalizationstate.cpp

-      m_last_finalized_epoch = expected_finalized;
+    uint32_t prev_justified = m_current_epoch - 2;
+    if (GetCheckpoint(prev_justified).m_is_justified) {
+      cp.m_is_finalized = true;


Should't GetCheckpoint(prev_justified) become finalized as well?

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.

@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.

@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.

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)

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 retrieve FinalizationState for these checkpoints, they won't have is_finalized=true flag.

src/esperanza/finalizationstate.cpp

src/test/esperanza/finalizationstate_utils.cpp

src/test/finalization/state_repository_tests.cpp

test/functional/feature_fork_choice_parallel_justifications.py

test/functional/rpc_preciousblock.py

src/esperanza/finalizationstate.cpp

test/functional/finalization_vote.py

test/functional/finalization_withdraw.py

src/esperanza/finalizationstate.h

src/esperanza/finalizationstate.cpp

Gnappuraz

utACK 5f330a7

scravy · 2019-05-10T13:09:09Z

Concept ACK 5f330a7

When we have two consecutive justifications, finalize the latest one instead of the previous one. Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

src/test/esperanza/finalizationstate_calculate_withdraw_amount_tests.cpp

Nizametdinov · 2019-05-13T13:49:26Z

src/esperanza/finalizationstate.cpp

-    if (GetCheckpoint(expected_finalized).m_is_justified) {
-      GetCheckpoint(expected_finalized).m_is_finalized = true;
-      m_last_finalized_epoch = expected_finalized;
+    uint32_t to_be_finalized = m_current_epoch - 2;


I find it a bit misleading that this variable is named to_be_finalized but we finalize the next epoch m_current_epoch - 1.

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 :)

@Gnappuraz's comment doesn't actually make sense to me. prev_justified seems to be closer to the purpose of this variable.

src/test/finalization/state_processor_tests.cpp

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Nizametdinov

utACK 47edbaa

frolosofsky · 2019-05-14T08:39:19Z

ConceptACK 47edbaa

scravy

utACK 47edbaa

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Gnappuraz · 2019-05-15T09:07:52Z

Did a quick review, it looks ok, utACK
149fb96

kostyantyn added bug A problem of existing functionality wip Work in progress which is not supposed to be merged yet finalization labels May 7, 2019

This comment has been minimized.

Sign in to view

kostyantyn changed the title ~~Finalize the latest epoch~~ WIP: Finalize the latest epoch May 8, 2019

kostyantyn marked this pull request as ready for review May 8, 2019 08:13

kostyantyn closed this May 8, 2019

kostyantyn reopened this May 8, 2019

kostyantyn changed the title ~~WIP: Finalize the latest epoch~~ Finalize the latest epoch May 8, 2019

kostyantyn removed the wip Work in progress which is not supposed to be merged yet label May 8, 2019

kostyantyn requested review from a team, frolosofsky and Gnappuraz May 8, 2019 10:03

kostyantyn commented May 8, 2019

View reviewed changes

src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved

kostyantyn requested a review from Nizametdinov May 8, 2019 10:21

scravy reviewed May 8, 2019

View reviewed changes

kostyantyn commented May 8, 2019

View reviewed changes

frolosofsky reviewed May 8, 2019

View reviewed changes

Gnappuraz assigned kostyantyn May 8, 2019

Gnappuraz reviewed May 8, 2019

View reviewed changes

src/esperanza/finalizationstate.cpp Outdated Show resolved Hide resolved

kostyantyn force-pushed the rebased_finalize_latest_epoch branch from e707890 to 0f891e8 Compare May 9, 2019 16:54

Gnappuraz reviewed May 10, 2019

View reviewed changes

frolosofsky mentioned this pull request May 10, 2019

finalization_vote.py is floating #1092

Closed

Gnappuraz approved these changes May 10, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Kostiantyn Stepaniuk added 3 commits May 10, 2019 17:37

Finalize the latest epoch

b5bdaad

When we have two consecutive justifications, finalize the latest one instead of the previous one. Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Leave comments to not obvious calculations in the code

c083aa1

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Fix floating finalization_slash_itself.py

f26af23

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Kostiantyn Stepaniuk added 4 commits May 10, 2019 17:37

Test CalculateWithdrawAmount when finalizer sometimes votes

b081918

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Leave a comment why finalization is disabled

f765b51

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Disable test block

7dbf515

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Improvements after review

aa7a2f7

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

kostyantyn force-pushed the rebased_finalize_latest_epoch branch from 5f330a7 to aa7a2f7 Compare May 10, 2019 15:41

This comment has been minimized.

Sign in to view

scravy requested a review from frolosofsky May 10, 2019 15:42

This comment has been minimized.

Sign in to view

Fix lint

b496602

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Nizametdinov reviewed May 13, 2019

View reviewed changes

src/test/finalization/state_processor_tests.cpp Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Act upon reviews

47edbaa

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Nizametdinov approved these changes May 14, 2019

View reviewed changes

scravy approved these changes May 14, 2019

View reviewed changes

Kostiantyn Stepaniuk added 2 commits May 14, 2019 15:17

Fix finalization_state_restoration.py

5393210

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

Fix feature_reindex_commits.py

149fb96

Signed-off-by: Kostiantyn Stepaniuk <[email protected]>

kostyantyn merged commit a2abb66 into dtr-org:master May 15, 2019

kostyantyn deleted the rebased_finalize_latest_epoch branch May 15, 2019 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize the latest epoch #1082

Finalize the latest epoch #1082

kostyantyn commented May 7, 2019 •

edited

Loading

This comment has been minimized.

kostyantyn May 8, 2019

kostyantyn May 8, 2019 •

edited

Loading

frolosofsky left a comment

frolosofsky May 8, 2019

frolosofsky May 8, 2019 •

edited

Loading

kostyantyn May 8, 2019 •

edited

Loading

frolosofsky May 9, 2019

kostyantyn May 9, 2019

Gnappuraz left a comment

scravy commented May 10, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Nizametdinov May 13, 2019

kostyantyn May 13, 2019

Nizametdinov May 13, 2019

This comment has been minimized.

Nizametdinov left a comment

frolosofsky commented May 14, 2019

scravy left a comment •

edited

Loading

Gnappuraz commented May 15, 2019

Finalize the latest epoch #1082

Finalize the latest epoch #1082

Conversation

kostyantyn commented May 7, 2019 • edited Loading

This comment has been minimized.

kostyantyn May 8, 2019

Choose a reason for hiding this comment

kostyantyn May 8, 2019 • edited Loading

Choose a reason for hiding this comment

frolosofsky left a comment

Choose a reason for hiding this comment

frolosofsky May 8, 2019

Choose a reason for hiding this comment

frolosofsky May 8, 2019 • edited Loading

Choose a reason for hiding this comment

kostyantyn May 8, 2019 • edited Loading

Choose a reason for hiding this comment

frolosofsky May 9, 2019

Choose a reason for hiding this comment

kostyantyn May 9, 2019

Choose a reason for hiding this comment

Gnappuraz left a comment

Choose a reason for hiding this comment

scravy commented May 10, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Nizametdinov May 13, 2019

Choose a reason for hiding this comment

kostyantyn May 13, 2019

Choose a reason for hiding this comment

Nizametdinov May 13, 2019

Choose a reason for hiding this comment

This comment has been minimized.

Nizametdinov left a comment

Choose a reason for hiding this comment

frolosofsky commented May 14, 2019

scravy left a comment • edited Loading

Choose a reason for hiding this comment

Gnappuraz commented May 15, 2019

kostyantyn commented May 7, 2019 •

edited

Loading

kostyantyn May 8, 2019 •

edited

Loading

frolosofsky May 8, 2019 •

edited

Loading

kostyantyn May 8, 2019 •

edited

Loading

scravy left a comment •

edited

Loading