Skip to content

Commit

Permalink
PR19521 suggestions, commit e880a5e "Add Coinstats index"
Browse files Browse the repository at this point in the history
  • Loading branch information
@jonatack
jonatack committed Mar 27, 2021
1 parent dd72116 commit e9262d2
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 39 deletions.
54 changes: 24 additions & 30 deletions src/index/coinstatsindex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
#include <undo.h>
#include <validation.h>

constexpr char DB_BLOCK_HASH = 's';
constexpr char DB_BLOCK_HEIGHT = 't';
constexpr char DB_MUHASH = 'M';
static constexpr char DB_BLOCK_HASH = 's';
static constexpr char DB_BLOCK_HEIGHT = 't';
static constexpr char DB_MUHASH = 'M';

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 27, 2021 •
edited
Loading

Author Owner

e880a5e why the different casing between 's', 't' and 'M'? (maybe add an explanatory comment)

namespace {

Expand Down Expand Up @@ -52,7 +52,6 @@ struct DBVal {

struct DBHeightKey {
int height;

explicit DBHeightKey(int height_in) : height(height_in) {}

template <typename Stream>
Expand All @@ -75,12 +74,11 @@ struct DBHeightKey {

struct DBHashKey {
uint256 block_hash;

explicit DBHashKey(const uint256& hash_in) : block_hash(hash_in) {}

SERIALIZE_METHODS(DBHashKey, obj)
{
char prefix = DB_BLOCK_HASH;
char prefix{DB_BLOCK_HASH};
READWRITE(prefix);
if (prefix != DB_BLOCK_HASH) {
throw std::ios_base::failure("Invalid format for coinstatsindex DB hash key");
Expand All @@ -96,19 +94,19 @@ std::unique_ptr<CoinStatsIndex> g_coin_stats_index;

CoinStatsIndex::CoinStatsIndex(size_t n_cache_size, bool f_memory, bool f_wipe)
{
fs::path path = GetDataDir() / "indexes" / "coinstats";
fs::path path{GetDataDir() / "indexes" / "coinstats"};
fs::create_directories(path);

m_db = std::make_unique<CoinStatsIndex::DB>(path / "db", n_cache_size, f_memory, f_wipe);
}

bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
{
CBlockUndo block_undo;
const CAmount block_subsidy{GetBlockSubsidy(pindex->nHeight, Params().GetConsensus())};
m_total_subsidy += block_subsidy;

// Ignore genesis block
CBlockUndo block_undo;

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner

Declare local variables in as local a scope as possible, and as close to the first use as possible (https://google.github.io/styleguide/cppguide.html#local-variables)
if (pindex->nHeight > 0) {
if (!UndoReadFromDisk(block_undo, pindex)) {
return false;
Expand All @@ -119,7 +117,7 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
return false;
}

uint256 expected_block_hash = pindex->pprev->GetBlockHash();
uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
if (read_out.first != expected_block_hash) {
if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
return error("%s: previous block header belongs to unexpected block %s; expected %s",
Expand Down Expand Up @@ -285,10 +283,10 @@ bool CoinStatsIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* n
return BaseIndex::Rewind(current_tip, new_tip);
}

static bool LookupOne(const CDBWrapper& db, const CBlockIndex* block_index, DBVal& result)
static bool LookUpOne(const CDBWrapper& db, const CBlockIndex* block_index, DBVal& result)

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 27, 2021 •
edited
Loading

Author Owner

the verb is "look up" -- two words

"lookup" is a noun or adjective
{
// First check if the result is stored under the height index and the value there matches the
// block hash. This should be the case if the block is on the active chain.
// First check if the result is stored under the height index and the value there
// matches the block hash. This should be the case if the block is on the active chain.

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 29, 2021

Author Owner

feel free to ignore, my editor optimizes these automatically, but it seems more readable this way
std::pair<uint256, DBVal> read_out;
if (!db.Read(DBHeightKey(block_index->nHeight), read_out)) {
return false;
Expand All @@ -298,15 +296,15 @@ static bool LookupOne(const CDBWrapper& db, const CBlockIndex* block_index, DBVa
return true;
}

// If value at the height index corresponds to an different block, the result will be stored in
// the hash index.
// If value at the height index corresponds to an different block, the
// result will be stored in the hash index.
return db.Read(DBHashKey(block_index->GetBlockHash()), result);
}

bool CoinStatsIndex::LookupStats(const CBlockIndex* block_index, CCoinsStats& coins_stats) const
bool CoinStatsIndex::LookUpStats(const CBlockIndex* block_index, CCoinsStats& coins_stats) const
{
DBVal entry;
if (!LookupOne(*m_db, block_index, entry)) {
if (!LookUpOne(*m_db, block_index, entry)) {
return false;
}

Expand Down Expand Up @@ -344,7 +342,7 @@ bool CoinStatsIndex::Init()

if (pindex) {
DBVal entry;
if (!LookupOne(*m_db, pindex, entry)) {
if (!LookUpOne(*m_db, pindex, entry)) {
return false;
}

Expand All @@ -371,12 +369,12 @@ bool CoinStatsIndex::Init()
// Reverse a single block as part of a reorg
bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex)
{
CBlockUndo block_undo;
std::pair<uint256, DBVal> read_out;

const CAmount block_subsidy{GetBlockSubsidy(pindex->nHeight, Params().GetConsensus())};
m_total_subsidy -= block_subsidy;

CBlockUndo block_undo;

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 27, 2021 •
edited
Loading

Author Owner

declare as close as possible to first use
if (pindex->nHeight > 0) {
if (!UndoReadFromDisk(block_undo, pindex)) {
return false;
Expand All @@ -400,9 +398,9 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
const auto& tx = block.vtx.at(i);

for (size_t j = 0; j < tx->vout.size(); ++j) {
const CTxOut& out = tx->vout[j];
COutPoint outpoint = COutPoint(tx->GetHash(), j);
Coin coin = Coin(out, pindex->nHeight, tx->IsCoinBase());
const CTxOut& out{tx->vout[j]};
COutPoint outpoint{COutPoint(tx->GetHash(), j)};
Coin coin{Coin(out, pindex->nHeight, tx->IsCoinBase())};

// Skip unspendable coins
if (coin.out.scriptPubKey.IsUnspendable()) {
Expand All @@ -426,17 +424,17 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex

// The coinbase tx has no undo data since no former output is spent
if (i > 0) {
const auto& tx_undo = block_undo.vtxundo.at(i - 1);
const auto& tx_undo{block_undo.vtxundo.at(i - 1)};

for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) {
Coin coin = tx_undo.vprevout[j];
COutPoint outpoint = COutPoint(tx->vin[j].prevout.hash, tx->vin[j].prevout.n);
Coin coin{tx_undo.vprevout[j]};
COutPoint outpoint{COutPoint(tx->vin[j].prevout.hash, tx->vin[j].prevout.n)};

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 29, 2021

Author Owner

prefer braced initialization

m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));

m_block_prevout_spent_amount -= coin.out.nValue;

m_transaction_output_count++;
++m_transaction_output_count;

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 29, 2021

Author Owner

prefer prefix operators
m_total_amount += coin.out.nValue;
m_bogo_size += GetBogoSize(coin.out.scriptPubKey);
}
Expand Down Expand Up @@ -465,9 +463,5 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
Assert(m_unspendables_scripts == read_out.second.unspendables_scripts);
Assert(m_unspendables_unclaimed_rewards == read_out.second.unspendables_unclaimed_rewards);

if (!m_db->Write(DB_MUHASH, m_muhash)) {
return false;
}

return true;
return m_db->Write(DB_MUHASH, m_muhash);

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 27, 2021 •
edited
Loading

Author Owner

can simplify while keeping the same behavior
}
2 changes: 1 addition & 1 deletion src/index/coinstatsindex.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class CoinStatsIndex final : public BaseIndex
explicit CoinStatsIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false);

// Look up stats for a specific block using CBlockIndex
bool LookupStats(const CBlockIndex* block_index, CCoinsStats& coins_stats) const;
bool LookUpStats(const CBlockIndex* block_index, CCoinsStats& coins_stats) const;
};

/// The global UTXO set hash object.
Expand Down
6 changes: 3 additions & 3 deletions src/node/coinstats.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@

#include <map>

uint64_t GetBogoSize(const CScript& scriptPubKey)
uint64_t GetBogoSize(const CScript& script_pub_key)
{
return 32 /* txid */ +
4 /* vout index */ +
4 /* height + coinbase */ +
8 /* amount */ +
2 /* scriptPubKey len */ +
scriptPubKey.size() /* scriptPubKey */;
script_pub_key.size() /* scriptPubKey */;

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner

naming per developer-notes.md
}

CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin) {
Expand Down Expand Up @@ -105,7 +105,7 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats&

// Use CoinStatsIndex if it is available and hash_type none was requested
if ((stats.m_hash_type == CoinStatsHashType::MUHASH || stats.m_hash_type == CoinStatsHashType::NONE) && g_coin_stats_index && stats.from_index) {
return g_coin_stats_index->LookupStats(pindex, stats);
return g_coin_stats_index->LookUpStats(pindex, stats);
}
stats.from_index = false;

Expand Down
2 changes: 1 addition & 1 deletion src/node/coinstats.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ struct CCoinsStats
//! Calculate statistics about the unspent transaction output set
bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, const std::function<void()>& interruption_point = {}, const CBlockIndex* pindex = nullptr);

uint64_t GetBogoSize(const CScript& scriptPubKey);
uint64_t GetBogoSize(const CScript& script_pub_key);

CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin);

Expand Down
8 changes: 4 additions & 4 deletions src/test/coinstatsindex_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
}

// CoinStatsIndex should not be found before it is started.
BOOST_CHECK(!coin_stats_index.LookupStats(block_index, coin_stats));
BOOST_CHECK(!coin_stats_index.LookUpStats(block_index, coin_stats));

// BlockUntilSyncedToCurrentChain should return false before CoinStatsIndex
// is started.
Expand All @@ -45,10 +45,10 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
LOCK(cs_main);
genesis_block_index = ::ChainActive().Genesis();
}
BOOST_CHECK(coin_stats_index.LookupStats(genesis_block_index, coin_stats));
BOOST_CHECK(coin_stats_index.LookUpStats(genesis_block_index, coin_stats));

// Check that CoinStatsIndex updates with new blocks.
coin_stats_index.LookupStats(block_index, coin_stats);
coin_stats_index.LookUpStats(block_index, coin_stats);

CScript scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
std::vector<CMutableTransaction> noTxns;
Expand All @@ -63,7 +63,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
LOCK(cs_main);
new_block_index = ::ChainActive().Tip();
}
coin_stats_index.LookupStats(new_block_index, new_coin_stats);
coin_stats_index.LookUpStats(new_block_index, new_coin_stats);

BOOST_CHECK(block_index != new_block_index);

Expand Down

0 comments on commit e9262d2

Please sign in to comment.