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.

Sign up for GitHub

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

Jump to bottom

index, rpc: Coinstatsindex follow-ups #22047

Merged
merged 8 commits into from
Jul 28, 2021
Merged

index, rpc: Coinstatsindex follow-ups #22047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8ea8c92
index: Avoid unnecessary type casts in coinstatsindex
fjahr May 23, 2021
fb65dde
scripted-diff: Fix coinstats data member names
fjahr Jun 2, 2021
1e38423
index: Use batch writing in coinstatsindex WriteBlock
fjahr May 23, 2021
01386bf
Index: Return early from failed coinstatsindex init
fjahr May 23, 2021
a5f6791
rpc: Add missing gettxoutsetinfo help docs
fjahr May 23, 2021
d4356d4
rpc: Block until synced if coinstatsindex is used in gettxoutsetinfo
fjahr May 24, 2021
5b3d4e7
Index: Improve logging in coinstatsindex
fjahr May 24, 2021
779e638
coinstats: Add comments for new coinstatsindex values
fjahr Jun 3, 2021
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
187 changes: 97 additions & 90 deletions src/index/coinstatsindex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ struct DBVal {
uint64_t bogo_size;
CAmount total_amount;
CAmount total_subsidy;
Copy link
Member

Choose a reason for hiding this comment

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

b778ac8: I think the total_ prefix is more clear (and consistent with total_amount which you wanted to keep). So maybe use the second approach @MarcoFalke suggested:

unspendables are also "total", but don't mention it. Might be good to remove the "total_" here, or add it to unspendables?

More importantly, can you add comments explaining how these values are used? Their cumulative nature is a bit counter intuitive, though it makes sense for rollbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, changed the names to all use total_ and added comments explaining each of the new values in coinstats.h a bit more clearly.

CAmount block_unspendable_amount;
CAmount block_prevout_spent_amount;
CAmount block_new_outputs_ex_coinbase_amount;
CAmount block_coinbase_amount;
CAmount unspendables_genesis_block;
CAmount unspendables_bip30;
CAmount unspendables_scripts;
CAmount unspendables_unclaimed_rewards;
CAmount total_unspendable_amount;
CAmount total_prevout_spent_amount;
CAmount total_new_outputs_ex_coinbase_amount;
CAmount total_coinbase_amount;
Copy link
Member

Choose a reason for hiding this comment

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

unspendables are also "total", but don't mention it. Might be good to remove the "total_" here, or add it to unspendables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped the total_ everywhere except for the m_total_amount, it seemed right to keep it in this case since it's closed to the corresponding legacy name nTotalAmount that is also still in use.

CAmount total_unspendables_genesis_block;
CAmount total_unspendables_bip30;
CAmount total_unspendables_scripts;
CAmount total_unspendables_unclaimed_rewards;

SERIALIZE_METHODS(DBVal, obj)
{
Expand All @@ -40,14 +40,14 @@ struct DBVal {
READWRITE(obj.bogo_size);
READWRITE(obj.total_amount);
READWRITE(obj.total_subsidy);
READWRITE(obj.block_unspendable_amount);
READWRITE(obj.block_prevout_spent_amount);
READWRITE(obj.block_new_outputs_ex_coinbase_amount);
READWRITE(obj.block_coinbase_amount);
READWRITE(obj.unspendables_genesis_block);
READWRITE(obj.unspendables_bip30);
READWRITE(obj.unspendables_scripts);
READWRITE(obj.unspendables_unclaimed_rewards);
READWRITE(obj.total_unspendable_amount);
READWRITE(obj.total_prevout_spent_amount);
READWRITE(obj.total_new_outputs_ex_coinbase_amount);
READWRITE(obj.total_coinbase_amount);
READWRITE(obj.total_unspendables_genesis_block);
READWRITE(obj.total_unspendables_bip30);
READWRITE(obj.total_unspendables_scripts);
READWRITE(obj.total_unspendables_unclaimed_rewards);
}
};

Expand Down Expand Up @@ -122,9 +122,12 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)

uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
if (read_out.first != expected_block_hash) {
LogPrintf("WARNING: previous block header belongs to unexpected block %s; expected %s\n",
read_out.first.ToString(), expected_block_hash.ToString());

if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
return error("%s: previous block header belongs to unexpected block %s; expected %s",
__func__, read_out.first.ToString(), expected_block_hash.ToString());
return error("%s: previous block header not found; expected %s",
__func__, expected_block_hash.ToString());
}
}

Expand All @@ -138,29 +141,29 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)

// Skip duplicate txid coinbase transactions (BIP30).
if (is_bip30_block && tx->IsCoinBase()) {
m_block_unspendable_amount += block_subsidy;
m_unspendables_bip30 += block_subsidy;
m_total_unspendable_amount += block_subsidy;
m_total_unspendables_bip30 += block_subsidy;
continue;
}

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

// Skip unspendable coins
if (coin.out.scriptPubKey.IsUnspendable()) {
m_block_unspendable_amount += coin.out.nValue;
m_unspendables_scripts += coin.out.nValue;
m_total_unspendable_amount += coin.out.nValue;
m_total_unspendables_scripts += coin.out.nValue;
continue;
}

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

if (tx->IsCoinBase()) {
m_block_coinbase_amount += coin.out.nValue;
m_total_coinbase_amount += coin.out.nValue;
} else {
m_block_new_outputs_ex_coinbase_amount += coin.out.nValue;
m_total_new_outputs_ex_coinbase_amount += coin.out.nValue;
}

++m_transaction_output_count;
Expand All @@ -178,7 +181,7 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)

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

m_block_prevout_spent_amount += coin.out.nValue;
m_total_prevout_spent_amount += coin.out.nValue;

--m_transaction_output_count;
m_total_amount -= coin.out.nValue;
Expand All @@ -188,38 +191,41 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
}
} else {
// genesis block
m_block_unspendable_amount += block_subsidy;
m_unspendables_genesis_block += block_subsidy;
m_total_unspendable_amount += block_subsidy;
m_total_unspendables_genesis_block += block_subsidy;
}

// If spent prevouts + block subsidy are still a higher amount than
// new outputs + coinbase + current unspendable amount this means
// the miner did not claim the full block reward. Unclaimed block
// rewards are also unspendable.
const CAmount unclaimed_rewards{(m_block_prevout_spent_amount + m_total_subsidy) - (m_block_new_outputs_ex_coinbase_amount + m_block_coinbase_amount + m_block_unspendable_amount)};
m_block_unspendable_amount += unclaimed_rewards;
m_unspendables_unclaimed_rewards += unclaimed_rewards;
const CAmount unclaimed_rewards{(m_total_prevout_spent_amount + m_total_subsidy) - (m_total_new_outputs_ex_coinbase_amount + m_total_coinbase_amount + m_total_unspendable_amount)};
m_total_unspendable_amount += unclaimed_rewards;
m_total_unspendables_unclaimed_rewards += unclaimed_rewards;

std::pair<uint256, DBVal> value;
value.first = pindex->GetBlockHash();
value.second.transaction_output_count = m_transaction_output_count;
value.second.bogo_size = m_bogo_size;
value.second.total_amount = m_total_amount;
value.second.total_subsidy = m_total_subsidy;
value.second.block_unspendable_amount = m_block_unspendable_amount;
value.second.block_prevout_spent_amount = m_block_prevout_spent_amount;
value.second.block_new_outputs_ex_coinbase_amount = m_block_new_outputs_ex_coinbase_amount;
value.second.block_coinbase_amount = m_block_coinbase_amount;
value.second.unspendables_genesis_block = m_unspendables_genesis_block;
value.second.unspendables_bip30 = m_unspendables_bip30;
value.second.unspendables_scripts = m_unspendables_scripts;
value.second.unspendables_unclaimed_rewards = m_unspendables_unclaimed_rewards;
value.second.total_unspendable_amount = m_total_unspendable_amount;
value.second.total_prevout_spent_amount = m_total_prevout_spent_amount;
value.second.total_new_outputs_ex_coinbase_amount = m_total_new_outputs_ex_coinbase_amount;
value.second.total_coinbase_amount = m_total_coinbase_amount;
value.second.total_unspendables_genesis_block = m_total_unspendables_genesis_block;
value.second.total_unspendables_bip30 = m_total_unspendables_bip30;
value.second.total_unspendables_scripts = m_total_unspendables_scripts;
value.second.total_unspendables_unclaimed_rewards = m_total_unspendables_unclaimed_rewards;

