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

Add support for changing the content of the finalizer safety info (fsi) file and loading older versions #492

Merged
merged 6 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
32 changes: 31 additions & 1 deletion libraries/chain/finality/finalizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ void my_finalizers_t::save_finalizer_safety_info() const {

persist_file.seek(0);
fc::raw::pack(persist_file, fsi_t::magic);
fc::raw::pack(persist_file, current_safety_file_version);
fc::raw::pack(persist_file, (uint64_t)(finalizers.size() + inactive_safety_info.size()));
for (const auto& [pub_key, f] : finalizers) {
fc::raw::pack(persist_file, pub_key);
Expand Down Expand Up @@ -235,10 +236,26 @@ my_finalizers_t::fsi_map my_finalizers_t::load_finalizer_safety_info() {
}
try {
persist_file.seek(0);

// read magic number. Can be `fsi_t::magic_unversioned` (for files without embedded version number)
// or `fsi_t::magic` (for files with a version number right after `magic`).
// ------------------------------------------------------------------------------------------------
uint64_t magic = 0;
fc::raw::unpack(persist_file, magic);
EOS_ASSERT(magic == fsi_t::magic, finalizer_safety_exception,
EOS_ASSERT(magic == fsi_t::magic_unversioned || magic == fsi_t::magic, finalizer_safety_exception,
"bad magic number in finalizer safety persistence file: ${p}", ("p", persist_file_path));

// If we expect the file to contain a version number, read it. We can load files with older
// versions, but it better not be a file with a version higher that the running nodeos understands.
// ------------------------------------------------------------------------------------------------
uint64_t file_version = 0; // default version for file with magic == fsi_t::magic_unversioned
if (magic != fsi_t::magic_unversioned)
fc::raw::unpack(persist_file, file_version);
EOS_ASSERT(file_version <= current_safety_file_version, finalizer_safety_exception,
"Incorrect version number in finalizer safety persistence file: ${p}", ("p", persist_file_path));

// finally read the `finalizer_safety_information` info
// ----------------------------------------------------
uint64_t num_finalizers {0};
fc::raw::unpack(persist_file, num_finalizers);
for (size_t i=0; i<num_finalizers; ++i) {
Expand All @@ -248,6 +265,7 @@ my_finalizers_t::fsi_map my_finalizers_t::load_finalizer_safety_info() {
fc::raw::unpack(persist_file, fsi);
res.emplace(pubkey, fsi);
}

persist_file.close();
} FC_LOG_AND_RETHROW()
// don't remove file we can't load
Expand Down Expand Up @@ -305,4 +323,16 @@ void my_finalizers_t::set_default_safety_information(const fsi_t& fsi) {
default_fsi = fsi;
}

bool my_finalizers_t::are_equivalent(uint32_t version, const fsi_map& old, const fsi_map& current) {
assert(version < current_safety_file_version); // this function compares an older version with the current one
switch(version) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need version, as it is hardcoded to 0 anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need version, as it is hardcoded to 0 anyway?

Where is it hardcoded to zero? It can take all values less than current_safety_file_version

Copy link
Member

Choose a reason for hiding this comment

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

I meant the case 0:. OK, currently current_safety_file_version is 1.

case 0:
return old == current;
break;
default:
assert(0);
return false;
}
}

} // namespace eosio::chain
7 changes: 6 additions & 1 deletion libraries/chain/include/eosio/chain/finality/finalizer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ namespace eosio::chain {
block_ref last_vote;
block_ref lock;

static constexpr uint64_t magic = 0x5AFE11115AFE1111ull;
static constexpr uint64_t magic_unversioned = 0x5AFE11115AFE1111ull;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any need to have the extra complexity of supporting the un-versioned file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not since people already have unversioned files? And it just a couple lines.

static constexpr uint64_t magic = 0x5AFE11115AFE1112ull;

static finalizer_safety_information unset_fsi() { return {}; }

Expand Down Expand Up @@ -69,6 +70,9 @@ namespace eosio::chain {

// ----------------------------------------------------------------------------------------
struct my_finalizers_t {
public:
static constexpr uint64_t current_safety_file_version = 1;

using fsi_t = finalizer_safety_information;
using fsi_map = std::map<bls_public_key, fsi_t>;

Expand Down Expand Up @@ -155,6 +159,7 @@ namespace eosio::chain {
// for testing purposes only, not thread safe
const fsi_t& get_fsi(const bls_public_key& k) { return finalizers[k].fsi; }
void set_fsi(const bls_public_key& k, const fsi_t& fsi) { finalizers[k].fsi = fsi; }
static bool are_equivalent(uint32_t version, const fsi_map& old, const fsi_map& current);
};

}
Expand Down
2 changes: 2 additions & 0 deletions unittests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/snapshots.hpp.in ${CMAKE_CURRENT_BINA

configure_file(${CMAKE_CURRENT_SOURCE_DIR}/deep-mind/deep-mind.hpp.in ${CMAKE_CURRENT_BINARY_DIR}/include/deep-mind.hpp ESCAPE_QUOTES)

configure_file(${CMAKE_CURRENT_SOURCE_DIR}/test-data/test-data.hpp.in ${CMAKE_CURRENT_BINARY_DIR}/include/test-data.hpp ESCAPE_QUOTES)

add_custom_command(
OUTPUT protocol_feature_digest_tests.cpp
COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/gen_protocol_feature_digest_tests.py ${CMAKE_CURRENT_SOURCE_DIR}/../libraries/chain/protocol_feature_manager.cpp > protocol_feature_digest_tests.cpp
Expand Down
65 changes: 61 additions & 4 deletions unittests/finalizer_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@
#include <eosio/testing/tester.hpp>
#include <eosio/testing/bls_utils.hpp>
#include <fc/io/cfile.hpp>
#include <test-data.hpp>

using namespace eosio;
using namespace eosio::chain;
using namespace eosio::testing;
using namespace std::string_literals;

using tstamp = block_timestamp_type;
using fsi_t = finalizer_safety_information;
using fsi_map = my_finalizers_t::fsi_map;

struct bls_keys_t {
bls_private_key privkey;
Expand All @@ -29,10 +32,16 @@ template<class FSI>
std::vector<FSI> create_random_fsi(size_t count) {
std::vector<FSI> res;
res.reserve(count);
for (size_t i=0; i<count; ++i) {
res.push_back(FSI{tstamp(i),
block_ref{sha256::hash((const char *)"vote"), tstamp(i*100 + 3)},
block_ref{sha256::hash((const char *)"lock"), tstamp(i*100)} });
for (size_t i = 0; i < count; ++i) {
res.push_back(FSI{
.last_vote_range_start = tstamp(i),
.last_vote = block_ref{.block_id = sha256::hash("vote"s + std::to_string(i)),
.timestamp = tstamp(i * 100 + 3),
.finality_digest = sha256::hash("vote_digest"s + std::to_string(i))},
.lock = block_ref{.block_id = sha256::hash("lock"s + std::to_string(i)),
.timestamp = tstamp(i * 100),
.finality_digest = sha256::hash("lock_digest"s + std::to_string(i))}
});
if (i)
assert(res.back() != res[0]);
}
Expand Down Expand Up @@ -210,4 +219,52 @@ BOOST_AUTO_TEST_CASE( finalizer_safety_file_io ) try {
} FC_LOG_AND_RETHROW()


BOOST_AUTO_TEST_CASE( finalizer_safety_file_versioning ) try {
std::filesystem::path test_data_path { UNITTEST_TEST_DATA_DIR };
auto fsi_reference_dir = test_data_path / "fsi";

auto create_fsi_reference = [&](my_finalizers_t& fset) {
std::vector<bls_keys_t> keys = create_keys(3);
std::vector<fsi_t> fsi = create_random_fsi<fsi_t>(3);

bls_pub_priv_key_map_t local_finalizers = create_local_finalizers<0, 1, 2>(keys);
fset.set_keys(local_finalizers);
set_fsi<decltype(fsi), 0, 1, 2>(fset, keys, fsi);
};

auto create_fsi_reference_file = [&](const std::filesystem::path& safety_file_path) {
my_finalizers_t fset{safety_file_path};
create_fsi_reference(fset);
fset.save_finalizer_safety_info();
};

auto mk_versioned_fsi_file_path = [&](uint32_t v) {
return fsi_reference_dir / ("safety_v"s + std::to_string(v) + ".dat");
};

auto current_version = my_finalizers_t::current_safety_file_version;

// enable code below to create a new reference version of the finalizer_safety_file
if (0)
create_fsi_reference_file(mk_versioned_fsi_file_path(current_version));

auto load_fsi_map = [&](const std::filesystem::path& safety_file_path) {
BOOST_REQUIRE(std::filesystem::exists(safety_file_path));
my_finalizers_t fset{safety_file_path};
auto map = fset.load_finalizer_safety_info();
return map;
};

// make sure we can read previous versions of the safety file correctly
// --------------------------------------------------------------------
auto fsi_map_current = load_fsi_map(mk_versioned_fsi_file_path(current_version));

for (size_t i=0; i<current_version; ++i) {
auto fsi_map_vi = load_fsi_map(mk_versioned_fsi_file_path(i));
BOOST_REQUIRE(my_finalizers_t::are_equivalent(i, fsi_map_vi, fsi_map_current));
}

} FC_LOG_AND_RETHROW()


BOOST_AUTO_TEST_SUITE_END()
Binary file added unittests/test-data/fsi/safety_v0.dat
Binary file not shown.
Binary file added unittests/test-data/fsi/safety_v1.dat
Binary file not shown.
1 change: 1 addition & 0 deletions unittests/test-data/test-data.hpp.in
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#define UNITTEST_TEST_DATA_DIR "${CMAKE_CURRENT_SOURCE_DIR}/test-data"