Repository: bitcoin/bitcoin (language: cpp)

Issue:
ci: failure in `rpc_scanblocks.py`
https://github.com/bitcoin/bitcoin/actions/runs/8600455763/job/23565458580?pr=29707#step:7:3826:
```bash
  node0 2024-04-08T13:15:12.710688Z [httpworker.3] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=getindexinfo user=__cookie__ 
 test  2024-04-08T13:15:12.711000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_framework.py", line 132, in main
                                       self.run_test()
                                     File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/rpc_scanblocks.py", line 49, in run_test
                                       assert blockhash in out['relevant_blocks']
                                   AssertionError
 node0 2024-04-08T13:15:12.711330Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:51647 
```


Discussion / hints:
Is this a race where a validation event is lost? 

```
 node0 2024-04-08T13:15:11.629578Z [httpworker.0] [validationinterface.cpp:212] [BlockConnected] [validation] Enqueuing BlockConnected: block hash=2194b76c425a2423728356cb8b9c82ad89068bcddfe8dc287c93732fb3354b59 block height=201 
...
 node0 2024-04-08T13:15:11.629801Z [scheduler] [validationinterface.cpp:212] [operator()] [validation] BlockConnected: block hash=2194b76c425a2423728356cb8b9c82ad89068bcddfe8dc287c93732fb3354b59 block height=201 
...
 node0 2024-04-08T13:15:11.639724Z [basic block filter index] [index/base.cpp:209] [Sync] basic block filter index is enabled at height 200 
...
 node0 2024-04-08T13:15:11.641186Z [basic block filter index] [util/thread.cpp:22] [TraceThread] basic block filter index thread exit 
...
 node0 2024-04-08T13:15:12.710688Z [httpworker.3] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=getindexinfo user=__cookie__ 
```


Not sure if this is a bug in the index logic, or if the test needs to first wait for the sync.

Moving `self.wait_until(lambda: all(i["synced"] for i in node.getindexinfo().values()))` before the generate should hide this issue in the test.

cc @furszy 
Quick check, the index seems to not have processed the block by the time `scanblocks()` (line 48) is called. The `getindexinfo()['synced']` value is set to true after the index initial sync. It does not tell us anything about tip scanning.
So, we can change line 46 check to verify the latest block height instead of the `synced` value. And that should fix the issue:
```diff
diff --git a/test/functional/rpc_scanblocks.py b/test/functional/rpc_scanblocks.py
--- a/test/functional/rpc_scanblocks.py	(revision 496f38dab4ba4506e4b628cb08ce4e582d638d07)
+++ b/test/functional/rpc_scanblocks.py	(date 1712655455099)
@@ -43,7 +43,7 @@
         # mine a block and assure that the mined blockhash is in the filterresult
         blockhash = self.generate(node, 1)[0]
         height = node.getblockheader(blockhash)['height']
-        self.wait_until(lambda: all(i["synced"] for i in node.getindexinfo().values()))
+        self.wait_until(lambda: all(i["best_block_height"] == height for i in node.getindexinfo().values()))
 
         out = node.scanblocks("start", [f"addr({addr_1})"])
         assert blockhash in out['relevant_blocks']
```
@furszy Are you sure? The event at block height = 201 does not exist anymore, so I don't think it will be processed at all.

To summarize my excerpts from above:

* Block 201 is mined, the event is put on the scheduler, and the event is consumed
* The background index build is completing for height 200, and the index will start to listen to events?
* The block 201 event is already consumed, so wont make it to the index
* What would happen if a block 202 event was fired at some point?

Basically, my question is if this is a bug in the index code, or in the test code.
> @furszy Are you sure? The event at block height = 201 does not exist anymore, so I don't think it will be processed at all.

Hmm ok, what I said could be another issue.. nvm. The index will probably stop syncing if this happens.. throwing a "WARNING: Block %s does not connect to an ancestor of..". Will do some tests.
Who performs ci? 
> Will do some tests.

Update: working on a fix. Will push it next week.
https://cirrus-ci.com/task/4562186641604608?logs=ci#L4268

Fix patch:
diff --git a/src/index/base.cpp b/src/index/base.cpp
index a203ce4a9f381..e66c89f9e4565 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -160,12 +160,24 @@ void BaseIndex::Sync()
             }
 
             const CBlockIndex* pindex_next = WITH_LOCK(cs_main, return NextSyncBlock(pindex, m_chainstate->m_chain));
+            // If pindex_next is null, it means pindex is the chain tip, so
+            // commit data indexed so far.
             if (!pindex_next) {
                 SetBestBlockIndex(pindex);
                 // No need to handle errors in Commit. See rationale above.
                 Commit();
-                m_synced = true;
-                break;
+
+                // If pindex is still the chain tip after committing, exit the
+                // sync loop. It is important for cs_main to be locked while
+                // setting m_synced = true, otherwise a new block could be
+                // attached while m_synced is still false, and it would not be
+                // indexed.
+                LOCK(::cs_main);
+                pindex_next = NextSyncBlock(pindex, m_chainstate->m_chain);
+                if (!pindex_next) {
+                    m_synced = true;
+                    break;
+                }
             }
             if (pindex_next->pprev != pindex && !Rewind(pindex, pindex_next->pprev)) {
                 FatalErrorf("%s: Failed to rewind index %s to a previous chain tip", __func__, GetName());


Test patch: