Skip to content

Commit 31be1a4

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#29236: log: Nuke error(...)
fa39151 refactor: Remove unused error() (MarcoFalke) fad0335 scripted-diff: Replace error() with LogError() (MarcoFalke) fa808fb refactor: Make error() return type void (MarcoFalke) fa1d624 scripted-diff: return error(...); ==> error(...); return false; (MarcoFalke) fa9a5e8 refactor: Add missing {} around error() calls (MarcoFalke) Pull request description: `error(...)` has many issues: * It is often used in the context of `return error(...)`, implying that it has a "fancy" type, creating confusion with `util::Result/Error` * `-logsourcelocations` does not work with it, because it will pretend the error happened inside of `logging.h` * The log line contains `ERROR: `, as opposed to `[error]`, like for other errors logged with `LogError`. Fix all issues by removing it. ACKs for top commit: fjahr: re-utACK fa39151 stickies-v: re-ACK fa39151, no changes since 4a90374 ryanofsky: Code review ACK fa39151. Just rebase since last review Tree-SHA512: ec5bb502ab0d3733fdb14a8a00762638fce0417afd8dd6294ae0d485ce2b7ca5b1efeb50fc2cd7467f6c652e4ed3e99b0f283b08aeca04bbfb7ea4f2c95d283a
2 parents e705909 + fa39151 commit 31be1a4

File tree

14 files changed

+205
-110
lines changed

14 files changed

+205
-110
lines changed

‎src/addrdb.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ bool SerializeDB(Stream& stream, const Data& data)
4444
hashwriter << Params().MessageStart() << data;
4545
stream << hashwriter.GetHash();
4646
} catch (const std::exception& e) {
47-
return error("%s: Serialize or I/O error - %s", __func__, e.what());
47+
LogError("%s: Serialize or I/O error - %s\n", __func__, e.what());
48+
return false;
4849
}
4950

5051
return true;
@@ -64,7 +65,8 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
6465
if (fileout.IsNull()) {
6566
fileout.fclose();
6667
remove(pathTmp);
67-
return error("%s: Failed to open file %s", __func__, fs::PathToString(pathTmp));
68+
LogError("%s: Failed to open file %s\n", __func__, fs::PathToString(pathTmp));
69+
return false;
6870
}
6971

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

8386
// replace existing file, if any, with new file
8487
if (!RenameOver(pathTmp, path)) {
8588
remove(pathTmp);
86-
return error("%s: Rename-into-place failed", __func__);
89+
LogError("%s: Rename-into-place failed\n", __func__);
90+
return false;
8791
}
8892

8993
return true;
@@ -140,7 +144,7 @@ bool CBanDB::Write(const banmap_t& banSet)
140144
}
141145

142146
for (const auto& err : errors) {
143-
error("%s", err);
147+
LogError("%s\n", err);
144148
}
145149
return false;
146150
}

‎src/flatfile.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,15 +82,18 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize)
8282
{
8383
FILE* file = Open(FlatFilePos(pos.nFile, 0)); // Avoid fseek to nPos
8484
if (!file) {
85-
return error("%s: failed to open file %d", __func__, pos.nFile);
85+
LogError("%s: failed to open file %d\n", __func__, pos.nFile);
86+
return false;
8687
}
8788
if (finalize && !TruncateFile(file, pos.nPos)) {
8889
fclose(file);
89-
return error("%s: failed to truncate file %d", __func__, pos.nFile);
90+
LogError("%s: failed to truncate file %d\n", __func__, pos.nFile);
91+
return false;
9092
}
9193
if (!FileCommit(file)) {
9294
fclose(file);
93-
return error("%s: failed to commit file %d", __func__, pos.nFile);
95+
LogError("%s: failed to commit file %d\n", __func__, pos.nFile);
96+
return false;
9497
}
9598
DirectoryCommit(m_dir);
9699

‎src/index/base.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,8 @@ bool BaseIndex::Commit()
229229
}
230230
}
231231
if (!ok) {
232-
return error("%s: Failed to commit latest %s state", __func__, GetName());
232+
LogError("%s: Failed to commit latest %s state\n", __func__, GetName());
233+
return false;
233234
}
234235
return true;
235236
}

