Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

fixes for dpos3 #2740

Merged
merged 3 commits into from
May 3, 2018
Merged

fixes for dpos3 #2740

merged 3 commits into from
May 3, 2018

Conversation

wanderingbort
Copy link
Contributor

No description provided.

b1bart added 2 commits May 3, 2018 15:35
…ts own watermarks the same way any producer plugin would, fix off-by-one caused by not accounting for a block confirming itself; fix off-by-one caused by propagating the value of dpos-lib before it was updated; decoupled dpos-lib from bft-lib though controller still returns the max of them
…needed at the time a block was made instead of how many are active at the time we are updating confirmations
@wanderingbort wanderingbort changed the title Feature/fixes for dpos3 fixes for dpos3 May 3, 2018
@@ -22,10 +23,6 @@ struct block_header_state {
public_key_type block_signing_key;
vector<uint8_t> confirm_count;

Copy link
Contributor

Choose a reason for hiding this comment

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

Move confirmations up to here.

@@ -60,7 +57,7 @@ struct block_header_state {
} } /// namespace eosio::chain

FC_REFLECT( eosio::chain::block_header_state,
(id)(block_num)(header)(dpos_last_irreversible_blocknum)
(id)(block_num)(header)(dpos_irreversible_blocknum)
(pending_schedule_lib_num)(pending_schedule_hash)
(pending_schedule)(active_schedule)(blockroot_merkle)
(producer_to_last_produced)(block_signing_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing confirm_count. Also I would put these in the same order as declared.

}
}
result.producer_to_last_produced = move( new_producer_to_last_produced );
result.producer_to_last_produced[prokey.producer_name] = result.block_num;
}

/// grow the confirmed count
uint8_t required_confs = (result.active_schedule.producers.size() * 2 / 3) + 1;
if( confirm_count.size() < 1024 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 1024, comment would be good.

@@ -85,36 +85,19 @@ namespace eosio { namespace chain {
result.block_num = block_num + 1;
result.producer_to_last_produced = producer_to_last_produced;
result.producer_to_last_produced[prokey.producer_name] = result.block_num;
// result.dpos_last_irreversible_blocknum = result.calc_dpos_last_irreversible();
// result.dpos_irreversible_blocknum = result.calc_dpos_last_irreversible();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this commented out code.

int32_t i = confirm_count.size() - 2;
while( i >= 0 && num_prev_blocks ) {
++confirm_count[i];
int32_t i = (int32_t)(confirm_count.size() - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assert confirm_count.size() != 0

arhag added a commit that referenced this pull request May 3, 2018
… in anticipation of PR #2740 which should fix the test.
@heifner heifner merged commit 7c1748a into EOSIO:slim May 3, 2018
@luomeiqin
Copy link

luomeiqin commented Jul 12, 2018

I have a question that why eos changes last_irreversible_block_num from bft_irreversible_blocknum to this ?
uint32_t controller::last_irreversible_block_num() const {
return std::max(my->head->bft_irreversible_blocknum, my->head->dpos_irreversible_blocknum);
}
i was suppose to take bft_irreversible_blocknum as the LIB,after i go thought this issue # #3088

@raymondnuaa
Copy link

@luomeiqin
Current EOS only updates dpos_irreversible_blocknum, the bft_irreversible_blocknum should be for realtime BFT. And please note there is another variable dpos_proposed_irreversible_blocknum, which is used to implemented the two rounds confirmation of BFT. (Related functions: set_confirmed and calc_dpos_last_irreversible()).

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

Successfully merging this pull request may close these issues.

5 participants