Repository: bitcoin/bitcoin (language: cpp)

Issue:
Test `policyestimator_tests/BlockPolicyEstimates` failure
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I have a reproducible test case failure on my machine. I've bisected it to commit 714523918ba2b853fc69bee6b04a33ba0c828bf5. Since this commit, the test case `policyestimator_tests/BlockPolicyEstimates` fails, but only when I use my builds script:

```sh
chrt -i 0 make -j32 check
```

`chrt` runs the make command in idle priority (I use that so I can smoothly browse while testing). I have an AMD 7950x with 16 cores/32 threads. When I remove `chrt -i 0` the test works. I suspect that it fails because for whatever reason it can't run in parallel with another test, but so far I couldn't narrow it down.

```
❯ chrt -i 0 make -j32 check |grep error
test/policyestimator_tests.cpp(129): error: in "policyestimator_tests/BlockPolicyEstimates": check origFeeEst[i-1] < mult*baseRate.GetFeePerK() + deltaFee has failed
test/policyestimator_tests.cpp(129): error: in "policyestimator_tests/BlockPolicyEstimates": check origFeeEst[i-1] < mult*baseRate.GetFeePerK() + deltaFee has failed
test/policyestimator_tests.cpp(129): error: in "policyestimator_tests/BlockPolicyEstimates": check origFeeEst[i-1] < mult*baseRate.GetFeePerK() + deltaFee has failed
test/policyestimator_tests.cpp(129): error: in "policyestimator_tests/BlockPolicyEstimates": check origFeeEst[i-1] < mult*baseRate.GetFeePerK() + deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
test/policyestimator_tests.cpp(151): error: in "policyestimator_tests/BlockPolicyEstimates": check feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee has failed
make[3]: *** [Makefile:22606: test/policyestimator_tests.cpp.test] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [Makefile:20548: check-am] Error 2
make[1]: *** [Makefile:20213: check-recursive] Error 1
make: *** [Makefile:813: check-recursive] Error 1
```

### Expected behaviour

All unit tests should succeed

### Steps to reproduce

```sh
./autogen.sh
./configure CXX=clang++ CC=clang --disable-wallet --with-guid=no
chrt -i 0 make -j32 check
```

### Relevant log output

I uploaded the full log of `chrt -i 0 make -j32 check >out.txt` here: https://gist.github.com/martinus/5d6795cd618f7d203e95d4f5426746f0

### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitcoin Core are you using?

160d23677ad

### Operating system and version

Linux 6.6.2-201.fc39.x86_64

### Machine specifications

AMD 7950x


Discussion / hints:
You can fix it by adding `SyncWithValidationInterfaceQueue` in line 113, no?
Right above the end of the loop? Yes, this seems to do the trick, I've run it 10 times and it always worked
Thats because the fee estimator now updates asynchronously, and there are about 200 `removeForBlock` calls in the loop
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/test/policyestimator_tests.cpp#L98

We suppose to wait for the fee estimator to catch up right after the loop below line 113 just as @maflcko said.

I will open a pull that will fix this and some outstanding comments from @willcl-ark in #28368

Fix patch:
diff --git a/src/init.cpp b/src/init.cpp
index ac52b34fc5c56..c07da0da42ea7 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -301,7 +301,7 @@ void Shutdown(NodeContext& node)
         DumpMempool(*node.mempool, MempoolPath(*node.args));
     }
 
