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.

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
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
54 changes: 51 additions & 3 deletions libraries/chain/include/eosio/chain/hotstuff.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,46 @@ namespace eosio::chain {

using hs_bitset = boost::dynamic_bitset<uint32_t>;

static digest_type get_digest_to_sign(const block_id_type& block_id, uint8_t phase_counter, const fc::sha256& final_on_qc){
heifner marked this conversation as resolved.
Show resolved Hide resolved
digest_type h1 = digest_type::hash( std::make_pair( block_id, phase_counter ) );
Copy link
Member

Choose a reason for hiding this comment

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

std::make_pair( std::cref(block_id), phase_counter ) avoids copy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you don't need the make_pair constructor!

Copy link
Contributor

@greg7mdp greg7mdp Aug 31, 2023

Choose a reason for hiding this comment

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

removing the std::pair constructor:

    struct view_number {
       view_number() : _block_height(0), _phase_counter(0) {}
-      explicit view_number(std::pair<uint32_t, uint8_t> data) :
-         _block_height(data.first),
-         _phase_counter(data.second)
-      {
-      }
       
       explicit view_number(uint32_t block_height, uint8_t phase_counter) :
          _block_height(block_height),
@@ -44,9 +39,7 @@ namespace eosio::chain {
       uint64_t    to_uint64_t() const { return compute_height(_block_height, _phase_counter);   }
       std::string to_string()   const { return std::to_string(_block_height) + "::" + std::to_string(_phase_counter); }
 
-      uint64_t get_key() const { return get_view_number().to_uint64_t(); };
-
-      view_number get_view_number() const { return view_number{ block_height(), phase_counter() }; };
+      uint64_t get_key() const { return to_uint64_t(); };
 
       uint32_t _block_height;
       uint8_t  _phase_counter;
@@ -82,7 +75,7 @@ namespace eosio::chain {
       uint32_t block_num() const { return block_header::num_from_id(block_id); }
       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(std::make_pair(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); };
 
    };
 
modified   libraries/hotstuff/test/hotstuff_tools.cpp
@@ -56,8 +56,8 @@ BOOST_AUTO_TEST_CASE(view_number_tests) try {
 
   //test constructor
 
-  eosio::hotstuff::view_number vn_6 = eosio::hotstuff::view_number(std::make_pair(194217068, 2));
+  eosio::hotstuff::view_number vn_6(194217068, 2);
 
   BOOST_CHECK_EQUAL(vn_5 == vn_6, true);

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused? How do you do this without make_pair or some kind of tuple struct type?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the std::pair constructor of view_number is not needed, but what does that have to do with this get_digest_to_sign implementation?

Copy link
Member

Choose a reason for hiding this comment

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

This comment chain is on get_digest_to_sign. I think we are in agreement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, my bad, sorry. For the get_digest_to_sign, can we only hash a pair and not a tuple? would be nicer to hash all 3 at once.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, no reason to hash them separately. Good call.

Copy link
Member

@heifner heifner Sep 1, 2023

Choose a reason for hiding this comment

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

I would do this:

struct hotstuff_digest_type { 
   const block_id_type& block_id; 
   uint8_t phase_counter = 0; 
   const fc::sha256& final_on_qc;

   digest_type to_digest() const { digest_type::hash( *this ); }
}; 

auto digest = hotstuff_digest_type{blk_id, phase_c, f_on_qc}.to_digest();

FC_REFLECT(eosio::chain::hotstuff_digest_type, (block_id)(phase_counter)(final_on_qc));

Copy link

Choose a reason for hiding this comment

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

The digest simplification is great but I think it's going to change all of the hotstuff unit tests since those use hardcoded hash values. If that's the case then I think we should do that in a separate issue/PR, referencing the comment with the fix: #1578 (comment)

digest_type h2 = digest_type::hash( std::make_pair( h1, final_on_qc ) );
heifner marked this conversation as resolved.
Show resolved Hide resolved
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() : _block_height(0), _phase_counter(0) {}
explicit view_number(std::pair<uint32_t, uint8_t> data) :
_block_height(data.first),
_phase_counter(data.second)
{
}

explicit view_number(uint32_t block_height, uint8_t phase_counter) :
_block_height(block_height),
_phase_counter(phase_counter)
{
}

auto operator<=>(const view_number&) const = default;

uint32_t block_height() const { return _block_height; }
uint8_t phase_counter() const { return _phase_counter; }

uint64_t to_uint64_t() const { return compute_height(_block_height, _phase_counter); }
std::string to_string() const { return std::to_string(_block_height) + "::" + std::to_string(_phase_counter); }

uint64_t get_key() const { return get_view_number().to_uint64_t(); };
heifner marked this conversation as resolved.
Show resolved Hide resolved

view_number get_view_number() const { return view_number{ block_height(), phase_counter() }; };
heifner marked this conversation as resolved.
Show resolved Hide resolved

uint32_t _block_height;
uint8_t _phase_counter;
};

struct extended_schedule {
producer_authority_schedule producer_schedule;
std::map<name, fc::crypto::blslib::bls_public_key> bls_pub_keys;
Expand All @@ -41,8 +77,13 @@ 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(std::make_pair(block_header::num_from_id(block_id), phase_counter)); };
heifner marked this conversation as resolved.
Show resolved Hide resolved

};

struct hs_new_block_message {
Expand All @@ -64,7 +105,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 @@ -78,9 +119,16 @@ namespace eosio::chain {
}
};


using hs_proposal_message_ptr = std::shared_ptr<hs_proposal_message>;
using hs_vote_message_ptr = std::shared_ptr<hs_vote_message>;
using hs_new_view_message_ptr = std::shared_ptr<hs_new_view_message>;
using hs_new_block_message_ptr = std::shared_ptr<hs_new_block_message>;
heifner marked this conversation as resolved.
Show resolved Hide resolved

} //eosio::chain

// // @ignore quorum_met

FC_REFLECT(eosio::chain::view_number, (_block_height)(_phase_counter));
heifner marked this conversation as resolved.
Show resolved Hide resolved
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)(sig));
Expand Down
2 changes: 1 addition & 1 deletion libraries/hotstuff/chain_pacemaker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ namespace eosio { namespace hotstuff {

chain_pacemaker::chain_pacemaker(controller* chain, std::set<account_name> my_producers, fc::logger& logger)
: _chain(chain),
_qc_chain("default"_n, this, std::move(my_producers), logger),
_qc_chain("default"_n, this, std::move(my_producers), logger, DEFAULT_SAFETY_STATE_FILE),
heifner marked this conversation as resolved.
Show resolved Hide resolved
_logger(logger)
{
_accepted_block_connection = chain->accepted_block.connect( [this]( const block_state_ptr& blk ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ namespace eosio::chain {

namespace eosio::hotstuff {

const std::string DEFAULT_SAFETY_STATE_FILE = "hs_tm_safety_state"; //todo : add reversible blocks folder
heifner marked this conversation as resolved.
Show resolved Hide resolved

class chain_pacemaker : public base_pacemaker {
public:

//class-specific functions

chain_pacemaker(controller* chain, std::set<account_name> my_producers, fc::logger& logger);
void register_bcast_function(std::function<void(const chain::hs_message&)> broadcast_hs_message);

Expand Down
47 changes: 40 additions & 7 deletions libraries/hotstuff/include/eosio/hotstuff/qc_chain.hpp
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 <fc/crypto/bls_utils.hpp>
#include <fc/crypto/sha256.hpp>
Expand All @@ -18,6 +19,8 @@

#include <boost/dynamic_bitset.hpp>

#include <fc/io/cfile.hpp>

#include <exception>
#include <stdexcept>

Expand All @@ -30,6 +33,34 @@ namespace eosio::hotstuff {
using namespace boost::multi_index;
using namespace eosio::chain;

template <typename StateObj>
static void read_state(const std::string file_path, StateObj& s){

if (file_path != std::string()){
fc::cfile pfile;
pfile.set_file_path(file_path);
pfile.open("rb");
auto ds = pfile.create_datastream();
fc::raw::unpack(ds, s);
pfile.close();
heifner marked this conversation as resolved.
Show resolved Hide resolved
}

}

template <typename StateObj>
static void write_state(const std::string file_path, const StateObj s){

if (file_path != std::string()){
fc::cfile pfile;
pfile.set_file_path(file_path);
pfile.open(fc::cfile::truncate_rw_mode);
auto data = fc::raw::pack(s);
pfile.write(data.data(), data.size());
pfile.close();
heifner marked this conversation as resolved.
Show resolved Hide resolved
}

}

class quorum_certificate {
public:
explicit quorum_certificate(size_t finalizer_size = 0) {
Expand Down Expand Up @@ -80,6 +111,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 @@ -95,7 +127,7 @@ namespace eosio::hotstuff {

qc_chain() = delete;

qc_chain(name id, base_pacemaker* pacemaker, std::set<name> my_producers, fc::logger& logger);
qc_chain(name id, base_pacemaker* pacemaker, std::set<name> my_producers, fc::logger& logger, std::string safety_state_file);

uint64_t get_state_version() const { return _state_version; } // calling this w/ thread sync is optional

Expand Down Expand Up @@ -123,7 +155,7 @@ namespace eosio::hotstuff {

hs_bitset update_bitset(const hs_bitset& finalizer_set, name finalizer);

digest_type get_digest_to_sign(const block_id_type& block_id, uint8_t phase_counter, const fc::sha256& final_on_qc); //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); //get digest to sign from proposal data
heifner marked this conversation as resolved.
Show resolved Hide resolved

void reset_qc(const fc::sha256& proposal_id); //reset current internal qc

Expand Down Expand Up @@ -183,15 +215,16 @@ 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

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;
std::string _safety_state_file;
heifner marked this conversation as resolved.
Show resolved Hide resolved
fc::sha256 _b_finality_violation;
quorum_certificate _high_qc;
quorum_certificate _current_qc;
uint32_t _v_height = 0;
eosio::chain::extended_schedule _schedule;
base_pacemaker* _pacemaker = nullptr;
std::set<name> _my_producers;
Expand Down Expand Up @@ -220,11 +253,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 All @@ -234,4 +267,4 @@ namespace eosio::hotstuff {

};

} /// eosio::hotstuff
} /// eosio::hotstuff
63 changes: 63 additions & 0 deletions libraries/hotstuff/include/eosio/hotstuff/state.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#include <eosio/chain/block_header.hpp>

#include <eosio/hotstuff/qc_chain.hpp>

namespace eosio::hotstuff {

using namespace eosio::chain;

struct safety_state {

void set_v_height(const name finalizer, const eosio::chain::view_number v_height){
_states[finalizer.to_uint64_t()].first = v_height;
}

void set_b_lock(const name finalizer, fc::sha256 b_lock){
_states[finalizer.to_uint64_t()].second = b_lock;
}

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

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

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

//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 std::make_pair(eosio::chain::view_number(),fc::sha256());
}

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

fc::sha256 get_b_lock() const{
auto s_itr = _states.begin();
if (s_itr != _states.end()) return s_itr->second.second;
else return fc::sha256();
};
heifner marked this conversation as resolved.
Show resolved Hide resolved

std::unordered_map<uint64_t, std::pair<eosio::chain::view_number, fc::sha256>> _states;

};

}

FC_REFLECT(eosio::hotstuff::safety_state, (_states))
Loading