uint256 out;
m_muhash.Finalize(out);
value.second.muhash = out;

return m_db->Write(DBHeightKey(pindex->nHeight), value) && m_db->Write(DB_MUHASH, m_muhash);
CDBBatch batch(*m_db);
batch.Write(DBHeightKey(pindex->nHeight), value);
batch.Write(DB_MUHASH, m_muhash);
return m_db->WriteBatch(batch);
}

static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
Expand Down Expand Up @@ -317,14 +323,14 @@ bool CoinStatsIndex::LookUpStats(const CBlockIndex* block_index, CCoinsStats& co
coins_stats.nBogoSize = entry.bogo_size;
coins_stats.nTotalAmount = entry.total_amount;
coins_stats.total_subsidy = entry.total_subsidy;
coins_stats.block_unspendable_amount = entry.block_unspendable_amount;
coins_stats.block_prevout_spent_amount = entry.block_prevout_spent_amount;
coins_stats.block_new_outputs_ex_coinbase_amount = entry.block_new_outputs_ex_coinbase_amount;
coins_stats.block_coinbase_amount = entry.block_coinbase_amount;
coins_stats.unspendables_genesis_block = entry.unspendables_genesis_block;
coins_stats.unspendables_bip30 = entry.unspendables_bip30;
coins_stats.unspendables_scripts = entry.unspendables_scripts;
coins_stats.unspendables_unclaimed_rewards = entry.unspendables_unclaimed_rewards;
coins_stats.total_unspendable_amount = entry.total_unspendable_amount;
coins_stats.total_prevout_spent_amount = entry.total_prevout_spent_amount;
coins_stats.total_new_outputs_ex_coinbase_amount = entry.total_new_outputs_ex_coinbase_amount;
coins_stats.total_coinbase_amount = entry.total_coinbase_amount;
coins_stats.total_unspendables_genesis_block = entry.total_unspendables_genesis_block;
coins_stats.total_unspendables_bip30 = entry.total_unspendables_bip30;
coins_stats.total_unspendables_scripts = entry.total_unspendables_scripts;
coins_stats.total_unspendables_unclaimed_rewards = entry.total_unspendables_unclaimed_rewards;

return true;
}
Expand All @@ -341,33 +347,31 @@ bool CoinStatsIndex::Init()
}
}

if (BaseIndex::Init()) {
const CBlockIndex* pindex{CurrentIndex()};
if (!BaseIndex::Init()) return false;

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

m_transaction_output_count = entry.transaction_output_count;
m_bogo_size = entry.bogo_size;
m_total_amount = entry.total_amount;
m_total_subsidy = entry.total_subsidy;
m_block_unspendable_amount = entry.block_unspendable_amount;
m_block_prevout_spent_amount = entry.block_prevout_spent_amount;
m_block_new_outputs_ex_coinbase_amount = entry.block_new_outputs_ex_coinbase_amount;
m_block_coinbase_amount = entry.block_coinbase_amount;
m_unspendables_genesis_block = entry.unspendables_genesis_block;
m_unspendables_bip30 = entry.unspendables_bip30;
m_unspendables_scripts = entry.unspendables_scripts;
m_unspendables_unclaimed_rewards = entry.unspendables_unclaimed_rewards;
if (pindex) {
DBVal entry;
if (!LookUpOne(*m_db, pindex, entry)) {
return false;
}

return true;
m_transaction_output_count = entry.transaction_output_count;
m_bogo_size = entry.bogo_size;
m_total_amount = entry.total_amount;
m_total_subsidy = entry.total_subsidy;
m_total_unspendable_amount = entry.total_unspendable_amount;
m_total_prevout_spent_amount = entry.total_prevout_spent_amount;
m_total_new_outputs_ex_coinbase_amount = entry.total_new_outputs_ex_coinbase_amount;
m_total_coinbase_amount = entry.total_coinbase_amount;
m_total_unspendables_genesis_block = entry.total_unspendables_genesis_block;
m_total_unspendables_bip30 = entry.total_unspendables_bip30;
m_total_unspendables_scripts = entry.total_unspendables_scripts;
m_total_unspendables_unclaimed_rewards = entry.total_unspendables_unclaimed_rewards;
}

return false;
return true;
}

