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

IF: Persist hotstuff safety state #1578

Merged
merged 17 commits into from
Nov 19, 2023
Merged

IF: Persist hotstuff safety state #1578

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
372b8bf
Replaced hs_proposal_message::get_height() with ::get_view_number(), …
systemzax Aug 28, 2023
01764cd
Code reorganization, extension of liveness_state definition to includ…
systemzax Aug 29, 2023
d56303a
Added safety / liveness state persistence
systemzax Aug 29, 2023
b9e7e18
Added persistence of safety information - WIP
systemzax Aug 30, 2023
ede43a1
Added state.hpp file
systemzax Aug 30, 2023
12a0ea7
Added persistance of safety state core functionality
systemzax Aug 30, 2023
5572262
Merge branch 'hotstuff_integration' into persist_hs_safety_liveness_s…
systemzax Aug 30, 2023
dd33d93
added check for missing file
systemzax Aug 30, 2023
6f61f6b
Merge branch 'hotstuff_integration' into persist_hs_safety_liveness_s…
greg7mdp Aug 31, 2023
a990bc9
Merge remote-tracking branch 'origin/hotstuff_integration' into persi…
fcecin Sep 28, 2023
17d587b
Misc improvements
fcecin Sep 29, 2023
e771f49
Fixes
fcecin Nov 10, 2023
4b34cf5
Merge branch 'hotstuff_integration' into persist_hs_safety_liveness_s…
fcecin Nov 10, 2023
e0da055
Some of the fixes from last reviews (WIP)
fcecin Nov 18, 2023
c171034
Update libraries/chain/include/eosio/chain/hotstuff.hpp
fcecin Nov 18, 2023
14a898e
Merge branch 'hotstuff_integration' into persist_hs_safety_liveness_s…
fcecin Nov 18, 2023
749cf41
Fix for merge hotstuff_integration -> persist_hs_safety_liveness_stat…
fcecin Nov 18, 2023
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
2 changes: 1 addition & 1 deletion libraries/chain/include/eosio/chain/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const static auto reversible_blocks_dir_name = "reversible";

const static auto default_state_dir_name = "state";
const static auto forkdb_filename = "fork_db.dat";
const static auto qcdb_filename = "qc_db.dat";
const static auto safetydb_filename = "safety_db.dat";
const static auto default_state_size = 1*1024*1024*1024ll;
const static auto default_state_guard_size = 128*1024*1024ll;

