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.

Sign up for GitHub

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

Jump to bottom

Finalize the latest epoch #1082

Merged
merged 11 commits into from
May 15, 2019
Merged

Finalize the latest epoch #1082

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b5bdaad
Finalize the latest epoch
Apr 25, 2019
c083aa1
Leave comments to not obvious calculations in the code
May 8, 2019
f26af23
Fix floating finalization_slash_itself.py
May 8, 2019
b081918
Test CalculateWithdrawAmount when finalizer sometimes votes
May 9, 2019
f765b51
Leave a comment why finalization is disabled
May 9, 2019
7dbf515
Disable test block
May 9, 2019
aa7a2f7
Improvements after review
May 10, 2019
b496602
Fix lint
May 13, 2019
47edbaa
Act upon reviews
May 13, 2019
5393210
Fix finalization_state_restoration.py
May 14, 2019
149fb96
Fix feature_reindex_commits.py
May 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.test.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ UNITE_TESTS =\
test/esperanza/admincommand_tests.cpp \
test/esperanza/adminstate_tests.cpp \
test/esperanza/checks_tests.cpp \
test/esperanza/finalizationstate_calculate_withdraw_amount_tests.cpp \
test/esperanza/finalizationstate_tests.cpp \
test/esperanza/finalizationstate_deposit_tests.cpp \
kostyantyn marked this conversation as resolved.
Show resolved Hide resolved
test/esperanza/finalizationstate_vote_tests.cpp \
Expand Down
56 changes: 27 additions & 29 deletions src/esperanza/finalizationstate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,7 @@ Result FinalizationState::InitializeEpoch(blockchain::Height blockHeight) {
assert(m_settings.IsEpochStart(blockHeight) &&
"provided blockHeight is not the first block of a new epoch");

IncrementDynasty();

const uint32_t new_epoch = GetEpoch(blockHeight);

if (new_epoch != m_current_epoch + 1) {
return fail(Result::INIT_WRONG_EPOCH,
/*log_errors=*/true,
Expand Down Expand Up @@ -119,10 +116,22 @@ Result FinalizationState::InitializeEpoch(blockchain::Height blockHeight) {
}

} else {
InstaJustify();
m_reward_factor = 0;
}

IncrementDynasty();

if (GetActiveFinalizers().empty()) {
Copy link
Member Author

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() 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.

InstaJustify();
}

// new source is already known when the checkpoint is processed but we didn't
// update it as the previous one is used inside of this function. To not keep
// current and previous expected source in the FinalizationState, we update
// it here and the limitation of it is that we can't create vote inside the
// first block of new epoch as new source is not known yet for finalizers.
m_expected_source_epoch = m_last_justified_epoch;

std::string log_msg;
if (m_current_epoch >= 2 && m_last_justified_epoch != m_current_epoch - 2) {
log_msg = " epoch=" + std::to_string(m_current_epoch - 2) + " was not justified.";
Expand All @@ -146,10 +155,10 @@ void FinalizationState::InstaJustify() {
m_last_justified_epoch = m_current_epoch - 1;

if (m_current_epoch > 1) {
uint32_t expected_finalized = m_current_epoch - 2;
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;
Copy link
Member

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 epoch m_current_epoch - 1.

Copy link
Member Author

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

Copy link
Member

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.

if (GetCheckpoint(to_be_finalized).m_is_justified) {
cp.m_is_finalized = true;
scravy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

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?

Copy link
Member

@frolosofsky frolosofsky May 8, 2019 •
edited
Loading

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.

Copy link
Member Author

@kostyantyn kostyantyn May 8, 2019 •
edited
Loading

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.

Copy link
Member

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.

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)

Copy link
Member Author

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

m_last_finalized_epoch = m_last_justified_epoch;
}
}

Expand All @@ -159,7 +168,7 @@ void FinalizationState::InstaJustify() {

void FinalizationState::IncrementDynasty() {
// finalized epoch is m_current_epoch - 2 because:
// finalized (0) - justified (1) - votes to justify (2)
// finalized(0) - votes to justify(1) - m_current_epoch(2)

// skip dynasty increment for the hardcoded finalized epoch=0
// as it's already "considered" incremented from -1 to 0.
Expand All @@ -172,7 +181,7 @@ void FinalizationState::IncrementDynasty() {
m_current_dynasty += 1;
m_prev_dyn_deposits = m_cur_dyn_deposits;
m_cur_dyn_deposits += GetDynastyDelta(m_current_dynasty);
m_dynasty_start_epoch[m_current_dynasty] = m_current_epoch + 1;
m_dynasty_start_epoch[m_current_dynasty] = m_current_epoch;

for (auto it = m_checkpoints.begin(); it != m_checkpoints.end();) {
if (it->first < m_last_finalized_epoch) {
Expand All @@ -187,19 +196,20 @@ void FinalizationState::IncrementDynasty() {
}

ufp64::ufp64_t FinalizationState::GetCollectiveRewardFactor() {
uint32_t epoch = m_current_epoch;
bool isLive = GetEpochsSinceFinalization() <= 2;

if (!DepositExists() || !isLive) {
return 0;
}

assert(m_last_finalized_epoch == m_current_epoch - 2);

ufp64::ufp64_t curVoteFraction = ufp64::div_2uint(
GetCheckpoint(epoch - 1).GetCurDynastyVotes(m_expected_source_epoch),
GetCheckpoint(m_last_finalized_epoch).GetCurDynastyVotes(m_expected_source_epoch),
m_cur_dyn_deposits);

ufp64::ufp64_t prevVoteFraction = ufp64::div_2uint(
GetCheckpoint(epoch - 1).GetPrevDynastyVotes(m_expected_source_epoch),
GetCheckpoint(m_last_finalized_epoch).GetPrevDynastyVotes(m_expected_source_epoch),
m_prev_dyn_deposits);

ufp64::ufp64_t voteFraction = ufp64::min(curVoteFraction, prevVoteFraction);
Expand All @@ -208,7 +218,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;
Copy link
Member Author

@kostyantyn kostyantyn May 8, 2019 •
edited
Loading

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.

}

ufp64::ufp64_t FinalizationState::GetSqrtOfTotalDeposits() const {
Expand Down Expand Up @@ -418,7 +428,7 @@ void FinalizationState::ProcessDeposit(const uint160 &validatorAddress,
CAmount depositValue) {
LOCK(cs_esperanza);

uint32_t startDynasty = m_current_dynasty + 3;
uint32_t startDynasty = m_current_dynasty + 2;
scravy marked this conversation as resolved.
Show resolved Hide resolved
uint64_t scaledDeposit = ufp64::div_to_uint(static_cast<uint64_t>(depositValue),
GetDepositScaleFactor(m_current_epoch));

Expand Down Expand Up @@ -538,8 +548,8 @@ void FinalizationState::ProcessVote(const Vote &vote) {
targetEpoch);

if (targetEpoch == sourceEpoch + 1) {
GetCheckpoint(sourceEpoch).m_is_finalized = true;
m_last_finalized_epoch = sourceEpoch;
GetCheckpoint(targetEpoch).m_is_finalized = true;
m_last_finalized_epoch = targetEpoch;
LogPrint(BCLog::FINALIZATION, "%s: epoch=%d finalized.\n", __func__,
sourceEpoch);
}
Expand Down Expand Up @@ -862,17 +872,6 @@ uint32_t FinalizationState::GetCurrentDynasty() const {
return m_current_dynasty;
}

uint32_t FinalizationState::GetCheckpointHeightAfterFinalizedEpoch() const {
const uint32_t epoch = m_last_finalized_epoch + 1;
if (m_last_finalized_epoch != 0) {
// epoch=0 is self-finalized and doesn't require
// parent epoch to justify it but for other epochs
// this rule must hold
assert(GetCheckpoint(epoch).m_is_justified);
}
return GetEpochCheckpointHeight(epoch);
}

uint32_t FinalizationState::GetEpochLength() const {
return m_settings.epoch_length;
}
Expand Down Expand Up @@ -1026,7 +1025,6 @@ void FinalizationState::ProcessNewCommits(const CBlockIndex &block_index,

m_recommended_target_hash = block_index.GetBlockHash();
m_recommended_target_epoch = GetEpoch(block_index);
m_expected_source_epoch = m_last_justified_epoch;
}
m_status = FROM_COMMITS;
}
Expand Down
3 changes: 0 additions & 3 deletions src/esperanza/finalizationstate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,6 @@ class FinalizationState : public FinalizationStateData {
uint32_t GetCurrentEpoch() const;
uint32_t GetCurrentDynasty() const;

//! \brief Returns the height of checkpoint next to last finalized checkpoint. It must be justified.
uint32_t GetCheckpointHeightAfterFinalizedEpoch() const;

uint64_t GetDepositSize(const uint160 &validatorAddress) const;

uint32_t GetRecommendedTargetEpoch() const;
Expand Down
11 changes: 3 additions & 8 deletions src/finalization/state_processor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,10 @@ bool ProcessorImpl::ProcessNewTip(const CBlockIndex &block_index, const CBlock &
assert(state);

// We cannot make forks before this point as they can revert finalization.
const uint32_t trim_until = state->GetCheckpointHeightAfterFinalizedEpoch();
const uint32_t checkpoint_height = state->GetEpochCheckpointHeight(state->GetLastFinalizedEpoch());
m_repo->TrimUntilHeight(checkpoint_height);

// for 0 epoch it will be in the future
if (static_cast<uint32_t>(block_index.nHeight) > trim_until) {
m_repo->TrimUntilHeight(trim_until);
}

const uint32_t checkpoint = state->GetEpochCheckpointHeight(state->GetLastFinalizedEpoch());
snapshot::Creator::FinalizeSnapshots(m_active_chain->AtHeight(checkpoint));
snapshot::Creator::FinalizeSnapshots(m_active_chain->AtHeight(checkpoint_height));
}
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/finalization/state_repository.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ bool RepositoryImpl::LoadStatesFromDB() {
LogPrint(BCLog::FINALIZATION, "Restoring state repository from disk, last_finalized_epoch=%d\n",
*last_finalized_epoch);
const blockchain::Height height =
m_finalization_params->GetEpochCheckpointHeight(*last_finalized_epoch + 1);
m_finalization_params->GetEpochCheckpointHeight(*last_finalized_epoch);
m_state_db->LoadStatesHigherThan(height, &m_states);
if (!m_states.empty()) {
return true;
Expand All @@ -212,7 +212,7 @@ void RepositoryImpl::CheckAndRecover(Dependency<finalization::StateProcessor> pr

const uint32_t last_finalized_epoch = state->GetLastFinalizedEpoch();

const blockchain::Height height = last_finalized_epoch == 0 ? 0 : m_finalization_params->GetEpochCheckpointHeight(last_finalized_epoch + 1);
const blockchain::Height height = m_finalization_params->GetEpochCheckpointHeight(last_finalized_epoch);

std::set<const CBlockIndex *> unrecoverable;

Expand Down
4 changes: 2 additions & 2 deletions src/test/esperanza/checks_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ BOOST_AUTO_TEST_CASE(ContextualCheckVoteTx_test) {
CPubKey pub_key = key.GetPubKey();
uint160 validator_address = pub_key.GetID();

Vote vote_out{pub_key.GetID(), target_hash, 4, 5};
Vote vote_out{pub_key.GetID(), target_hash, 2, 3};

std::vector<unsigned char> vote_sig_out;
BOOST_REQUIRE(CreateVoteSignature(&keystore, vote_out, vote_sig_out));
Expand Down Expand Up @@ -1036,7 +1036,7 @@ BOOST_AUTO_TEST_CASE(IsVoteExpired_test) {
Vote current{RandValidatorAddr(), target_hash, 0, 5};
BOOST_CHECK_EQUAL(IsVoteExpired(CreateVoteTx(current, k), spy), false);

Vote afterLastFinalization{RandValidatorAddr(), target_hash, 0, 3};
Vote afterLastFinalization{RandValidatorAddr(), target_hash, 0, 2};
BOOST_CHECK_EQUAL(IsVoteExpired(CreateVoteTx(afterLastFinalization, k), spy), true);

Vote future{RandValidatorAddr(), target_hash, 0, 12};
Expand Down
Loading