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

log: Nuke error(...) #29236

Merged
merged 5 commits into from
Mar 12, 2024
Merged

log: Nuke error(...) #29236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa9a5e8
refactor: Add missing {} around error() calls
Jan 11, 2024
fa1d624
scripted-diff: return error(...); ==> error(...); return false;
Jan 11, 2024
fa808fb
refactor: Make error() return type void
Jan 11, 2024
fad0335
scripted-diff: Replace error() with LogError()
Jan 11, 2024
fa39151
refactor: Remove unused error()
Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/addrdb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ bool SerializeDB(Stream& stream, const Data& data)
hashwriter << Params().MessageStart() << data;
stream << hashwriter.GetHash();
} catch (const std::exception& e) {
return error("%s: Serialize or I/O error - %s", __func__, e.what());
LogError("%s: Serialize or I/O error - %s\n", __func__, e.what());
return false;
}

return true;
Expand All @@ -64,7 +65,8 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
if (fileout.IsNull()) {
fileout.fclose();
remove(pathTmp);
return error("%s: Failed to open file %s", __func__, fs::PathToString(pathTmp));
LogError("%s: Failed to open file %s\n", __func__, fs::PathToString(pathTmp));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it seems the return value from SerializeFileDB is never used to actually shut down the node. In the case of failing to open file, I think LogError is appropriate because that probably should lead to a shutdown. In the case of "Failed to flush file", I think a LogWarning could be more appropriate.

I think it's best to leave the PR as is to make progress, bikeshedding over warning vs error categories doesn't have a huge amount of benefit here. Just leaving this comment for visibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but "error" is still applicable within the addrdb functions, as they can not continue and must return. So I think, ideally they'd return Result<void>, and the caller could downgrade the error to a warning and log it? Happy to review such a change, if someone creates it.

return false;
}

// Serialize
Expand All @@ -76,14 +78,16 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
if (!FileCommit(fileout.Get())) {
fileout.fclose();
remove(pathTmp);
return error("%s: Failed to flush file %s", __func__, fs::PathToString(pathTmp));
LogError("%s: Failed to flush file %s\n", __func__, fs::PathToString(pathTmp));
return false;
}
fileout.fclose();

// replace existing file, if any, with new file
if (!RenameOver(pathTmp, path)) {
remove(pathTmp);
return error("%s: Rename-into-place failed", __func__);
LogError("%s: Rename-into-place failed\n", __func__);
return false;
}

return true;
Expand Down Expand Up @@ -140,7 +144,7 @@ bool CBanDB::Write(const banmap_t& banSet)
}