‎src/index/blockfilterindex.cpp

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,9 @@ bool BlockFilterIndex::CustomInit(const std::optional<interfaces::BlockKey>& blo
119119
// indicate database corruption or a disk failure, and starting the index would cause
120120
// further corruption.
121121
if (m_db->Exists(DB_FILTER_POS)) {
122-
return error("%s: Cannot read current %s state; index may be corrupted",
122+
LogError("%s: Cannot read current %s state; index may be corrupted\n",
123123
__func__, GetName());
124+
return false;
124125
}
125126

126127
// If the DB_FILTER_POS is not set, then initialize to the first location.
@@ -137,10 +138,12 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
137138
// Flush current filter file to disk.
138139
AutoFile file{m_filter_fileseq->Open(pos)};
139140
if (file.IsNull()) {
140-
return error("%s: Failed to open filter file %d", __func__, pos.nFile);
141+
LogError("%s: Failed to open filter file %d\n", __func__, pos.nFile);
142+
return false;
141143
}
142144
if (!FileCommit(file.Get())) {
143-
return error("%s: Failed to commit filter file %d", __func__, pos.nFile);
145+
LogError("%s: Failed to commit filter file %d\n", __func__, pos.nFile);
146+
return false;
144147
}
145148

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

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

236243
uint256 expected_block_hash = *Assert(block.prev_hash);
237244
if (read_out.first != expected_block_hash) {
238-
return error("%s: previous block header belongs to unexpected block %s; expected %s",
245+
LogError("%s: previous block header belongs to unexpected block %s; expected %s\n",
239246
__func__, read_out.first.ToString(), expected_block_hash.ToString());
247+
return false;
240248
}
241249

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

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

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

283293
batch.Write(DBHashKey(value.first), std::move(value.second));
@@ -330,11 +340,13 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start
330340
const CBlockIndex* stop_index, std::vector<DBVal>& results)
331341
{
332342
if (start_height < 0) {
333-
return error("%s: start height (%d) is negative", __func__, start_height);
343+
LogError("%s: start height (%d) is negative\n", __func__, start_height);
344+
return false;
334345
}
335346
if (start_height > stop_index->nHeight) {
336-
return error("%s: start height (%d) is greater than stop height (%d)",
347+
LogError("%s: start height (%d) is greater than stop height (%d)\n",
337348
__func__, start_height, stop_index->nHeight);
349+
return false;
338350
}
339351

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

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

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

375388
if (!db.Read(DBHashKey(block_hash), results[i])) {
376-
return error("%s: unable to read value in %s at key (%c, %s)",
389+
LogError("%s: unable to read value in %s at key (%c, %s)\n",
377390
__func__, index_name, DB_BLOCK_HASH, block_hash.ToString());
391+
return false;
378392
}
379393
}
380394

‎src/index/coinstatsindex.cpp

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,9 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
138138
read_out.first.ToString(), expected_block_hash.ToString());
139139

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

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

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

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

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

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

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

361366
if (block) {
362367
DBVal entry;
363368
if (!LookUpOne(*m_db, *block, entry)) {
364-
return error("%s: Cannot read current %s state; index may be corrupted",
369+
LogError("%s: Cannot read current %s state; index may be corrupted\n",
365370
__func__, GetName());
371+
return false;
366372
}
367373

368374
uint256 out;
369375
m_muhash.Finalize(out);
370376
if (entry.muhash != out) {
371-
return error("%s: Cannot read current %s state; index may be corrupted",
377+
LogError("%s: Cannot read current %s state; index may be corrupted\n",
372378
__func__, GetName());
379+
return false;
373380
}
374381

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

424431
if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
425-
return error("%s: previous block header not found; expected %s",
432+
LogError("%s: previous block header not found; expected %s\n",
426433
__func__, expected_block_hash.ToString());
434+
return false;
427435
}
428436
}
429437
}

‎src/index/txindex.cpp

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,20 +81,24 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe
8181

8282
AutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true)};
8383
if (file.IsNull()) {
84-
return error("%s: OpenBlockFile failed", __func__);
84+
LogError("%s: OpenBlockFile failed\n", __func__);
85+
return false;
8586
}
8687
CBlockHeader header;
8788
try {
8889
file >> header;
8990
if (fseek(file.Get(), postx.nTxOffset, SEEK_CUR)) {
90-
return error("%s: fseek(...) failed", __func__);
91+
LogError("%s: fseek(...) failed\n", __func__);
92+
return false;
9193
}
9294
file >> TX_WITH_WITNESS(tx);
9395
} catch (const std::exception& e) {
94-
return error("%s: Deserialize or I/O error - %s", __func__, e.what());
96+
LogError("%s: Deserialize or I/O error - %s\n", __func__, e.what());
97+
return false;
9598
}
9699
if (tx->GetHash() != tx_hash) {
97-
return error("%s: txid mismatch", __func__);
100+
LogError("%s: txid mismatch\n", __func__);
101+
return false;
98102
}
99103
block_hash = header.GetHash();
100104
return true;

‎src/kernel/coinstats.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,8 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
134134
outputs[key.n] = std::move(coin);
135135
stats.coins_count++;
136136
} else {
137-
return error("%s: unable to read value", __func__);
137+
LogError("%s: unable to read value\n", __func__);
138+
return false;
138139
}
139140
pcursor->Next();
140141
}

‎src/logging.h

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -263,11 +263,4 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
263263
// Deprecated conditional logging
264264
#define LogPrint(category, ...) LogDebug(category, __VA_ARGS__)
265265

266-
template <typename... Args>
267-
bool error(const char* fmt, const Args&... args)
268-
{
269-
LogPrintf("ERROR: %s\n", tfm::format(fmt, args...));
270-
return false;
271-
}
272-
273266
#endif // BITCOIN_LOGGING_H

‎src/net.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -570,7 +570,7 @@ void CNode::SetAddrLocal(const CService& addrLocalIn) {
570570
AssertLockNotHeld(m_addr_local_mutex);
571571
LOCK(m_addr_local_mutex);
572572
if (addrLocal.IsValid()) {
573-
error("Addr local already set for node: %i. Refusing to change from %s to %s", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort());
573+
LogError("Addr local already set for node: %i. Refusing to change from %s to %s\n", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort());
574574
} else {
575575
addrLocal = addrLocalIn;
576576
}

0 commit comments

Comments
 (0)