Skip to content

Commit 65c05db

Browse files
glozowglozow
committed
Merge bitcoin/bitcoin#29013: test: doc: follow-up #28368
b1318dc test: change `m_submitted_in_package` input to fuzz data provider boolean (ismaelsadeeq) 5615e16 tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` (ismaelsadeeq) fcd4296 doc: fix typo and update incorrect comment (ismaelsadeeq) 562664d test: wait for fee estimator to catch up before estimating fees (ismaelsadeeq) Pull request description: This is a simple PR that does two things 1. Fixes #29000 by waiting for the fee estimator to catch up after `removeForBlock` calls before calling `estimateFee` in the `BlockPolicyEstimates` unit test. 2. Addressed some outstanding review comments from #28368 - Updated `NewMempoolTransactionInfo::m_from_disconnected_block` to `NewMempoolTransactionInfo::m_mempool_limit_bypassed` which now correctly indicates what the boolean does. - Changed input of `processTransaction`'s tx_info `m_submitted_in_package` input from false to fuzz data provider boolean. - Fixed some typos, and update incorrect comment ACKs for top commit: martinus: re-ACK b1318dc glozow: utACK b1318dc Tree-SHA512: 45268729bc044da4748fe004524e0df696d2ec92c5bd053db9aad6e15675f3838429b2a7b9061a6b694be4dc319d1782a876b44df506ddd439d62ad07252d0e1
2 parents c3038bf + b1318dc commit 65c05db

File tree

6 files changed

+27
-22
lines changed

6 files changed

+27
-22
lines changed

‎src/init.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ void Shutdown(NodeContext& node)
313313
DumpMempool(*node.mempool, MempoolPath(*node.args));
314314
}
315315

316-
// Drop transactions we were still watching, record fee estimations and Unregister
316+
// Drop transactions we were still watching, record fee estimations and unregister
317317
// fee estimator from validation interface.
318318
if (node.fee_estimator) {
319319
node.fee_estimator->Flush();

‎src/kernel/mempool_entry.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ struct NewMempoolTransactionInfo {
226226
* This boolean indicates whether the transaction was added
227227
* without enforcing mempool fee limits.
228228
*/
229-
const bool m_from_disconnected_block;
229+
const bool m_mempool_limit_bypassed;
230230
/* This boolean indicates whether the transaction is part of a package. */
231231
const bool m_submitted_in_package;
232232
/*
@@ -239,11 +239,11 @@ struct NewMempoolTransactionInfo {
239239

240240
explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee,
241241
const int64_t vsize, const unsigned int height,
242-
const bool from_disconnected_block, const bool submitted_in_package,
242+
const bool mempool_limit_bypassed, const bool submitted_in_package,
243243
const bool chainstate_is_current,
244244
const bool has_no_mempool_parents)
245245
: info{tx, fee, vsize, height},
246-
m_from_disconnected_block{from_disconnected_block},
246+
m_mempool_limit_bypassed{mempool_limit_bypassed},
247247
m_submitted_in_package{submitted_in_package},
248248
m_chainstate_is_current{chainstate_is_current},
249249
m_has_no_mempool_parents{has_no_mempool_parents} {}

‎src/policy/fees.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -612,7 +612,7 @@ void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo&
612612
// - the node is not behind
613613
// - the transaction is not dependent on any other transactions in the mempool
614614
// - it's not part of a package.
615-
const bool validForFeeEstimation = !tx.m_from_disconnected_block && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents;
615+
const bool validForFeeEstimation = !tx.m_mempool_limit_bypassed && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents;
616616

617617
// Only want to be updating estimates when our blockchain is synced,
618618
// otherwise we'll miscalculate how many blocks its taking to get included.

‎src/test/fuzz/policy_estimator.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,14 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
4545
}
4646
const CTransaction tx{*mtx};
4747
const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx);
48+
const auto tx_submitted_in_package = fuzzed_data_provider.ConsumeBool();
49+
const auto tx_has_mempool_parents = fuzzed_data_provider.ConsumeBool();
4850
const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(),
4951
entry.GetTxSize(), entry.GetHeight(),
50-
/* m_from_disconnected_block */ false,
51-
/* m_submitted_in_package */ false,
52-
/* m_chainstate_is_current */ true,
53-
/* m_has_no_mempool_parents */ fuzzed_data_provider.ConsumeBool());
52+
/*mempool_limit_bypassed=*/false,
53+
tx_submitted_in_package,
54+
/*chainstate_is_current=*/true,
55+
tx_has_mempool_parents);
5456
block_policy_estimator.processTransaction(tx_info);
5557
if (fuzzed_data_provider.ConsumeBool()) {
5658
(void)block_policy_estimator.removeTx(tx.GetHash());

‎src/test/policyestimator_tests.cpp‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
7070
feeV[j],
7171
virtual_size,
7272
entry.nHeight,
73-
/* m_from_disconnected_block */ false,
74-
/* m_submitted_in_package */ false,
75-
/* m_chainstate_is_current */ true,
76-
/* m_has_no_mempool_parents */ true)};
73+
/*mempool_limit_bypassed=*/false,
74+
/*submitted_in_package=*/false,
75+
/*chainstate_is_current=*/true,
76+
/*has_no_mempool_parents=*/true)};
7777
GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
7878
}
7979
uint256 hash = tx.GetHash();
@@ -112,6 +112,9 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
112112
}
113113
}
114114

115+
// Wait for fee estimator to catch up
116+
SyncWithValidationInterfaceQueue();
117+
115118
std::vector<CAmount> origFeeEst;
116119
// Highest feerate is 10*baseRate and gets in all blocks,
117120
// second highest feerate is 9*baseRate and gets in 9/10 blocks = 90%,
@@ -168,10 +171,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
168171
feeV[j],
169172
virtual_size,
170173
entry.nHeight,
171-
/* m_from_disconnected_block */ false,
172-
/* m_submitted_in_package */ false,
173-
/* m_chainstate_is_current */ true,
174-
/* m_has_no_mempool_parents */ true)};
174+
/*mempool_limit_bypassed=*/false,
175+
/*submitted_in_package=*/false,
176+
/*chainstate_is_current=*/true,
177+
/*has_no_mempool_parents=*/true)};
175178
GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
176179
}
177180
uint256 hash = tx.GetHash();
@@ -232,10 +235,10 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
232235
feeV[j],
233236
virtual_size,
234237
entry.nHeight,
235-
/* m_from_disconnected_block */ false,
236-
/* m_submitted_in_package */ false,
237-
/* m_chainstate_is_current */ true,
238-
/* m_has_no_mempool_parents */ true)};
238+
/*mempool_limit_bypassed=*/false,
239+
/*submitted_in_package=*/false,
240+
/*chainstate_is_current=*/true,
241+
/*has_no_mempool_parents=*/true)};
239242
GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
240243
}
241244
uint256 hash = tx.GetHash();

‎src/validationinterface.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ class CValidationInterface {
150150
virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
151151
/**
152152
* Notifies listeners of a block being disconnected
153-
* Provides the block that was connected.
153+
* Provides the block that was disconnected.
154154
*
155155
* Called on a background thread. Only called for the active chainstate, since
156156
* background chainstates should never disconnect blocks.

0 commit comments

Comments
 (0)