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: Modify set_finalizer host function struct type #1588

Merged
merged 8 commits into from
Sep 1, 2023
Merged
1 change: 1 addition & 0 deletions libraries/chain/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ add_library( eosio_chain
${CHAIN_WEBASSEMBLY_SOURCES}

authority.cpp
finalizer_set.cpp
trace.cpp
transaction_metadata.cpp
protocol_state_object.cpp
Expand Down
1 change: 1 addition & 0 deletions libraries/chain/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1980,6 +1980,7 @@ struct controller_impl {
void set_finalizers_impl(const finalizer_set& fin_set) {
// TODO store in chainbase
current_finalizer_set = fin_set;
++current_finalizer_set.generation;
linh2931 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
27 changes: 27 additions & 0 deletions libraries/chain/finalizer_set.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#include <eosio/chain/finalizer_set.hpp>
#include <eosio/chain/finalizer_authority.hpp>
#include <fc/crypto/bls_public_key.hpp>

namespace eosio::chain {

/**
* These definitions are all here to avoid including bls_public_key.hpp which includes <bls12-381/bls12-381.hpp>
* and pulls in bls12-381 types. This keeps bls12-381 out of libtester.
*/

finalizer_set::finalizer_set() = default;
finalizer_set::~finalizer_set() = default;

finalizer_set::finalizer_set(const finalizer_set&) = default;
finalizer_set::finalizer_set(finalizer_set&&) = default;
greg7mdp marked this conversation as resolved.
Show resolved Hide resolved

finalizer_set& finalizer_set::operator=(const finalizer_set&) = default;
finalizer_set& finalizer_set::operator=(finalizer_set&&) = default;

auto finalizer_set::operator<=>(const finalizer_set&) const = default;


hs_finalizer_set_extension::hs_finalizer_set_extension(const finalizer_set& s)
: finalizer_set(s) {}

} /// eosio::chain
3 changes: 2 additions & 1 deletion libraries/chain/include/eosio/chain/config.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ static_assert(maximum_tracked_dpos_confirmations >= ((max_producers * 2 / 3) + 1
/**
* Maximum number of finalizers in the finalizer set
*/
const static int max_finalizers = max_producers;
const static size_t max_finalizers = 64*1024;
const static size_t max_finalizer_description = 256;
greg7mdp marked this conversation as resolved.
Show resolved Hide resolved

/**
* The number of blocks produced per round is based upon all producers having a chance
Expand Down
19 changes: 19 additions & 0 deletions libraries/chain/include/eosio/chain/finalizer_authority.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#pragma once

#include <fc/crypto/bls_public_key.hpp>
#include <string>

namespace eosio::chain {

struct finalizer_authority {

std::string description;
uint64_t fweight = 0; // weight that this finalizer's vote has for meeting fthreshold
fc::crypto::blslib::bls_public_key public_key;

auto operator<=>(const finalizer_authority&) const = default;
};

} /// eosio::chain

FC_REFLECT( eosio::chain::finalizer_authority, (description)(fweight)(public_key) )
80 changes: 35 additions & 45 deletions libraries/chain/include/eosio/chain/finalizer_set.hpp
Original file line number Diff line number Diff line change
@@ -1,59 +1,49 @@
#pragma once

#include <eosio/chain/config.hpp>
#include <eosio/chain/types.hpp>
#include <chainbase/chainbase.hpp>
#include <eosio/chain/authority.hpp>
#include <eosio/chain/snapshot.hpp>

#include <fc/crypto/bls_public_key.hpp>

namespace eosio::chain {

struct finalizer_authority {
struct finalizer_authority;

struct finalizer_set {
finalizer_set();
~finalizer_set();

finalizer_set(const finalizer_set&);
finalizer_set(finalizer_set&&);

finalizer_set& operator=(const finalizer_set&);
finalizer_set& operator=(finalizer_set&&);

std::string description;
uint64_t fweight = 0; // weight that this finalizer's vote has for meeting fthreshold
fc::crypto::blslib::bls_public_key public_key;
uint32_t generation = 0; ///< sequentially incrementing version number
uint64_t fthreshold = 0; ///< vote fweight threshold to finalize blocks
std::vector<finalizer_authority> finalizers; ///< Instant Finality voter set
greg7mdp marked this conversation as resolved.
Show resolved Hide resolved

friend bool operator == ( const finalizer_authority& lhs, const finalizer_authority& rhs ) {
return tie( lhs.description, lhs.fweight, lhs.public_key ) == tie( rhs.description, rhs.fweight, rhs.public_key );
}
friend bool operator != ( const finalizer_authority& lhs, const finalizer_authority& rhs ) {
return !(lhs == rhs);
}
auto operator<=>(const finalizer_set&) const;
greg7mdp marked this conversation as resolved.
Show resolved Hide resolved
};

struct finalizer_set {
finalizer_set() = default;

finalizer_set( uint32_t version, uint64_t fthreshold, std::initializer_list<finalizer_authority> finalizers )
:version(version)
,fthreshold(fthreshold)
,finalizers(finalizers)
{}

uint32_t version = 0; ///< sequentially incrementing version number
uint64_t fthreshold = 0; // vote fweight threshold to finalize blocks
vector<finalizer_authority> finalizers; // Instant Finality voter set

friend bool operator == ( const finalizer_set& a, const finalizer_set& b )
{
if( a.version != b.version ) return false;
if( a.fthreshold != b.fthreshold ) return false;
if ( a.finalizers.size() != b.finalizers.size() ) return false;
for( uint32_t i = 0; i < a.finalizers.size(); ++i )
if( ! (a.finalizers[i] == b.finalizers[i]) ) return false;
return true;
}

friend bool operator != ( const finalizer_set& a, const finalizer_set& b )
{
return !(a==b);
}
using finalizer_set_ptr = std::shared_ptr<finalizer_set>;

/**
* Block Header Extension Compatibility
*/
struct hs_finalizer_set_extension : finalizer_set {

static constexpr uint16_t extension_id() { return 2; } // TODO 3 instead?
static constexpr bool enforce_unique() { return true; }

hs_finalizer_set_extension() = default;
hs_finalizer_set_extension(const hs_finalizer_set_extension&) = default;
hs_finalizer_set_extension( hs_finalizer_set_extension&& ) = default;

hs_finalizer_set_extension& operator=(const hs_finalizer_set_extension&) = default;
hs_finalizer_set_extension& operator=(hs_finalizer_set_extension&&) = default;

hs_finalizer_set_extension(const finalizer_set& s);
greg7mdp marked this conversation as resolved.
Show resolved Hide resolved
};

} /// eosio::chain

FC_REFLECT( eosio::chain::finalizer_authority, (description)(fweight)(public_key) )
FC_REFLECT( eosio::chain::finalizer_set, (version)(fthreshold)(finalizers) )
FC_REFLECT( eosio::chain::finalizer_set, (generation)(fthreshold)(finalizers) )
FC_REFLECT_DERIVED( eosio::chain::hs_finalizer_set_extension, (eosio::chain::finalizer_set), )
19 changes: 11 additions & 8 deletions libraries/chain/webassembly/privileged.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <eosio/chain/resource_limits.hpp>
#include <eosio/chain/apply_context.hpp>
#include <eosio/chain/finalizer_set.hpp>
#include <eosio/chain/finalizer_authority.hpp>

#include <fc/io/datastream.hpp>

Expand Down Expand Up @@ -152,28 +153,30 @@ namespace eosio { namespace chain { namespace webassembly {
}

void interface::set_finalizers(span<const char> packed_finalizer_set) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we use std::span instead of just span? Same with vector below.

Copy link
Member Author

Choose a reason for hiding this comment

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

This not std::span but using span = eosio::vm::span<T, Extent>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, missed that, thanks. Still vector should be std::vector, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, changed to std::vector

EOS_ASSERT(!context.trx_context.is_read_only(), wasm_execution_error, "set_proposed_finalizers not allowed in a readonly transaction");
EOS_ASSERT(!context.trx_context.is_read_only(), wasm_execution_error, "set_finalizers not allowed in a readonly transaction");
fc::datastream<const char*> ds( packed_finalizer_set.data(), packed_finalizer_set.size() );
finalizer_set finset;
fc::raw::unpack(ds, finset);
vector<finalizer_authority> & finalizers = finset.finalizers;
// contract finalizer_set does not include uint32_t generation
// struct abi_finalizer_set {
// uint64_t fthreshold
// vector<finalizer_authority> finalizers; }
fc::raw::unpack(ds, finset.fthreshold);
fc::raw::unpack(ds, finset.finalizers);

vector<finalizer_authority>& finalizers = finset.finalizers;

// TODO: check version and increment it or verify correct
EOS_ASSERT( finalizers.size() <= config::max_finalizers, wasm_execution_error, "Finalizer set exceeds the maximum finalizer count for this chain" );
EOS_ASSERT( finalizers.size() > 0, wasm_execution_error, "Finalizer set cannot be empty" );

std::set<fc::crypto::blslib::bls_public_key> unique_finalizer_keys;
#warning REVIEW: Is checking for unique finalizer descriptions at all relevant?
std::set<std::string> unique_finalizers;
uint64_t f_weight_sum = 0;

for (const auto& f: finalizers) {
EOS_ASSERT( f.description.size() <= config::max_finalizer_description, wasm_execution_error, "Finalizer description greater than 256" );
f_weight_sum += f.fweight;
unique_finalizer_keys.insert(f.public_key);
unique_finalizers.insert(f.description);
}

EOS_ASSERT( finalizers.size() == unique_finalizers.size(), wasm_execution_error, "Duplicate finalizer description in finalizer set" );
EOS_ASSERT( finalizers.size() == unique_finalizer_keys.size(), wasm_execution_error, "Duplicate finalizer bls key in finalizer set" );
EOS_ASSERT( finset.fthreshold > f_weight_sum / 2, wasm_execution_error, "Finalizer set threshold cannot be met by finalizer weights" );
Copy link
Contributor

Choose a reason for hiding this comment

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

why / 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know @arhag or @fcecin ?

Copy link

Choose a reason for hiding this comment

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

That is my attempt to implement this spec: #1511 (comment) ; I hope it is correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the spec mentions 15, which is 2/3+1 of 21. I wonder if we should not check that threshold > (f_weight_sum / 3) * 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, seems like it should be threshold >= (f_weight_sum * 2 / 3) + 1 or threshold > (f_weight_sum * 2 / 3) .

Copy link
Member Author

Choose a reason for hiding this comment

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

Conversation summary: The host function will enforce f_weight_sum / 2. It will be up to system contract to enforce a higher threshold if desired.

So if the chain would prefer to be more secure against finality violations at the cost of increased risk of losing liveness, it can increase the threshold above 1/2. Perhaps even all the way to 100% if it wanted.


Expand Down