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

Unchecked cleanup #3600

Merged
merged 9 commits into from
Dec 13, 2021
23 changes: 11 additions & 12 deletions nano/core_test/block_store.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ TEST (bootstrap, simple)
nano::logger_mt logger;
auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants);
ASSERT_TRUE (!store->init_error ());
auto block1 (std::make_shared<nano::send_block> (0, 1, 2, nano::keypair ().prv, 4, 5));
std::shared_ptr<nano::block> block1 = std::make_shared<nano::send_block> (0, 1, 2, nano::keypair ().prv, 4, 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

This clean up looks good, but since you touched the lines anyway, why not auto block = std::make_shared...? Not really important but just in the spirit of if cleaning it up, clean it all the way :D.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one specifically is because std::shared_ptrnano::send_block won't implicitly convert to unchecked_info when passed to unchecked_store::put. It won't do an upcast and also an implicit conversion.

auto transaction (store->tx_begin_write ());
auto block2 (store->unchecked.get (transaction, block1->previous ()));
ASSERT_TRUE (block2.empty ());
Expand All @@ -375,7 +375,7 @@ TEST (unchecked, multiple)
nano::logger_mt logger;
auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants);
ASSERT_TRUE (!store->init_error ());
auto block1 (std::make_shared<nano::send_block> (4, 1, 2, nano::keypair ().prv, 4, 5));
std::shared_ptr<nano::block> block1 = std::make_shared<nano::send_block> (4, 1, 2, nano::keypair ().prv, 4, 5);
auto transaction (store->tx_begin_write ());
auto block2 (store->unchecked.get (transaction, block1->previous ()));
ASSERT_TRUE (block2.empty ());
Expand All @@ -392,7 +392,7 @@ TEST (unchecked, double_put)
nano::logger_mt logger;
auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants);
ASSERT_TRUE (!store->init_error ());
auto block1 (std::make_shared<nano::send_block> (4, 1, 2, nano::keypair ().prv, 4, 5));
std::shared_ptr<nano::block> block1 = std::make_shared<nano::send_block> (4, 1, 2, nano::keypair ().prv, 4, 5);
auto transaction (store->tx_begin_write ());
auto block2 (store->unchecked.get (transaction, block1->previous ()));
ASSERT_TRUE (block2.empty ());
Expand All @@ -407,9 +407,9 @@ TEST (unchecked, multiple_get)
nano::logger_mt logger;
auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants);
ASSERT_TRUE (!store->init_error ());
auto block1 (std::make_shared<nano::send_block> (4, 1, 2, nano::keypair ().prv, 4, 5));
auto block2 (std::make_shared<nano::send_block> (3, 1, 2, nano::keypair ().prv, 4, 5));
auto block3 (std::make_shared<nano::send_block> (5, 1, 2, nano::keypair ().prv, 4, 5));
std::shared_ptr<nano::block> block1 = std::make_shared<nano::send_block> (4, 1, 2, nano::keypair ().prv, 4, 5);
std::shared_ptr<nano::block> block2 = std::make_shared<nano::send_block> (3, 1, 2, nano::keypair ().prv, 4, 5);
std::shared_ptr<nano::block> block3 = std::make_shared<nano::send_block> (5, 1, 2, nano::keypair ().prv, 4, 5);
{
auto transaction (store->tx_begin_write ());
store->unchecked.put (transaction, block1->previous (), block1); // unchecked1
Expand Down Expand Up @@ -482,8 +482,7 @@ TEST (block_store, empty_bootstrap)
auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants);
ASSERT_TRUE (!store->init_error ());
auto transaction (store->tx_begin_read ());
auto begin (store->unchecked.begin (transaction));
auto end (store->unchecked.end ());
auto [begin, end] = store->unchecked.full_range (transaction);
ASSERT_EQ (end, begin);
}