Expand Down
32 changes: 29 additions & 3 deletions libraries/chain/include/eosio/chain/hotstuff.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,31 @@ namespace eosio::chain {
using hs_bitset = boost::dynamic_bitset<uint8_t>;
using bls_key_map_t = std::map<fc::crypto::blslib::bls_public_key, fc::crypto::blslib::bls_private_key>;

inline digest_type get_digest_to_sign(const block_id_type& block_id, uint8_t phase_counter, const fc::sha256& final_on_qc) {
digest_type h1 = digest_type::hash( std::make_pair( std::cref(block_id), phase_counter ) );
digest_type h2 = digest_type::hash( std::make_pair( std::cref(h1), std::cref(final_on_qc) ) );
return h2;
}

inline uint64_t compute_height(uint32_t block_height, uint32_t phase_counter) {
return (uint64_t{block_height} << 32) | phase_counter;
}

struct view_number {
view_number() : bheight(0), pcounter(0) {}
explicit view_number(uint32_t block_height, uint8_t phase_counter) : bheight(block_height), pcounter(phase_counter) {}
auto operator<=>(const view_number&) const = default;

uint32_t block_height() const { return bheight; }
uint8_t phase_counter() const { return pcounter; }
uint64_t to_uint64_t() const { return compute_height(bheight, pcounter); }
std::string to_string() const { return std::to_string(bheight) + "::" + std::to_string(pcounter); }
uint64_t get_key() const { return to_uint64_t(); }
fcecin marked this conversation as resolved.
Show resolved Hide resolved

uint32_t bheight;
uint8_t pcounter;
};

struct extended_schedule {
producer_authority_schedule producer_schedule;
std::map<name, fc::crypto::blslib::bls_public_key> bls_pub_keys;
Expand All @@ -42,8 +63,12 @@ namespace eosio::chain {
quorum_certificate_message justify; //justification
uint8_t phase_counter = 0;

digest_type get_proposal_id() const { return get_digest_to_sign(block_id, phase_counter, final_on_qc); };

uint32_t block_num() const { return block_header::num_from_id(block_id); }
uint64_t get_height() const { return compute_height(block_header::num_from_id(block_id), phase_counter); };
uint64_t get_key() const { return compute_height(block_header::num_from_id(block_id), phase_counter); };

view_number get_view_number() const { return view_number(block_header::num_from_id(block_id), phase_counter); };
};

struct hs_new_block_message {
Expand Down Expand Up @@ -72,7 +97,7 @@ namespace eosio::chain {
fc::sha256 b_finality_violation;
block_id_type block_exec;
block_id_type pending_proposal_block;
uint32_t v_height = 0;
eosio::chain::view_number v_height;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this and other members should be std::optional, but we can update that later.

eosio::chain::quorum_certificate_message high_qc;
eosio::chain::quorum_certificate_message current_qc;
eosio::chain::extended_schedule schedule;
Expand All @@ -88,7 +113,8 @@ namespace eosio::chain {

} //eosio::chain

// // @ignore quorum_met

FC_REFLECT(eosio::chain::view_number, (bheight)(pcounter));
FC_REFLECT(eosio::chain::quorum_certificate_message, (proposal_id)(active_finalizers)(active_agg_sig));
FC_REFLECT(eosio::chain::extended_schedule, (producer_schedule)(bls_pub_keys));
FC_REFLECT(eosio::chain::hs_vote_message, (proposal_id)(finalizer_key)(sig));
Expand Down
3 changes: 2 additions & 1 deletion libraries/hotstuff/chain_pacemaker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,13 @@ namespace eosio { namespace hotstuff {
#endif
//===============================================================================================

#warning TODO get a data directory str passed into the chain_pacemaker ctor and use it to compose the absolute filepathname that is passed to qc_chain ctor
chain_pacemaker::chain_pacemaker(controller* chain,
std::set<account_name> my_producers,
bls_key_map_t finalizer_keys,
fc::logger& logger)
: _chain(chain),
_qc_chain("default", this, std::move(my_producers), std::move(finalizer_keys), logger),
_qc_chain("default", this, std::move(my_producers), std::move(finalizer_keys), logger, eosio::chain::config::safetydb_filename),
_logger(logger)
{
_accepted_block_connection = chain->accepted_block.connect( [this]( const block_state_ptr& blk ) {
Expand Down
73 changes: 64 additions & 9 deletions libraries/hotstuff/include/eosio/hotstuff/qc_chain.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <eosio/chain/controller.hpp>
#include <eosio/chain/block_state.hpp>
#include <eosio/hotstuff/base_pacemaker.hpp>
#include <eosio/hotstuff/state.hpp>

#include <eosio/chain/finalizer_set.hpp>
#include <eosio/chain/finalizer_authority.hpp>
Expand All @@ -21,6 +22,7 @@

#include <boost/dynamic_bitset.hpp>

#include <fc/io/cfile.hpp>

#include <exception>
#include <stdexcept>
Expand All @@ -30,6 +32,55 @@

namespace eosio::hotstuff {

template<typename StateObjectType> class state_db_manager {
public:
static bool read(fc::cfile& pfile, StateObjectType& sobj) {
if (!pfile.is_open())
return false;
pfile.seek_end(0);
if (pfile.tellp() <= 0)
return false;
pfile.seek(0);
auto datastream = pfile.create_datastream();
StateObjectType read_sobj;
try {
fc::raw::unpack(datastream, read_sobj);
sobj = std::move(read_sobj);
return true;
} catch (...) {
return false;
}
}
fcecin marked this conversation as resolved.
Show resolved Hide resolved
static bool write(fc::cfile& pfile, const StateObjectType& sobj) {
if (!pfile.is_open())
return false;
pfile.seek(0);
pfile.truncate();
auto data = fc::raw::pack(sobj);
pfile.write(data.data(), data.size());
pfile.flush();
return true;
}
static bool read(const std::string& file_path, StateObjectType& sobj) {
if (!std::filesystem::exists(file_path))
return false;
fc::cfile pfile;
pfile.set_file_path(file_path);
pfile.open("rb");
bool result = read(pfile, sobj);
pfile.close();
heifner marked this conversation as resolved.
Show resolved Hide resolved
return result;
fcecin marked this conversation as resolved.
Show resolved Hide resolved
}
static bool write(const std::string& file_path, const StateObjectType& sobj) {
fc::cfile pfile;
pfile.set_file_path(file_path);
pfile.open(fc::cfile::truncate_rw_mode);
bool result = write(pfile, sobj);
pfile.close();
return result;
fcecin marked this conversation as resolved.
Show resolved Hide resolved
}
};

using boost::multi_index_container;
using namespace boost::multi_index;
using namespace eosio::chain;
Expand Down Expand Up @@ -85,6 +136,7 @@ namespace eosio::hotstuff {
bool is_quorum_met() const { return quorum_met; }
void set_quorum_met() { quorum_met = true; }


private:
friend struct fc::reflector<quorum_certificate>;
fc::sha256 proposal_id;
Expand All @@ -103,7 +155,8 @@ namespace eosio::hotstuff {
qc_chain(std::string id, base_pacemaker* pacemaker,
std::set<name> my_producers,
chain::bls_key_map_t finalizer_keys,
fc::logger& logger);
fc::logger& logger,
std::string safety_state_file);

uint64_t get_state_version() const { return _state_version; } // no lock required

Expand All @@ -122,6 +175,8 @@ namespace eosio::hotstuff {

private:

void write_safety_state_file();

const hs_proposal_message* get_proposal(const fc::sha256& proposal_id); // returns nullptr if not found

// returns false if proposal with that same ID already exists at the store of its height
Expand All @@ -131,9 +186,6 @@ namespace eosio::hotstuff {

hs_bitset update_bitset(const hs_bitset& finalizer_set, const fc::crypto::blslib::bls_public_key& finalizer_key);

//get digest to sign from proposal data
digest_type get_digest_to_sign(const block_id_type& block_id, uint8_t phase_counter, const fc::sha256& final_on_qc);

void reset_qc(const fc::sha256& proposal_id);

bool evaluate_quorum(const hs_bitset& finalizers, const fc::crypto::blslib::bls_signature& agg_sig, const hs_proposal_message& proposal); //evaluate quorum for a proposal
Expand All @@ -154,7 +206,7 @@ namespace eosio::hotstuff {
void process_new_view(const std::optional<uint32_t>& connection_id, const hs_new_view_message& msg);
void process_new_block(const std::optional<uint32_t>& connection_id, const hs_new_block_message& msg);

hs_vote_message sign_proposal(const hs_proposal_message& proposal, const fc::crypto::blslib::bls_private_key& finalizer_priv_key);
hs_vote_message sign_proposal(const hs_proposal_message& proposal, const fc::crypto::blslib::bls_public_key& finalizer_pub_key, const fc::crypto::blslib::bls_private_key& finalizer_priv_key);

//verify that a proposal descends from another
bool extends(const fc::sha256& descendant, const fc::sha256& ancestor);
Expand Down Expand Up @@ -192,20 +244,23 @@ namespace eosio::hotstuff {
};

bool _chained_mode = false;

block_id_type _block_exec;
block_id_type _pending_proposal_block;
safety_state _safety_state;
Copy link
Member

@linh2931 linh2931 Nov 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Need to update all safety_state to safety_states to get compiled.

Copy link

Choose a reason for hiding this comment

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

I'm going to revert the std::optional patch. Either we commit this as file persistence ticket of a previous safety state logic, or we roll in reviewing and fixing the safety state logic with the file persistence in this same PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think you can commit this as previous safety state logic after CI is passed. Then in a new PR fix the remaining issues. This way we can move forward quickly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree, you can merge this. I think it is clear it is not the final version but this moves us forward.

fc::sha256 _b_leaf;
fc::sha256 _b_lock;
fc::sha256 _b_exec;
fc::sha256 _b_finality_violation;
quorum_certificate _high_qc;
quorum_certificate _current_qc;
uint32_t _v_height = 0;
base_pacemaker* _pacemaker = nullptr;
std::set<name> _my_producers;
chain::bls_key_map_t _my_finalizer_keys;
std::string _id;

std::string _safety_state_file; // if empty, safety state persistence is turned off
fc::cfile _safety_state_file_handle;

mutable std::atomic<uint64_t> _state_version = 1;

fc::logger& _logger;
Expand All @@ -229,11 +284,11 @@ namespace eosio::hotstuff {
indexed_by<
hashed_unique<
tag<by_proposal_id>,
BOOST_MULTI_INDEX_MEMBER(hs_proposal_message,fc::sha256,proposal_id)
BOOST_MULTI_INDEX_MEMBER(hs_proposal_message, fc::sha256,proposal_id)
>,
ordered_non_unique<
tag<by_proposal_height>,
BOOST_MULTI_INDEX_CONST_MEM_FUN(hs_proposal_message,uint64_t,get_height)
BOOST_MULTI_INDEX_CONST_MEM_FUN(hs_proposal_message, uint64_t, get_key)
>
>
> proposal_store_type;
Expand Down
61 changes: 61 additions & 0 deletions libraries/hotstuff/include/eosio/hotstuff/state.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#include <eosio/chain/block_header.hpp>

#include <eosio/hotstuff/qc_chain.hpp>

namespace eosio::hotstuff {

using namespace eosio::chain;

struct safety_state {

fcecin marked this conversation as resolved.
Show resolved Hide resolved
void set_v_height(const fc::crypto::blslib::bls_public_key& finalizer_key, const eosio::chain::view_number v_height) {
_states[finalizer_key].first = v_height;
}

void set_b_lock(const fc::crypto::blslib::bls_public_key& finalizer_key, const fc::sha256& b_lock) {
_states[finalizer_key].second = b_lock;
}
Comment on lines +11 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right, that we set one or the other, and the other half of the pair will contain default values.

Copy link

Choose a reason for hiding this comment

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

I think a zeroed-out hash is fine as an indicator of "we don't have one"; not sure about view number zero (bheight==0, pcounter==0).

Copy link

Choose a reason for hiding this comment

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

This is being discussed in the comment below: #1578


std::pair<eosio::chain::view_number, fc::sha256> get_safety_state(const fc::crypto::blslib::bls_public_key& finalizer_key) const {
auto s = _states.find(finalizer_key);
if (s != _states.end()) return s->second;
else return {};
}

eosio::chain::view_number get_v_height(const fc::crypto::blslib::bls_public_key& finalizer_key) const {
auto s = _states.find(finalizer_key);
if (s != _states.end()) return s->second.first;
else return {};
};

fc::sha256 get_b_lock(const fc::crypto::blslib::bls_public_key& finalizer_key) const {
auto s_itr = _states.find(finalizer_key);
if (s_itr != _states.end()) return s_itr->second.second;
else return {};
};

//todo : implement safety state default / sorting

std::pair<eosio::chain::view_number, fc::sha256> get_safety_state() const {
auto s = _states.begin();
if (s != _states.end()) return s->second;
else return {};
}

eosio::chain::view_number get_v_height() const {
auto s = _states.begin();
if (s != _states.end()) return s->second.first;
else return {};
};

fc::sha256 get_b_lock() const {
auto s_itr = _states.begin();
if (s_itr != _states.end()) return s_itr->second.second;
else return {};
};

std::map<fc::crypto::blslib::bls_public_key, std::pair<eosio::chain::view_number, fc::sha256>> _states;
};
}

FC_REFLECT(eosio::hotstuff::safety_state, (_states))
Comment on lines +9 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to remove the incorrect 3 bottom APIs, and make it less verbose with a couple using directives. Also should return optional so we have a clear way to test that something is not present, I think it is cleaner than relying on the empty() of sha256.
Also suggest to rename the struct safety_states as it holds the safety states for all finalizers.

Suggested change
struct safety_state {
void set_v_height(const fc::crypto::blslib::bls_public_key& finalizer_key, const eosio::chain::view_number v_height) {
_states[finalizer_key].first = v_height;
}
void set_b_lock(const fc::crypto::blslib::bls_public_key& finalizer_key, const fc::sha256& b_lock) {
_states[finalizer_key].second = b_lock;
}
std::pair<eosio::chain::view_number, fc::sha256> get_safety_state(const fc::crypto::blslib::bls_public_key& finalizer_key) const {
auto s = _states.find(finalizer_key);
if (s != _states.end()) return s->second;
else return {};
}
eosio::chain::view_number get_v_height(const fc::crypto::blslib::bls_public_key& finalizer_key) const {
auto s = _states.find(finalizer_key);
if (s != _states.end()) return s->second.first;
else return {};
};
fc::sha256 get_b_lock(const fc::crypto::blslib::bls_public_key& finalizer_key) const {
auto s_itr = _states.find(finalizer_key);
if (s_itr != _states.end()) return s_itr->second.second;
else return {};
};
//todo : implement safety state default / sorting
std::pair<eosio::chain::view_number, fc::sha256> get_safety_state() const {
auto s = _states.begin();
if (s != _states.end()) return s->second;
else return {};
}
eosio::chain::view_number get_v_height() const {
auto s = _states.begin();
if (s != _states.end()) return s->second.first;
else return {};
};
fc::sha256 get_b_lock() const {
auto s_itr = _states.begin();
if (s_itr != _states.end()) return s_itr->second.second;
else return {};
};
std::map<fc::crypto::blslib::bls_public_key, std::pair<eosio::chain::view_number, fc::sha256>> _states;
};
}
FC_REFLECT(eosio::hotstuff::safety_state, (_states))
struct safety_states {
using safety_state = std::pair<eosio::chain::view_number, fc::sha256>;
using public_key = fc::crypto::blslib::bls_public_key;
void set_v_height(const public_key& finalizer_key, const eosio::chain::view_number v_height) {
_states[finalizer_key].first = v_height;
}
void set_b_lock(const public_key& finalizer_key, const fc::sha256& b_lock) {
_states[finalizer_key].second = b_lock;
}
std::optional<safety_state> get_safety_state(const public_key& finalizer_key) const {
auto s = _states.find(finalizer_key);
if (s != _states.end())
return s->second;
return {};
}
std::optional<eosio::chain::view_number> get_v_height(const public_key& finalizer_key) const {
auto s = get_safety_state(finalizer_key);
if (s)
return s->first;
return {};
};
std::optional<fc::sha256> get_b_lock(const public_key& finalizer_key) const {
auto s = get_safety_state(finalizer_key);
if (s)
return s->second;
return {};
};
//todo : implement safety state default / sorting
std::map<public_key, safety_state> _states;
};
}
FC_REFLECT(eosio::hotstuff::safety_states, (_states))

Copy link

Choose a reason for hiding this comment

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

I don't know, since these results end up in decision-making math for the protocol. We can discuss this in the meeting next week.

Copy link

@fcecin fcecin Nov 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i) I committed this to the branch.

ii) maybe the missing optional v_height is always "less" (<) than any valid value for the sake of the protocol math. missing optional b_lock behaves like the zeroed-out/empty sha256 behaved.

iii) maybe b_lock and v_height should behave differently in the safety state record. b_lock is currently assigned simultaneously to all finalizer keys, so it looks like a singleton piece of sate for one qc_chain. v_height is attributed separately, so it would still go into a key-->v_height map.

struct safety_states {
    fc::sha256 b_lock;
    std::map<public_key, eosio::chain::view_number> v_height;
}

This would allow for safety_states::get_b_lock() to remain parameterless, which looks like what it wants to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I also don't think it is quite right as it is. You probably need to discuss with @systemzax as to what is the right data structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll approve the PR as is, even if it is not perfect, I think it still gives us a basis to move forward.

Loading