// Reverse a single block as part of a reorg
Expand All @@ -391,9 +395,12 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex

uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
if (read_out.first != expected_block_hash) {
LogPrintf("WARNING: previous block header belongs to unexpected block %s; expected %s\n",
read_out.first.ToString(), expected_block_hash.ToString());

if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
return error("%s: previous block header belongs to unexpected block %s; expected %s",
__func__, read_out.first.ToString(), expected_block_hash.ToString());
return error("%s: previous block header not found; expected %s",
__func__, expected_block_hash.ToString());
Copy link
Contributor

@jonatack jonatack Jul 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

18aa58b I did not test this change; it may be good to add a test here and for the same logging change above lines 125-130

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can really only hit this in cases of data corruption (or a bug somewhere else in the code). So that makes it really hard to test. I have made a note to look into it some more and think about how to add coverage without too much effort.

}
}
}
Expand All @@ -402,24 +409,24 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
for (size_t i = 0; i < block.vtx.size(); ++i) {
const auto& tx{block.vtx.at(i)};

for (size_t j = 0; j < tx->vout.size(); ++j) {
for (uint32_t j = 0; j < tx->vout.size(); ++j) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't vector<…>::size() return a size_t, making this effectively a cast?
(apparently not necessarily: which size_type it returns is C++ library dependent)

const CTxOut& out{tx->vout[j]};
COutPoint outpoint{tx->GetHash(), static_cast<uint32_t>(j)};
COutPoint outpoint{tx->GetHash(), j};
Coin coin{out, pindex->nHeight, tx->IsCoinBase()};

// Skip unspendable coins
if (coin.out.scriptPubKey.IsUnspendable()) {
m_block_unspendable_amount -= coin.out.nValue;
m_unspendables_scripts -= coin.out.nValue;
m_total_unspendable_amount -= coin.out.nValue;
m_total_unspendables_scripts -= coin.out.nValue;
continue;
}

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

if (tx->IsCoinBase()) {
m_block_coinbase_amount -= coin.out.nValue;
m_total_coinbase_amount -= coin.out.nValue;
} else {
m_block_new_outputs_ex_coinbase_amount -= coin.out.nValue;
m_total_new_outputs_ex_coinbase_amount -= coin.out.nValue;
}

--m_transaction_output_count;
Expand All @@ -437,7 +444,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex

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

m_block_prevout_spent_amount -= coin.out.nValue;
m_total_prevout_spent_amount -= coin.out.nValue;

m_transaction_output_count++;
m_total_amount += coin.out.nValue;
Expand All @@ -446,9 +453,9 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
}
}

const CAmount unclaimed_rewards{(m_block_new_outputs_ex_coinbase_amount + m_block_coinbase_amount + m_block_unspendable_amount) - (m_block_prevout_spent_amount + m_total_subsidy)};
m_block_unspendable_amount -= unclaimed_rewards;
m_unspendables_unclaimed_rewards -= unclaimed_rewards;
const CAmount unclaimed_rewards{(m_total_new_outputs_ex_coinbase_amount + m_total_coinbase_amount + m_total_unspendable_amount) - (m_total_prevout_spent_amount + m_total_subsidy)};
m_total_unspendable_amount -= unclaimed_rewards;
m_total_unspendables_unclaimed_rewards -= unclaimed_rewards;