Expand All @@ -492,7 +491,7 @@ TEST (block_store, one_bootstrap)
nano::logger_mt logger;
auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants);
ASSERT_TRUE (!store->init_error ());
auto block1 (std::make_shared<nano::send_block> (0, 1, 2, nano::keypair ().prv, 4, 5));
std::shared_ptr<nano::block> block1 = std::make_shared<nano::send_block> (0, 1, 2, nano::keypair ().prv, 4, 5);
auto transaction (store->tx_begin_write ());
store->unchecked.put (transaction, block1->hash (), block1);
auto begin (store->unchecked.begin (transaction));
Expand Down Expand Up @@ -925,8 +924,8 @@ TEST (block_store, DISABLED_change_dupsort) // Unchecked is no longer dupsort ta
auto transaction (store.tx_begin_write ());
ASSERT_EQ (0, mdb_drop (store.env.tx (transaction), store.unchecked_handle, 1));
ASSERT_EQ (0, mdb_dbi_open (store.env.tx (transaction), "unchecked", MDB_CREATE, &store.unchecked_handle));
auto send1 (std::make_shared<nano::send_block> (0, 0, 0, nano::dev::genesis_key.prv, nano::dev::genesis_key.pub, 0));
auto send2 (std::make_shared<nano::send_block> (1, 0, 0, nano::dev::genesis_key.prv, nano::dev::genesis_key.pub, 0));
std::shared_ptr<nano::block> send1 = std::make_shared<nano::send_block> (0, 0, 0, nano::dev::genesis_key.prv, nano::dev::genesis_key.pub, 0);
std::shared_ptr<nano::block> send2 = std::make_shared<nano::send_block> (1, 0, 0, nano::dev::genesis_key.prv, nano::dev::genesis_key.pub, 0);
ASSERT_NE (send1->hash (), send2->hash ());
store.unchecked.put (transaction, send1->hash (), send1);
store.unchecked.put (transaction, send1->hash (), send2);
Expand Down Expand Up @@ -2026,7 +2025,7 @@ TEST (rocksdb_block_store, tombstone_count)
auto store = std::make_unique<nano::rocksdb_store> (logger, nano::unique_path (), nano::dev::constants);
ASSERT_TRUE (!store->init_error ());
auto transaction = store->tx_begin_write ();
auto block1 (std::make_shared<nano::send_block> (0, 1, 2, nano::keypair ().prv, 4, 5));
std::shared_ptr<nano::block> block1 = std::make_shared<nano::send_block> (0, 1, 2, nano::keypair ().prv, 4, 5);
store->unchecked.put (transaction, block1->previous (), block1);
ASSERT_EQ (store->tombstone_map.at (nano::tables::unchecked).num_since_last_flush.load (), 0);
store->unchecked.del (transaction, nano::unchecked_key (block1->previous (), block1->hash ()));
Expand Down
18 changes: 9 additions & 9 deletions nano/core_test/ledger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3627,15 +3627,15 @@ TEST (ledger, migrate_lmdb_to_rocksdb)
nano::ledger ledger (store, stats, nano::dev::constants);
nano::work_pool pool{ nano::dev::network_params.network, std::numeric_limits<unsigned>::max () };

auto send = nano::state_block_builder ()
.account (nano::dev::genesis_key.pub)
.previous (nano::dev::genesis->hash ())
.representative (0)
.link (nano::account (10))
.balance (nano::dev::constants.genesis_amount - 100)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*pool.generate (nano::dev::genesis->hash ()))
.build_shared ();
std::shared_ptr<nano::block> send = nano::state_block_builder ()
.account (nano::dev::genesis_key.pub)
.previous (nano::dev::genesis->hash ())
.representative (0)
.link (nano::account (10))
.balance (nano::dev::constants.genesis_amount - 100)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*pool.generate (nano::dev::genesis->hash ()))
.build_shared ();

nano::endpoint_key endpoint_key (address.to_bytes (), port);
auto version = 99;
Expand Down
22 changes: 12 additions & 10 deletions nano/core_test/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3308,16 +3308,18 @@ TEST (node, block_processor_signatures)
.build_shared ();
send4->signature.bytes[32] ^= 0x1;
// Invalid signature bit (force)
auto send5 = builder.make_block ()
.account (nano::dev::genesis_key.pub)
.previous (send3->hash ())
.representative (nano::dev::genesis_key.pub)
.balance (nano::dev::constants.genesis_amount - 5 * nano::Gxrb_ratio)
.link (key3.pub)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*node1.work_generate_blocking (send3->hash ()))
.build_shared ();
send5->signature.bytes[31] ^= 0x1;
std::shared_ptr<nano::block> send5 = builder.make_block ()
.account (nano::dev::genesis_key.pub)
.previous (send3->hash ())
.representative (nano::dev::genesis_key.pub)
.balance (nano::dev::constants.genesis_amount - 5 * nano::Gxrb_ratio)
.link (key3.pub)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*node1.work_generate_blocking (send3->hash ()))
.build_shared ();
auto signature = send5->block_signature ();
signature.bytes[31] ^= 0x1;
send5->signature_set (signature);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's more clean to get the signature, touch it, then set it back, but was there a functional problem with changing it in-place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're holding a pointer to nano::block instead of the specific block type, the signature field is no longer directly accessible.