-    // Drop transactions we were still watching, record fee estimations and Unregister
+    // Drop transactions we were still watching, record fee estimations and unregister
     // fee estimator from validation interface.
     if (node.fee_estimator) {
         node.fee_estimator->Flush();
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index bd39c9cc5f69f..2adeaea652df8 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -226,7 +226,7 @@ struct NewMempoolTransactionInfo {
      * This boolean indicates whether the transaction was added
      * without enforcing mempool fee limits.
      */
-    const bool m_from_disconnected_block;
+    const bool m_mempool_limit_bypassed;
     /* This boolean indicates whether the transaction is part of a package. */
     const bool m_submitted_in_package;
     /*
@@ -239,11 +239,11 @@ struct NewMempoolTransactionInfo {
 
     explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee,
                                        const int64_t vsize, const unsigned int height,
-                                       const bool from_disconnected_block, const bool submitted_in_package,
+                                       const bool mempool_limit_bypassed, const bool submitted_in_package,
                                        const bool chainstate_is_current,
                                        const bool has_no_mempool_parents)
         : info{tx, fee, vsize, height},
-          m_from_disconnected_block{from_disconnected_block},
+          m_mempool_limit_bypassed{mempool_limit_bypassed},
           m_submitted_in_package{submitted_in_package},
           m_chainstate_is_current{chainstate_is_current},
           m_has_no_mempool_parents{has_no_mempool_parents} {}
diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
index 74c688060d26d..544054863609c 100644
--- a/src/policy/fees.cpp
+++ b/src/policy/fees.cpp
@@ -612,7 +612,7 @@ void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo&
     // - the node is not behind
     // - the transaction is not dependent on any other transactions in the mempool
     // - it's not part of a package.
-    const bool validForFeeEstimation = !tx.m_from_disconnected_block && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents;
+    const bool validForFeeEstimation = !tx.m_mempool_limit_bypassed && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents;
 
     // Only want to be updating estimates when our blockchain is synced,
     // otherwise we'll miscalculate how many blocks its taking to get included.
diff --git a/src/validationinterface.h b/src/validationinterface.h
index e1d6869fab4ea..d9292ae2c9626 100644
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -150,7 +150,7 @@ class CValidationInterface {
     virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
     /**
      * Notifies listeners of a block being disconnected
-     *  Provides the block that was connected.
+     * Provides the block that was disconnected.
      *
      * Called on a background thread. Only called for the active chainstate, since
      * background chainstates should never disconnect blocks.


Test patch:
diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp
index 40a1fc80f07df..a4e1947b9f6e3 100644
--- a/src/test/fuzz/policy_estimator.cpp
+++ b/src/test/fuzz/policy_estimator.cpp
@@ -45,12 +45,14 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
                 }
                 const CTransaction tx{*mtx};
                 const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx);
+                const auto tx_submitted_in_package = fuzzed_data_provider.ConsumeBool();
+                const auto tx_has_mempool_parents = fuzzed_data_provider.ConsumeBool();
                 const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(),
                                                                entry.GetTxSize(), entry.GetHeight(),
-                                                               /* m_from_disconnected_block */ false,
-                                                               /* m_submitted_in_package */ false,
-                                                               /* m_chainstate_is_current */ true,
-                                                               /* m_has_no_mempool_parents */ fuzzed_data_provider.ConsumeBool());
+                                                               /*mempool_limit_bypassed=*/false,
+                                                               tx_submitted_in_package,
+                                                               /*chainstate_is_current=*/true,
+                                                               tx_has_mempool_parents);
                 block_policy_estimator.processTransaction(tx_info);
                 if (fuzzed_data_provider.ConsumeBool()) {
                     (void)block_policy_estimator.removeTx(tx.GetHash());
diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp
index 13ec89663ae11..ede73c6895761 100644
--- a/src/test/policyestimator_tests.cpp
+++ b/src/test/policyestimator_tests.cpp
@@ -70,10 +70,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
                                                                                       feeV[j],
                                                                                       virtual_size,
                                                                                       entry.nHeight,
-                                                                                      /* m_from_disconnected_block */ false,
-                                                                                      /* m_submitted_in_package */ false,
-                                                                                      /* m_chainstate_is_current */ true,
-                                                                                      /* m_has_no_mempool_parents */ true)};
+                                                                                      /*mempool_limit_bypassed=*/false,
+                                                                                      /*submitted_in_package=*/false,
+                                                                                      /*chainstate_is_current=*/true,
+                                                                                      /*has_no_mempool_parents=*/true)};
                     GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
                 }
                 uint256 hash = tx.GetHash();
@@ -112,6 +112,9 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
         }
     }
 
+    // Wait for fee estimator to catch up
+    SyncWithValidationInterfaceQueue();
+
     std::vector<CAmount> origFeeEst;
     // Highest feerate is 10*baseRate and gets in all blocks,
     // second highest feerate is 9*baseRate and gets in 9/10 blocks = 90%,
@@ -168,10 +171,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
                                                                                       feeV[j],
                                                                                       virtual_size,
                                                                                       entry.nHeight,
-                                                                                      /* m_from_disconnected_block */ false,
-                                                                                      /* m_submitted_in_package */ false,
-                                                                                      /* m_chainstate_is_current */ true,
-                                                                                      /* m_has_no_mempool_parents */ true)};
+                                                                                      /*mempool_limit_bypassed=*/false,
+                                                                                      /*submitted_in_package=*/false,
+                                                                                      /*chainstate_is_current=*/true,
+                                                                                      /*has_no_mempool_parents=*/true)};
                     GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
                 }
                 uint256 hash = tx.GetHash();
@@ -232,10 +235,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
                                                                                       feeV[j],
                                                                                       virtual_size,
                                                                                       entry.nHeight,
-                                                                                      /* m_from_disconnected_block */ false,
-                                                                                      /* m_submitted_in_package */ false,
-                                                                                      /* m_chainstate_is_current */ true,
-                                                                                      /* m_has_no_mempool_parents */ true)};
+                                                                                      /*mempool_limit_bypassed=*/false,
+                                                                                      /*submitted_in_package=*/false,
+                                                                                      /*chainstate_is_current=*/true,
+                                                                                      /*has_no_mempool_parents=*/true)};
                     GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
                 }
                 uint256 hash = tx.GetHash();