// Check that the rolled back internal values are consistent with the DB read out
uint256 out;
Expand All @@ -459,14 +466,14 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
Assert(m_total_amount == read_out.second.total_amount);
Assert(m_bogo_size == read_out.second.bogo_size);
Assert(m_total_subsidy == read_out.second.total_subsidy);
Assert(m_block_unspendable_amount == read_out.second.block_unspendable_amount);
Assert(m_block_prevout_spent_amount == read_out.second.block_prevout_spent_amount);
Assert(m_block_new_outputs_ex_coinbase_amount == read_out.second.block_new_outputs_ex_coinbase_amount);
Assert(m_block_coinbase_amount == read_out.second.block_coinbase_amount);
Assert(m_unspendables_genesis_block == read_out.second.unspendables_genesis_block);
Assert(m_unspendables_bip30 == read_out.second.unspendables_bip30);
Assert(m_unspendables_scripts == read_out.second.unspendables_scripts);
Assert(m_unspendables_unclaimed_rewards == read_out.second.unspendables_unclaimed_rewards);
Assert(m_total_unspendable_amount == read_out.second.total_unspendable_amount);
Assert(m_total_prevout_spent_amount == read_out.second.total_prevout_spent_amount);
Assert(m_total_new_outputs_ex_coinbase_amount == read_out.second.total_new_outputs_ex_coinbase_amount);
Assert(m_total_coinbase_amount == read_out.second.total_coinbase_amount);
Assert(m_total_unspendables_genesis_block == read_out.second.total_unspendables_genesis_block);
Assert(m_total_unspendables_bip30 == read_out.second.total_unspendables_bip30);
Assert(m_total_unspendables_scripts == read_out.second.total_unspendables_scripts);
Assert(m_total_unspendables_unclaimed_rewards == read_out.second.total_unspendables_unclaimed_rewards);

return m_db->Write(DB_MUHASH, m_muhash);
}
16 changes: 8 additions & 8 deletions src/index/coinstatsindex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ class CoinStatsIndex final : public BaseIndex
uint64_t m_bogo_size{0};
CAmount m_total_amount{0};
CAmount m_total_subsidy{0};
CAmount m_block_unspendable_amount{0};
CAmount m_block_prevout_spent_amount{0};
CAmount m_block_new_outputs_ex_coinbase_amount{0};
CAmount m_block_coinbase_amount{0};
CAmount m_unspendables_genesis_block{0};
CAmount m_unspendables_bip30{0};
CAmount m_unspendables_scripts{0};
CAmount m_unspendables_unclaimed_rewards{0};
CAmount m_total_unspendable_amount{0};
CAmount m_total_prevout_spent_amount{0};
CAmount m_total_new_outputs_ex_coinbase_amount{0};
CAmount m_total_coinbase_amount{0};
CAmount m_total_unspendables_genesis_block{0};
CAmount m_total_unspendables_bip30{0};
CAmount m_total_unspendables_scripts{0};
CAmount m_total_unspendables_unclaimed_rewards{0};

bool ReverseBlock(const CBlock& block, const CBlockIndex* pindex);

Expand Down
26 changes: 18 additions & 8 deletions src/node/coinstats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,25 @@ struct CCoinsStats
bool index_used{false};

// Following values are only available from coinstats index

//! Total cumulative amount of block subsidies up to and including this block
CAmount total_subsidy{0};
CAmount block_unspendable_amount{0};
CAmount block_prevout_spent_amount{0};
CAmount block_new_outputs_ex_coinbase_amount{0};
CAmount block_coinbase_amount{0};
CAmount unspendables_genesis_block{0};
CAmount unspendables_bip30{0};
CAmount unspendables_scripts{0};
CAmount unspendables_unclaimed_rewards{0};
//! Total cumulative amount of unspendable coins up to and including this block
CAmount total_unspendable_amount{0};
//! Total cumulative amount of prevouts spent up to and including this block
CAmount total_prevout_spent_amount{0};
//! Total cumulative amount of outputs created up to and including this block
CAmount total_new_outputs_ex_coinbase_amount{0};
//! Total cumulative amount of coinbase outputs up to and including this block
CAmount total_coinbase_amount{0};
//! The unspendable coinbase amount from the genesis block
CAmount total_unspendables_genesis_block{0};
//! The two unspendable coinbase outputs total amount caused by BIP30
CAmount total_unspendables_bip30{0};
//! Total cumulative amount of outputs sent to unspendable scripts (OP_RETURN for example) up to and including this block
CAmount total_unspendables_scripts{0};
//! Total cumulative amount of coins lost due to unclaimed miner rewards up to and including this block
CAmount total_unspendables_unclaimed_rewards{0};

CCoinsStats(CoinStatsHashType hash_type) : m_hash_type(hash_type) {}
};
Expand Down
Loading