// Invalid signature to unchecked
{
auto transaction (node1.store.tx_begin_write ());
Expand Down
2 changes: 1 addition & 1 deletion nano/nano_node/entry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ int main (int argc, char * const * argv)
}

// Check all unchecked keys for matching frontier hashes. Indicates an issue with process_batch algorithm
for (auto i (node->store.unchecked.begin (transaction)), n (node->store.unchecked.end ()); i != n; ++i)
for (auto [i, n] = node->store.unchecked.full_range (transaction); i != n; ++i)
{
auto it = frontier_hashes.find (i->first.key ());
if (it != frontier_hashes.cend ())
Expand Down
12 changes: 3 additions & 9 deletions nano/node/blockprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,9 +382,7 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction
info_a.modified = nano::seconds_since_epoch ();
}

nano::unchecked_key unchecked_key (block->previous (), hash);
node.store.unchecked.put (transaction_a, unchecked_key, info_a);

node.store.unchecked.put (transaction_a, block->previous (), info_a);
events_a.events.emplace_back ([this, hash] (nano::transaction const & /* unused */) { this->node.gap_cache.add (hash); });

node.stats.inc (nano::stat::type::ledger, nano::stat::detail::gap_previous);
Expand All @@ -402,9 +400,7 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction
info_a.modified = nano::seconds_since_epoch ();
}

nano::unchecked_key unchecked_key (node.ledger.block_source (transaction_a, *(block)), hash);
node.store.unchecked.put (transaction_a, unchecked_key, info_a);

node.store.unchecked.put (transaction_a, node.ledger.block_source (transaction_a, *(block)), info_a);
events_a.events.emplace_back ([this, hash] (nano::transaction const & /* unused */) { this->node.gap_cache.add (hash); });

node.stats.inc (nano::stat::type::ledger, nano::stat::detail::gap_source);
Expand All @@ -422,9 +418,7 @@ nano::process_return nano::block_processor::process_one (nano::write_transaction
info_a.modified = nano::seconds_since_epoch ();
}

nano::unchecked_key unchecked_key (block->account (), hash); // Specific unchecked key starting with epoch open block account public key
node.store.unchecked.put (transaction_a, unchecked_key, info_a);