for (const auto& err : errors) {
error("%s", err);
LogError("%s\n", err);
}
return false;
}
Expand Down
9 changes: 6 additions & 3 deletions src/flatfile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,18 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize)
{
FILE* file = Open(FlatFilePos(pos.nFile, 0)); // Avoid fseek to nPos
if (!file) {
return error("%s: failed to open file %d", __func__, pos.nFile);
LogError("%s: failed to open file %d\n", __func__, pos.nFile);
return false;
}
if (finalize && !TruncateFile(file, pos.nPos)) {
fclose(file);
return error("%s: failed to truncate file %d", __func__, pos.nFile);
LogError("%s: failed to truncate file %d\n", __func__, pos.nFile);
return false;
}
if (!FileCommit(file)) {
fclose(file);
return error("%s: failed to commit file %d", __func__, pos.nFile);
LogError("%s: failed to commit file %d\n", __func__, pos.nFile);
return false;
}
DirectoryCommit(m_dir);

Expand Down
3 changes: 2 additions & 1 deletion src/index/base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,8 @@ bool BaseIndex::Commit()
}
}
if (!ok) {
return error("%s: Failed to commit latest %s state", __func__, GetName());
LogError("%s: Failed to commit latest %s state\n", __func__, GetName());
return false;
}
return true;
}
Expand Down
38 changes: 26 additions & 12 deletions src/index/blockfilterindex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,9 @@ bool BlockFilterIndex::CustomInit(const std::optional<interfaces::BlockKey>& blo
// indicate database corruption or a disk failure, and starting the index would cause
// further corruption.
if (m_db->Exists(DB_FILTER_POS)) {
return error("%s: Cannot read current %s state; index may be corrupted",
LogError("%s: Cannot read current %s state; index may be corrupted\n",
__func__, GetName());
return false;
}

// If the DB_FILTER_POS is not set, then initialize to the first location.
Expand All @@ -137,10 +138,12 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
// Flush current filter file to disk.
AutoFile file{m_filter_fileseq->Open(pos)};
if (file.IsNull()) {
return error("%s: Failed to open filter file %d", __func__, pos.nFile);
LogError("%s: Failed to open filter file %d\n", __func__, pos.nFile);
return false;
}
if (!FileCommit(file.Get())) {
return error("%s: Failed to commit filter file %d", __func__, pos.nFile);
LogError("%s: Failed to commit filter file %d\n", __func__, pos.nFile);
return false;
}

batch.Write(DB_FILTER_POS, pos);
Expand All @@ -159,11 +162,15 @@ bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256&
std::vector<uint8_t> encoded_filter;
try {
filein >> block_hash >> encoded_filter;
if (Hash(encoded_filter) != hash) return error("Checksum mismatch in filter decode.");
if (Hash(encoded_filter) != hash) {
LogError("Checksum mismatch in filter decode.\n");
return false;
}
filter = BlockFilter(GetFilterType(), block_hash, std::move(encoded_filter), /*skip_decode_check=*/true);
}
catch (const std::exception& e) {
return error("%s: Failed to deserialize block filter from disk: %s", __func__, e.what());
LogError("%s: Failed to deserialize block filter from disk: %s\n", __func__, e.what());
return false;
}

return true;
Expand Down Expand Up @@ -235,8 +242,9 @@ bool BlockFilterIndex::CustomAppend(const interfaces::BlockInfo& block)

uint256 expected_block_hash = *Assert(block.prev_hash);
if (read_out.first != expected_block_hash) {
return error("%s: previous block header belongs to unexpected block %s; expected %s",
LogError("%s: previous block header belongs to unexpected block %s; expected %s\n",
__func__, read_out.first.ToString(), expected_block_hash.ToString());
return false;
}

prev_header = read_out.second.header;
Expand Down Expand Up @@ -270,14 +278,16 @@ bool BlockFilterIndex::CustomAppend(const interfaces::BlockInfo& block)

for (int height = start_height; height <= stop_height; ++height) {
if (!db_it.GetKey(key) || key.height != height) {
return error("%s: unexpected key in %s: expected (%c, %d)",
LogError("%s: unexpected key in %s: expected (%c, %d)\n",
__func__, index_name, DB_BLOCK_HEIGHT, height);
return false;
}

std::pair<uint256, DBVal> value;
if (!db_it.GetValue(value)) {
return error("%s: unable to read value in %s at key (%c, %d)",
LogError("%s: unable to read value in %s at key (%c, %d)\n",
__func__, index_name, DB_BLOCK_HEIGHT, height);
return false;
}

batch.Write(DBHashKey(value.first), std::move(value.second));
Expand Down Expand Up @@ -330,11 +340,13 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start
const CBlockIndex* stop_index, std::vector<DBVal>& results)
{
if (start_height < 0) {
return error("%s: start height (%d) is negative", __func__, start_height);
LogError("%s: start height (%d) is negative\n", __func__, start_height);
return false;
}
if (start_height > stop_index->nHeight) {
return error("%s: start height (%d) is greater than stop height (%d)",
LogError("%s: start height (%d) is greater than stop height (%d)\n",
__func__, start_height, stop_index->nHeight);
return false;
}

size_t results_size = static_cast<size_t>(stop_index->nHeight - start_height + 1);
Expand All @@ -350,8 +362,9 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start

size_t i = static_cast<size_t>(height - start_height);
if (!db_it->GetValue(values[i])) {
return error("%s: unable to read value in %s at key (%c, %d)",
LogError("%s: unable to read value in %s at key (%c, %d)\n",
__func__, index_name, DB_BLOCK_HEIGHT, height);
return false;
}

db_it->Next();
Expand All @@ -373,8 +386,9 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start
}

if (!db.Read(DBHashKey(block_hash), results[i])) {
return error("%s: unable to read value in %s at key (%c, %s)",
LogError("%s: unable to read value in %s at key (%c, %s)\n",
__func__, index_name, DB_BLOCK_HASH, block_hash.ToString());
return false;
}
}

Expand Down
24 changes: 16 additions & 8 deletions src/index/coinstatsindex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,9 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
read_out.first.ToString(), expected_block_hash.ToString());

if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
return error("%s: previous block header not found; expected %s",
LogError("%s: previous block header not found; expected %s\n",
__func__, expected_block_hash.ToString());
return false;
}
}

Expand Down Expand Up @@ -245,14 +246,16 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)

for (int height = start_height; height <= stop_height; ++height) {
if (!db_it.GetKey(key) || key.height != height) {
return error("%s: unexpected key in %s: expected (%c, %d)",
LogError("%s: unexpected key in %s: expected (%c, %d)\n",
__func__, index_name, DB_BLOCK_HEIGHT, height);
return false;
}

std::pair<uint256, DBVal> value;
if (!db_it.GetValue(value)) {
return error("%s: unable to read value in %s at key (%c, %d)",
LogError("%s: unable to read value in %s at key (%c, %d)\n",
__func__, index_name, DB_BLOCK_HEIGHT, height);
return false;
}

batch.Write(DBHashKey(value.first), std::move(value.second));
Expand Down Expand Up @@ -285,8 +288,9 @@ bool CoinStatsIndex::CustomRewind(const interfaces::BlockKey& current_tip, const
CBlock block;

if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *iter_tip)) {
return error("%s: Failed to read block %s from disk",
LogError("%s: Failed to read block %s from disk\n",
__func__, iter_tip->GetBlockHash().ToString());
return false;
}

if (!ReverseBlock(block, iter_tip)) {
Expand Down Expand Up @@ -353,23 +357,26 @@ bool CoinStatsIndex::CustomInit(const std::optional<interfaces::BlockKey>& block
// exist. Any other errors indicate database corruption or a disk
// failure, and starting the index would cause further corruption.
if (m_db->Exists(DB_MUHASH)) {
return error("%s: Cannot read current %s state; index may be corrupted",
LogError("%s: Cannot read current %s state; index may be corrupted\n",
__func__, GetName());
return false;
}
}

if (block) {
DBVal entry;
if (!LookUpOne(*m_db, *block, entry)) {
return error("%s: Cannot read current %s state; index may be corrupted",
LogError("%s: Cannot read current %s state; index may be corrupted\n",
__func__, GetName());
return false;
}

uint256 out;
m_muhash.Finalize(out);
if (entry.muhash != out) {
return error("%s: Cannot read current %s state; index may be corrupted",
LogError("%s: Cannot read current %s state; index may be corrupted\n",
__func__, GetName());
return false;
}

m_transaction_output_count = entry.transaction_output_count;
Expand Down Expand Up @@ -422,8 +429,9 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
read_out.first.ToString(), expected_block_hash.ToString());

if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
return error("%s: previous block header not found; expected %s",
LogError("%s: previous block header not found; expected %s\n",
__func__, expected_block_hash.ToString());
return false;
}
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/index/txindex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,20 +81,24 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe

AutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true)};
if (file.IsNull()) {
return error("%s: OpenBlockFile failed", __func__);
LogError("%s: OpenBlockFile failed\n", __func__);
return false;
}
CBlockHeader header;
try {
file >> header;
if (fseek(file.Get(), postx.nTxOffset, SEEK_CUR)) {
return error("%s: fseek(...) failed", __func__);
LogError("%s: fseek(...) failed\n", __func__);
return false;
}
file >> TX_WITH_WITNESS(tx);
} catch (const std::exception& e) {
return error("%s: Deserialize or I/O error - %s", __func__, e.what());
LogError("%s: Deserialize or I/O error - %s\n", __func__, e.what());
return false;
}
if (tx->GetHash() != tx_hash) {
return error("%s: txid mismatch", __func__);
LogError("%s: txid mismatch\n", __func__);
return false;
}
block_hash = header.GetHash();
return true;
Expand Down
3 changes: 2 additions & 1 deletion src/kernel/coinstats.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
outputs[key.n] = std::move(coin);
stats.coins_count++;
} else {
return error("%s: unable to read value", __func__);
LogError("%s: unable to read value\n", __func__);
return false;
}
pcursor->Next();
}
Expand Down
7 changes: 0 additions & 7 deletions src/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,4 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
// Deprecated conditional logging
#define LogPrint(category, ...) LogDebug(category, __VA_ARGS__)

template <typename... Args>
bool error(const char* fmt, const Args&... args)
{
LogPrintf("ERROR: %s\n", tfm::format(fmt, args...));
return false;
}

#endif // BITCOIN_LOGGING_H
2 changes: 1 addition & 1 deletion src/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ void CNode::SetAddrLocal(const CService& addrLocalIn) {
AssertLockNotHeld(m_addr_local_mutex);
LOCK(m_addr_local_mutex);
if (addrLocal.IsValid()) {
error("Addr local already set for node: %i. Refusing to change from %s to %s", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort());
LogError("Addr local already set for node: %i. Refusing to change from %s to %s\n", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort());
} else {
addrLocal = addrLocalIn;
}
Expand Down