index: remove unnecessary locator cleaning in BaseIndex::Init()

HowHsu commented at 4:13 pm on July 5, 2025: none

refactor: remove redundant locator cleanup in BaseIndex::Init()

Return optional value in ReadBestBlock() to eliminate manual SetNull() calls.

DrahtBot added the label UTXO Db and Indexes on Jul 5, 2025

DrahtBot commented at 4:13 pm on July 5, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32882.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in src/index/base.h:59 in 1f678d832b outdated

55@@ -56,7 +56,7 @@ class BaseIndex : public CValidationInterface
56            bool f_memory = false, bool f_wipe = false, bool f_obfuscate = false);
57 
58         /// Read block locator of the chain that the index is in sync with.
59-        bool ReadBestBlock(CBlockLocator& locator) const;
60+        void ReadBestBlock(CBlockLocator& locator) const;

luke-jr commented at 9:03 pm on July 7, 2025:

No harm in returning success, even if ignored currently.

HowHsu commented at 2:19 am on July 8, 2025:

I tend to favor the principle of adding things only when they’re actually needed.

l0rinc approved

l0rinc commented at 11:52 pm on August 4, 2025: contributor

ACK 1f678d832b1196ca7d7c2ff7e70d2172ae8c90b6

Recreated and retested it locally. It’s a similar change to #33042, we have a few more places which should be cleanup up like this.

I think this already makes the code slightly simpler, but alternatively, you could return an optional and not rely output params and nullness

 0diff --git a/src/index/base.cpp b/src/index/base.cpp
 1index fdd0e0d8af..0408021fd5 100644
 2--- a/src/index/base.cpp
 3+++ b/src/index/base.cpp
 4@@ -59,13 +59,10 @@ BaseIndex::DB::DB(const fs::path& path, size_t n_cache_size, bool f_memory, bool
 5         .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()}}
 6 {}
 7 
 8-bool BaseIndex::DB::ReadBestBlock(CBlockLocator& locator) const
 9+std::optional<CBlockLocator> BaseIndex::DB::ReadBestBlock() const
10 {
11-    bool success = Read(DB_BEST_BLOCK, locator);
12-    if (!success) {
13-        locator.SetNull();
14-    }
15-    return success;
16+    if (CBlockLocator locator; Read(DB_BEST_BLOCK, locator)) return locator;
17+    return {};
18 }
19 
20 void BaseIndex::DB::WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator)
21@@ -97,20 +94,17 @@ bool BaseIndex::Init()
22     // callbacks are not missed once m_synced is true.
23     m_chain->context()->validation_signals->RegisterValidationInterface(this);
24 
25-    CBlockLocator locator;
26-    if (!GetDB().ReadBestBlock(locator)) {
27-        locator.SetNull();
28-    }
29+    const auto locator{GetDB().ReadBestBlock()};
30 
31     LOCK(cs_main);
32     CChain& index_chain = m_chainstate->m_chain;
33 
34-    if (locator.IsNull()) {
35+    if (!locator) {
36         SetBestBlockIndex(nullptr);
37     } else {
38         // Setting the best block to the locator's top block. If it is not part of the
39         // best chain, we will rewind to the fork point during index sync
40-        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))};
41+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator->vHave.at(0))};
42         if (!locator_index) {
43             return InitError(Untranslated(strprintf("best block of %s not found. Please rebuild the index.", GetName())));
44         }
45diff --git a/src/index/base.h b/src/index/base.h
46index 4131b06cad..c1971cae3f 100644
47--- a/src/index/base.h
48+++ b/src/index/base.h
49@@ -56,7 +56,7 @@ protected:
50            bool f_memory = false, bool f_wipe = false, bool f_obfuscate = false);
51 
52         /// Read block locator of the chain that the index is in sync with.
53-        bool ReadBestBlock(CBlockLocator& locator) const;
54+        std::optional<CBlockLocator> ReadBestBlock() const;
55 
56         /// Write block locator of the chain that the index is in sync with.
57         void WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator);

HowHsu force-pushed on Aug 5, 2025

HowHsu commented at 5:28 am on August 5, 2025: none

ACK 1f678d8

Recreated and retested it locally. It’s a similar change to #33042, we have a few more places which should be cleanup up like this.

I think this already makes the code slightly simpler, but alternatively, you could return an optional and not rely output params and nullness

Details

Thanks, l0rinc. I’ve updated the code as what you suggested

l0rinc commented at 5:32 am on August 5, 2025: contributor

The commit message and PR description needs to be updated now. Also consider adding co-authors when someone else also contributed.

HowHsu force-pushed on Aug 5, 2025

HowHsu commented at 5:54 am on August 5, 2025: none

The commit message and PR description needs to be updated now. Also consider adding co-authors when someone else also contributed.

True. Done.

l0rinc commented at 5:55 am on August 5, 2025: contributor

There’s still a typo in the title

l0rinc approved

l0rinc commented at 6:00 am on August 5, 2025: contributor

ACK ce7323ec635c389732e142c043f83868ddcefdb3

The commit message still sounds a bit unnatural and has a few typos, consider something like:

index: remove unnecessary locator cleaning in BaseIndex::Init()

Refactor BaseIndex::DB::ReadBestBlock() to return an optional value so that callers don’t need to rely on the locator itself to check if the call is successful or not. This simplifies the code.

or just simply:

refactor: remove redundant locator cleanup in BaseIndex::Init()

Return optional value in ReadBestBlock() to eliminate manual SetNull() calls.

HowHsu force-pushed on Aug 5, 2025

l0rinc commented at 5:34 pm on August 5, 2025: contributor

ACK 04044e554e6982977f9d82f9bb60659315a45f92

PR title still has the typo

in src/index/base.cpp:65 in 04044e554e outdated

66-    if (!success) {
67-        locator.SetNull();
68-    }
69-    return success;
70+    if (CBlockLocator locator; Read(DB_BEST_BLOCK, locator)) return locator;
71+    return {};

furszy commented at 5:52 pm on August 5, 2025:

Better to return std::nullopt.

l0rinc commented at 6:55 pm on August 5, 2025:

Isn’t that what we’re doing already? I tried making it explicit in:

0if (CBlockLocator locator; false) return locator;
1return {};

which successfully enters:

0if (!locator) {
1    SetBestBlockIndex(nullptr);

since locator.has_value() == false when running txindex_tests/txindex_initial_sync.

Or do you mean just that it’s more aesthetic that way?

furszy commented at 7:12 pm on August 5, 2025:

I think the fact that you had to run the txindex_tests/txindex_initial_sync test to assert the return value is kind of self-explanatory.

Using the standard std::nullopt is better in terms of style and clarity. It’s explicit and self-documenting. In contrast, return {}; is more ambiguous. One need to check the function’s signature. Is it returning an empty CBlockLocator? a null optional?

refactor: remove redundant locator cleanup in BaseIndex::Init()

Return optional value in ReadBestBlock() to eliminate manual SetNull() calls.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Signed-off-by: Hao Xu <hao.xu@linux.dev>

c83741e403

HowHsu force-pushed on Aug 6, 2025

l0rinc commented at 3:11 am on August 6, 2025: contributor

ACK c83741e4033824b4c798fc3e9cdc33cb30bbaf32

please fix the typo in the PR subject: locater -> locator

HowHsu renamed this:
~~index: remove unnecessary locater cleaning in BaseIndex::Init()~~
index: remove unnecessary locator cleaning in BaseIndex::Init()
on Aug 6, 2025

in src/index/base.cpp:102 in c83741e403

102 
103     LOCK(cs_main);
104     CChain& index_chain = m_chainstate->m_chain;
105 
106-    if (locator.IsNull()) {
107+    if (!locator) {

furszy commented at 10:27 pm on August 16, 2025:

Might be good to do if (!locator || locator->IsNull()) { just in case we ever stored an empty locator on disk (probably we never have, but.. we’ve had issues in this area before #24117, so better to be a bit conservative here).

That said, given this finding, I feel we should just use CBlockLocator directly, without the optional type. This would simplify the code and minimize the changes in the PR. You’d just need to make the function return a locator instead of a bool (because the locator is created within the Read function, you would never need to call SetNull on the locator), and extend the method’s doc to note that the returned locator will be empty if no record exists too.

l0rinc commented at 11:20 pm on August 16, 2025:

Since there’s already a null locator, you’re right, we don’t need an optional

index: remove unnecessary locator cleaning in BaseIndex::Init() #32882

Code Coverage & Benchmarks

Reviews