node.store.unchecked.put (transaction_a, block->account (), info_a); // Specific unchecked key starting with epoch open block account public key
node.stats.inc (nano::stat::type::ledger, nano::stat::detail::gap_source);
break;
}
Expand Down
6 changes: 3 additions & 3 deletions nano/node/json_handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4026,7 +4026,7 @@ void nano::json_handler::unchecked ()
{
boost::property_tree::ptree unchecked;
auto transaction (node.store.tx_begin_read ());
for (auto i (node.store.unchecked.begin (transaction)), n (node.store.unchecked.end ()); i != n && unchecked.size () < count; ++i)
for (auto [i, n] = node.store.unchecked.full_range (transaction); i != n && unchecked.size () < count; ++i)
{
nano::unchecked_info const & info (i->second);
if (json_block_l)
Expand Down Expand Up @@ -4064,7 +4064,7 @@ void nano::json_handler::unchecked_get ()
if (!ec)
{
auto transaction (node.store.tx_begin_read ());
for (auto i (node.store.unchecked.begin (transaction)), n (node.store.unchecked.end ()); i != n; ++i)
for (auto [i, n] = node.store.unchecked.full_range (transaction); i != n; ++i)
{
nano::unchecked_key const & key (i->first);
if (key.hash == hash)
Expand Down Expand Up @@ -4112,7 +4112,7 @@ void nano::json_handler::unchecked_keys ()
{
boost::property_tree::ptree unchecked;
auto transaction (node.store.tx_begin_read ());
for (auto i (node.store.unchecked.begin (transaction, nano::unchecked_key (key, 0))), n (node.store.unchecked.end ()); i != n && unchecked.size () < count; ++i)
for (auto [i, n] = node.store.unchecked.equal_range (transaction, key); i != n && unchecked.size () < count; ++i)
{
boost::property_tree::ptree entry;
nano::unchecked_info const & info (i->second);
Expand Down
11 changes: 0 additions & 11 deletions nano/node/lmdb/lmdb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -811,17 +811,6 @@ void nano::mdb_store::create_backup_file (nano::mdb_env & env_a, boost::filesyst
}
}

std::vector<nano::unchecked_info> nano::unchecked_mdb_store::get (nano::transaction const & transaction_a, nano::block_hash const & hash_a)
{
std::vector<nano::unchecked_info> result;
for (auto i (begin (transaction_a, nano::unchecked_key (hash_a, 0))), n (end ()); i != n && i->first.key () == hash_a; ++i)
{
nano::unchecked_info const & unchecked_info (i->second);
result.push_back (unchecked_info);
}
return result;
}

nano::unchecked_mdb_store::unchecked_mdb_store (nano::mdb_store & mdb_store_a) :
unchecked_store_partial<MDB_val, mdb_store> (mdb_store_a){};

Expand Down
1 change: 0 additions & 1 deletion nano/node/lmdb/lmdb.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ class unchecked_mdb_store : public unchecked_store_partial<MDB_val, mdb_store>
{
public:
explicit unchecked_mdb_store (nano::mdb_store &);
std::vector<nano::unchecked_info> get (nano::transaction const &, nano::block_hash const &);
};

/**
Expand Down
2 changes: 1 addition & 1 deletion nano/node/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ void nano::node::unchecked_cleanup ()
auto const now (nano::seconds_since_epoch ());
auto const transaction (store.tx_begin_read ());
// Max 1M records to clean, max 2 minutes reading to prevent slow i/o systems issues
for (auto i (store.unchecked.begin (transaction)), n (store.unchecked.end ()); i != n && cleaning_list.size () < 1024 * 1024 && nano::seconds_since_epoch () - now < 120; ++i)
for (auto [i, n] = store.unchecked.full_range (transaction); i != n && cleaning_list.size () < 1024 * 1024 && nano::seconds_since_epoch () - now < 120; ++i)
{
nano::unchecked_key const & key (i->first);
nano::unchecked_info const & info (i->second);
Expand Down
40 changes: 0 additions & 40 deletions nano/node/rocksdb/rocksdb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,6 @@ rocksdb::ColumnFamilyOptions nano::rocksdb_store::get_cf_options (std::string co

// Create prefix bloom for memtable with the size of write_buffer_size * memtable_prefix_bloom_size_ratio
cf_options.memtable_prefix_bloom_size_ratio = 0.25;
// The prefix to use is the size of the unchecked key (root)
cf_options.prefix_extractor.reset (rocksdb::NewFixedPrefixTransform (sizeof (nano::root)));

// Number of files in level 0 which triggers compaction. Size of L0 and L1 should be kept similar as this is the only compaction which is single threaded
cf_options.level0_file_num_compaction_trigger = 2;
Expand Down Expand Up @@ -624,44 +622,6 @@ nano::version_rocksdb_store::version_rocksdb_store (nano::rocksdb_store & rocksd
nano::version_store_partial<rocksdb::Slice, nano::rocksdb_store> (rocksdb_store_a),
rocksdb_store{ rocksdb_store_a } {};

std::vector<nano::unchecked_info> nano::unchecked_rocksdb_store::get (nano::transaction const & transaction_a, nano::block_hash const & hash_a)
{
auto cf = rocksdb_store.table_to_column_family (tables::unchecked);

std::unique_ptr<rocksdb::Iterator> iter;
nano::qualified_root upper (hash_a, nano::block_hash (std::numeric_limits<nano::uint256_t>::max ()));
nano::rocksdb_val upper_bound (sizeof (upper), (void *)&upper);
if (is_read (transaction_a))
{
auto read_options = snapshot_options (transaction_a);
read_options.prefix_same_as_start = true;
read_options.auto_prefix_mode = true;
read_options.iterate_upper_bound = upper_bound;
read_options.fill_cache = false;
iter.reset (rocksdb_store.db->NewIterator (read_options, cf));
}
else
{
rocksdb::ReadOptions read_options;
read_options.prefix_same_as_start = true;
read_options.auto_prefix_mode = true;
read_options.iterate_upper_bound = upper_bound;
read_options.fill_cache = false;
iter.reset (rocksdb_store.tx (transaction_a)->GetIterator (read_options, cf));
}

// Uses prefix extraction
std::vector<nano::unchecked_info> result;

auto prefix = nano::rocksdb_val (hash_a);
for (iter->Seek (prefix); iter->Valid () && iter->key ().starts_with (prefix); iter->Next ())
{
auto unchecked_info = static_cast<nano::unchecked_info> (nano::rocksdb_val (iter->value ()));
result.push_back (unchecked_info);
}
return result;
}

void nano::rocksdb_store::construct_column_family_mutexes ()
{
for (auto table : all_tables ())
Expand Down
1 change: 0 additions & 1 deletion nano/node/rocksdb/rocksdb.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class unchecked_rocksdb_store : public unchecked_store_partial<rocksdb::Slice, n
{
public:
explicit unchecked_rocksdb_store (nano::rocksdb_store &);
std::vector<nano::unchecked_info> get (nano::transaction const &, nano::block_hash const &);

private:
nano::rocksdb_store & rocksdb_store;
Expand Down
10 changes: 10 additions & 0 deletions nano/secure/common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,11 @@ nano::unchecked_info::unchecked_info (std::shared_ptr<nano::block> const & block
{
}

nano::unchecked_info::unchecked_info (std::shared_ptr<nano::block> const & block) :
unchecked_info{ block, block->account (), nano::seconds_since_epoch (), nano::signature_verification::unknown }
{
}

void nano::unchecked_info::serialize (nano::stream & stream_a) const
{
debug_assert (block != nullptr);
Expand Down Expand Up @@ -861,6 +866,11 @@ nano::wallet_id nano::random_wallet_id ()
return wallet_id;
}

nano::unchecked_key::unchecked_key (nano::hash_or_account const & dependency) :
unchecked_key{ dependency, 0 }
{
}

nano::unchecked_key::unchecked_key (nano::hash_or_account const & previous_a, nano::block_hash const & hash_a) :
previous (previous_a.hash),
hash (hash_a)
Expand Down
2 changes: 2 additions & 0 deletions nano/secure/common.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ class unchecked_key final
{
public:
unchecked_key () = default;
explicit unchecked_key (nano::hash_or_account const & dependency);
unchecked_key (nano::hash_or_account const &, nano::block_hash const &);
unchecked_key (nano::uint512_union const &);
bool deserialize (nano::stream &);
Expand Down Expand Up @@ -199,6 +200,7 @@ class unchecked_info final
public:
unchecked_info () = default;
unchecked_info (std::shared_ptr<nano::block> const &, nano::account const &, uint64_t, nano::signature_verification = nano::signature_verification::unknown, bool = false);
unchecked_info (std::shared_ptr<nano::block> const &);
void serialize (nano::stream &) const;
bool deserialize (nano::stream &);
std::shared_ptr<nano::block> block;
Expand Down
2 changes: 1 addition & 1 deletion nano/secure/ledger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (boost::filesystem::path const & data
for (; i != n; ++i)
{
auto rocksdb_transaction (rocksdb_store->tx_begin_write ({}, { nano::tables::unchecked }));
rocksdb_store->unchecked.put (rocksdb_transaction, i->first, i->second);
rocksdb_store->unchecked.put (rocksdb_transaction, i->first.previous, i->second);
}
});

Expand Down
30 changes: 30 additions & 0 deletions nano/secure/store.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,33 @@ nano::store::store (
{
}
// clang-format on

auto nano::unchecked_store::equal_range (nano::transaction const & transaction, nano::block_hash const & dependency) -> std::pair<iterator, iterator>
{
nano::unchecked_key begin_l{ dependency, 0 };
nano::unchecked_key end_l{ nano::block_hash{ dependency.number () + 1 }, 0 };
// Adjust for edge case where number () + 1 wraps around.
auto end_iter = begin_l.previous < end_l.previous ? lower_bound (transaction, end_l) : end ();
return std::make_pair (lower_bound (transaction, begin_l), std::move (end_iter));
}

auto nano::unchecked_store::full_range (nano::transaction const & transaction) -> std::pair<iterator, iterator>
{
return std::make_pair (begin (transaction), end ());
}

std::vector<nano::unchecked_info> nano::unchecked_store::get (nano::transaction const & transaction, nano::block_hash const & dependency)
{
auto range = equal_range (transaction, dependency);
std::vector<nano::unchecked_info> result;
auto & i = range.first;
auto & n = range.second;
for (; i != n; ++i)
{
auto const & key = i->first;
auto const & value = i->second;
debug_assert (key.hash == value.block->hash ());
result.push_back (value);
}
return result;
}
Loading