Age | Commit message (Collapse) | Author |
|
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.
Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.
Github-Pull: #27622
Rebased-From: cf219f29f3c5b41070eaab9a549a476f01990f3a
|
|
target to avoid OOM
fa5752da6a58fadd3f79f47ff98b796d9768872a fuzz: Limit outpoints.size in txorphan target to avoid OOM (MacroFake)
Pull request description:
Identical commit from https://github.com/bitcoin/bitcoin/pull/26216
ACKs for top commit:
fanquake:
ACK fa5752da6a58fadd3f79f47ff98b796d9768872a
Tree-SHA512: fa9bd1defb71b6bc2c62a6cdec5958576cafa0b41cdfebe11d48dd13505eb4a5e0e1d2ce55c0ed561d2a872dab6bb10bd71d941899b3b0eb0b2c38e6b3691ed5
|
|
|
|
fa4ba04c157b83b827f7541fa007710bd6211fe7 fuzz: Remove no-op call to get() (MacroFake)
fa642286b83f29cb0ac0c8d4c7d8eba10600402c fuzz: Avoid timeout in bitdeque fuzz target (MacroFake)
Pull request description:
Identical commit from https://github.com/bitcoin/bitcoin/pull/26012
Not strictly required for 24.x, but I guess it can't hurt to avoid timeouts.
Top commit has no ACKs.
Tree-SHA512: 4d4bfb645e3513bf22cc9c64bdcbde2ad9e28b5a07ab07a02fbfa19df02147b371d2ca794ab3a095c22b66781832055e0de3af908aaead4c26ea12189e05cbe3
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/CChainState/Chainstate/g' $(git grep -l CChainState ':(exclude)doc/release-notes*')
-END VERIFY SCRIPT-
Co-authored-by: MacroFake <falke.marco@gmail.com>
|
|
|
|
|
|
377e9ccda469731d535829f184b70c73ed46b6ef scripted-diff: net: rename permissionFlags to permission_flags (Anthony Towns)
0a7fc428978c4db416fdcf9bf0b79de17d0558d7 net: make CNode::m_prefer_evict const (Anthony Towns)
d394156b99d6b9a99aedee78658310d169ca188d net: make CNode::m_permissionFlags const (Anthony Towns)
9dccc3328eeaf9cd66518d812c878def5d014e36 net: add CNodeOptions for optional CNode constructor params (Anthony Towns)
Pull request description:
Adds CNodeOptions to make it easier to add optional parameters to the CNode constructor, and makes prefer_evict and m_permissionFlags actually const.
ACKs for top commit:
naumenkogs:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef
jonatack:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef per `git range-diff 52dcb1d 2f3602b 377e9cc`
vasild:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef
ryanofsky:
Code review ACK 377e9ccda469731d535829f184b70c73ed46b6ef. Looks good and feel free to ignore suggestions!
Tree-SHA512: 06fd6748770bad75ec8c966fdb73b7534c10bd61838f6f1b36b3f3d6a438e58f6a7d0edb011977e5c118ed7ea85325fac35e10dde520fef249f7a780cf500a85
|
|
|
|
fa875349e22f2f0f9c2c98ee991372d08ff90318 Fix iwyu (MacroFake)
faad673716cfbad1e715f1bdf8ac00938a055aea Fix issues when calling std::move(const&) (MacroFake)
Pull request description:
Passing a symbol to `std::move` that is marked `const` is a no-op, which can be fixed in two ways:
* Remove the `const`, or
* Remove the `std::move`
ACKs for top commit:
ryanofsky:
Code review ACK fa875349e22f2f0f9c2c98ee991372d08ff90318. Looks good. Good for univalue to support c++11 move optimizations
Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
|
|
In order to prevent memory DoS, we must ensure that we don't accept a new
header into memory until we've performed anti-DoS checks, such as verifying
that the header is part of a sufficiently high work chain. This commit adds a
new argument to AcceptBlockHeader() so that we can ensure that all call-sites
which might cause a new header to be accepted into memory have to grapple with
the question of whether the header is safe to accept, or needs further
validation.
This patch also fixes two places where low-difficulty-headers could have been
processed without such validation (processing an unrequested block from the
network, and processing a compact block).
Credit to Niklas Gögge for noticing this issue, and thanks to Sjors Provoost
for test code.
|
|
|
|
The rule against difficulty adjustments changing by more than a factor of 4 can
be helpful for anti-DoS measures in contexts where we lack a full headers
chain, so expose this functionality separately and in the narrow case where we
only know the height, new value, and old value.
Includes fuzz test by Martin Zumsande.
|
|
|
|
It was used back when we didn't have a concept of descriptor. Now we
can check for solvability using descriptors.
|
|
In src/test/fuzz/script_sigcache.cpp, we should really be setting up a
full working BasicTestingSetup. The initialize_ function is only run
once anyway.
In src/test/txvalidationcache_tests.cpp, the Dersig100Setup inherits
from BasicTestingSetup, which should have already set up a global script
execution cache without the need to explicitly call
InitScriptExecutionCache.
|
|
|
|
It is part of the node library. Also, it won't be moved to the kernel
lib, as it will be pruned of ArgsManager.
-BEGIN VERIFY SCRIPT-
# Move module
git mv src/mempool_args.cpp src/node/
git mv src/mempool_args.h src/node/
# Replacements
sed -i 's:mempool_args\.h:node/mempool_args.h:g' $(git grep -l mempool_args)
sed -i 's:mempool_args\.cpp:node/mempool_args.cpp:g' $(git grep -l mempool_args)
sed -i 's:MEMPOOL_ARGS_H:NODE_MEMPOOL_ARGS_H:g' $(git grep -l MEMPOOL_ARGS_H)
-END VERIFY SCRIPT-
|
|
|
|
|
|
Each alias is only used in one place.
|
|
Apart from tests, it is only used in one place, so there is no need for
an alias.
|
|
|
|
and rename it
dd065dae9fcebd6806ff67703ffa8128e80b97cc refactor: Make mapBlocksUnknownParent local, and rename it (Hennadii Stepanov)
Pull request description:
This PR is a second attempt at #19594. This PR has two motivations:
- Improve code hygiene by eliminating a global variable, `mapBlocksUnknownParent`
- Fix fuzz test OOM when running too long ([see #19594 comment](https://github.com/bitcoin/bitcoin/pull/19594#issuecomment-958801638))
A minor added advantage is to release `mapBlocksUnknownParent` memory when the reindexing phase is done. The current situation is somewhat similar to a memory leak because this map exists unused for the remaining lifetime of the process. It's true that this map should be empty of data elements after use, but its internal metadata (indexing structures, etc.) can have non-trivial size because there can be many thousands of simultaneous elements in this map.
This PR helps our efforts to reduce the use of global variables. This variable isn't just global, it's hidden inside a function (it looks like a local variable but has the `static` attribute).
This global variable exists because the `-reindex` processing code calls `LoadExternalBlockFile()` multiple times (once for each block file), but that function must preserve some state between calls (the `mapBlocksUnknownParent` map). This PR fixes this by allocating this map as a local variable in the caller's scope and passing it in on each call. When reindexing completes, the map goes out of scope and is deallocated.
I tested this manually by reindexing on mainnet and signet. Also, the existing `feature_reindex.py` functional test passes.
ACKs for top commit:
mzumsande:
re-ACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
theStack:
re-ACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
shaavan:
reACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
Tree-SHA512: 9cd20e44d2fa1096dd405bc107bc065ea8f904f5b3f63080341b08d8cf57b790df565f58815c2f331377d044d5306708b4bf6bdfc5ef8d0ed85d8e97d744732c
|
|
`LimitOrphans` then return void
b4b657ba57a2ce31b3c21ea9245aad26d5b06a57 refactor: log `nEvicted` message in `LimitOrphans` then return void (chinggg)
Pull request description:
Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49347
LimitOrphans() can log expired tx and it should log evicted tx as well instead of returning the `nEvicted` number for caller to print the message.
Since `LimitOrphans()` now returns void, the redundant assertion check in fuzz test is also removed.
Top commit has no ACKs.
Tree-SHA512: 18c41702321b0e59812590cd389f3163831d431f4ebdc3b3e1e0698496a6bdbac52288f28f779237a58813c6717da1a35e8933d509822978ff726c1b13cfc778
|
|
`LimitOrphans()` can log expired tx and it should log evicted tx as well
instead of returning the number for caller to print the message.
Since `LimitOrphans()` now return void, the redundant assertion check in
fuzz test is also removed.
|
|
fa64dd6673767992eb4e0e775fb0afdfd298610d refactor: Use type-safe std::chrono for addrman time (MarcoFalke)
fa2ae373f33fa76dc4e435e7cb4778055aa6afd5 Add type-safe AdjustedTime() getter to timedata (MarcoFalke)
fa5103a9f5f8559ab005c0b012d3d3a8057d81fb Add ChronoFormatter to serialize (MarcoFalke)
fa253d385f9201ea10beacecf3e0e80ff69f3138 util: Add HoursDouble (MarcoFalke)
fa21fc60c292ab947b2200e54201440f16230566 scripted-diff: Rename addrman time symbols (MarcoFalke)
fa9284c3e9acec4b44b2560256f27b3d78c753e2 refactor: Remove not needed std::max (MacroFake)
Pull request description:
Those refactors are overlapping with, but otherwise largely unrelated to #24662.
ACKs for top commit:
naumenkogs:
utACK fa64dd6673767992eb4e0e775fb0afdfd298610d
dergoegge:
Code review ACK fa64dd6673767992eb4e0e775fb0afdfd298610d
Tree-SHA512: a50625e78036e7220a11997e6d9b6c6b317cb38ce02b1835fb41cbee2d8bfb1faf29b29d8990be78d6b5e15e9a9d8dec33bf25fa439b47610ef708950969724b
|
|
fa57c449cf45a4f1df195970c711bba8f02f3cc6 fuzz: Remove no-op SetMempoolConstraints (MacroFake)
Pull request description:
Now that the mempool no longer uses the args manager (after commit e4e201dfd9a9dbd8e22cac688dbbde16234cd937), there is no point setting the mempool limits after it is constructed.
Fix that by setting them once right before the mempool is constructed.
ACKs for top commit:
dongcarl:
utACK fa57c449cf45a4f1df195970c711bba8f02f3cc6
glozow:
utACK fa57c449cf45a4f1df195970c711bba8f02f3cc6
Tree-SHA512: d236f9cdcee8c2076272b82c97f8a5942f1ecf119ab36edafd42088ef97554592348a61e1fbe504fd52b30301ef0177813042599ad12e8cb95b4a20586c85bb0
|
|
UniValue::VNULL
fa28d0f3c3fe528dae7fd6dc7725219b9bdf0e1b scripted-diff: Replace NullUniValue with UniValue::VNULL (MacroFake)
fa962103e8eb0b078b83943a21831be39e7716c9 fuzz: refactor: Replace NullUniValue with UniValue{} (MacroFake)
Pull request description:
This refactor is needed to disable the (potentially expensive for large json) UniValue copy constructors.
ACKs for top commit:
fanquake:
ACK fa28d0f3c3fe528dae7fd6dc7725219b9bdf0e1b
Tree-SHA512: 7d4204cce0a6fc4ecda96973de77d15b7e4c7caa3e0e890e1f5b9a4b9ace8b240b1f7565d6ab586e168a5fa1201b6c60a924868ef34d6abfbfd8ab7f0f99fbc7
|
|
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
ren nLastTry m_last_try
ren nLastSuccess m_last_success
ren nLastGood m_last_good
ren nLastCountAttempt m_last_count_attempt
ren nSinceLastTry since_last_try
ren nTimePenalty time_penalty
ren nUpdateInterval update_interval
ren fCurrentlyOnline currently_online
-END VERIFY SCRIPT-
|
|
test/lint/all-lint.py includes the codespell lint
|
|
This is needed for the scripted-diff to compile in the next commit
|
|
safety, consistent behavior
3a61fc56a0ad6ed58570350dcfd9ed2d10239b48 refactor: move CBlockIndex#ToString() from header to implementation (Jon Atack)
57865eb51288852c3ce99607eff76c61ae5f5365 CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash() (Jon Atack)
99e8ec8721a52cd08bdca31f6e926c9c1ce281fb CDiskBlockIndex: remove unused ToString() class member (Jon Atack)
14aeece462b149eaf0d28a37d55cc169df99b2cb CBlockIndex: ensure phashBlock is not nullptr before dereferencing (Jon Atack)
Pull request description:
Fix a few design issues, potential footguns and inconsistent behavior in the CBlockIndex and CDiskBlockIndex classes.
- Ensure phashBlock in `CBlockIndex#GetBlockHash()` is not nullptr before dereferencing and remove a now-redundant assert preceding a GetBlockHash() caller. This protects against UB here, and in case of failure (which would indicate a consensus bug), the debug log will print `bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion 'phashBlock != nullptr' failed. Aborted` instead of `Segmentation fault`.
- Remove the unused `CDiskBlockIndex#ToString()` class member, and mark the inherited `CBlockIndex#ToString()` public interface member as deleted to disallow calling it in the derived CDiskBlockIndex class.
- Rename the `CDiskBlockIndex GetBlockHash()` class member to `ConstructBlockHash()`, which also makes sense as they perform different operations to return a blockhash, and mark the inherited `CBlockIndex#GetBlockHash()` public interface member as deleted to disallow calling it in the derived CDiskBlockIndex class.
- Move `CBlockIndex#ToString()` from header to implementation, which also allows dropping `tinyformat.h` from the header file.
Rationale and discussion regarding the CDiskBlockIndex changes:
Here is a failing test on master that demonstrates the inconsistent behavior of the current design: calling the same inherited public interface functions on the same CDiskBlockIndex object should yield identical behavior, but does not.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6dc522b421..dac3840f32 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
const CBlockIndex* tip = chainman.ActiveTip();
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
+ // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it.
+ // Test that calling the same inherited interface functions on the same
+ // object yields identical behavior.
+ CDiskBlockIndex index{tip};
+ CBlockIndex *pB = &index;
+ CDiskBlockIndex *pD = &index;
+ BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash());
+ BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString());
```
(build and run: `$ ./src/test/test_bitcoin -t validation_chainstatemanager_tests`)
The GetBlockHash() test assertion only passes on master because the different methods invoked by the current design happen to return the same result. If one of the two is changed, it fails like the ToString() assertion does.
Redefining inherited non-virtual functions is well-documented as incorrect design to avoid inconsistent behavior (see Scott Meyers, Effective C++, Item 36). Class usage is confusing when the behavior depends on the pointer definition instead of the object definition (static binding happening where dynamic binding was expected). This can lead to unsuspected or hard-to-track bugs.
Outside of critical hot spots, correctness usually comes before optimisation, but the current design dates back to main.cpp and it may possibly have been chosen to avoid the overhead of dynamic dispatch. This solution does the same: the class sizes are unchanged and no vptr or vtbl is added.
There are better designs for doing this that use composition instead of inheritance, or that separate the public interface from the private implementations. One example of the latter would be a non-virtual public interface that calls private virtual implementation methods, i.e. the Template pattern via the Non-Virtual Interface (NVI) idiom.
ACKs for top commit:
vasild:
ACK 3a61fc56a0ad6ed58570350dcfd9ed2d10239b48
Tree-SHA512: 9ff358ab0a6d010b8f053ad8303c6d4d061e62d9c3755a56b9c9f5eab855d02f02bee42acc77dfa0cbf4bb5cb775daa72d675e1560610a29bd285c46faa85ab7
|
|
|
|
and mark the inherited CBlockIndex#GetBlockHash public interface member
as deleted, to disallow calling it in the derived CDiskBlockIndex class.
Here is a failing test on master demonstrating the inconsistent behavior of the
current design: calling the same inherited public interface functions on the
same CDiskBlockIndex object should yield identical behavior.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6dc522b421..dac3840f32 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
const CBlockIndex* tip = chainman.ActiveTip();
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
+ // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it.
+ // Test that calling the same inherited interface functions on the same
+ // object yields identical behavior.
+ CDiskBlockIndex index{tip};
+ CBlockIndex *pB = &index;
+ CDiskBlockIndex *pD = &index;
+ BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash());
+ BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString());
+
```
The GetBlockHash() test assertion only passes on master because the different
methods invoked by the current design happen to return the same result. If one
of the two is changed, it fails like the ToString() assertion does.
Redefining inherited non-virtual functions is well-documented as incorrect
design to avoid inconsistent behavior (see Scott Meyers, "Effective C++", Item
36). Class usage is confusing when the behavior depends on the pointer
definition instead of the object definition (static binding happening where
dynamic binding was expected). This can lead to unsuspected or hard-to-track
bugs.
Outside of critical hot spots, correctness usually comes before optimisation,
but the current design dates back to main.cpp and it may possibly have been
chosen to avoid the overhead of dynamic dispatch. This solution does the same:
the class sizes are unchanged and no vptr or vtbl is added.
There are better designs for doing this that use composition instead of
inheritance or that separate the public interface from the private
implementations. One example of the latter would be a non-virtual public
interface that calls private virtual implementation methods, i.e. the Template
pattern via the Non-Virtual Interface (NVI) idiom.
|
|
and mark its inherited CBlockIndex#ToString public interface member
as deleted, to disallow calling it in the derived CDiskBlockIndex class.
|
|
|
|
and use it where possible
facc2fa7b8a218a0df6a19772e1641ea68dda2e3 Use AutoFile where possible (MacroFake)
6666803c897e4ad27b45cb74e3a9aa74a335f1bf streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8a218a0df6a19772e1641ea68dda2e3
fanquake:
ACK facc2fa7b8a218a0df6a19772e1641ea68dda2e3
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
|
|
|
|
CNode::nLocalServices to Peer
8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8eea1ad5be1a4bf8da63651e197a20359b2 [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe18dc5d81ce290876353555b51125127d1 [net processing] Remove CNode::nServices (John Newbery)
7d1c0369340cb752f0d78e24f4251595534bf5e9 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d51bfb6a34f1d5efccfb3d8786a51a4534 [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb528f7d7b33e2f2e443c5610a1551c7f099b [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47d5c09b59fd3153700751c74e63edc7d7e [net processing] Add m_our_services and m_their_services to Peer (John Newbery)
Pull request description:
Another step in #19398. Which services we offer to a peer and which services they offer to us is application layer data and should not be stored on `CNode`.
This is also a prerequisite for adding `PeerManager` unit tests (See #25515).
ACKs for top commit:
MarcoFalke:
ACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 🔑
jnewbery:
utACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67
mzumsande:
Code Review ACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67
Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
|
|
Co-authored-by: Larry Ruane <larryruane@gmail.com>
|
|
from `ArgsManager`
cb3e9a1e3f8d72daaa361fc45dd853775e754b9d Move {Load,Dump}Mempool to kernel namespace (Carl Dong)
aa306765419f7dbea12b12e15553039835ba0e4d Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong)
06b88ffb8ae7f2b2a93a32908cd80e77fafd270c LoadMempool: Pass in load_path, stop using gArgs (Carl Dong)
b857ac60d9a0433036519c26675378bbf56a1de1 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong)
b3267258b052557fc136b9a4dcb754afb9219470 Move FopenFn to fsbridge namespace (Carl Dong)
ae1e8e37567fa603a5977d7d05105c682dd3f7db mempool: Use NodeClock+friends for LoadMempool (Carl Dong)
f9e8e5719f28d84f68f7d75e26c8e7fccac8e7d3 mempool: Improve comments for [GS]etLoadTried (Carl Dong)
813962da0b17b918941c6849996845e35d84a451 scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong)
413f4bb52b72e082ad8716664ede48352b8e7e5a DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong)
bd4407817e523e3c5b347bc6be25ed007cb27034 DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong)
c84390b741ab7b61c9f702d8b447c8cadc1257c8 test/mempool_persist: Test manual savemempool when -persistmempool=0 (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
This PR moves `{Dump,Load}Mempool` into its own `kernel/mempool_persist` module and introduces `ArgsManager` `node::` helpers in `node/mempool_persist_args`to remove the scattered calls to `GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)`.
More context can be gleaned from the commit messages.
-----
One thing I was reflecting on as I wrote this was that in the long run, I think we should probably invert the validation <-> mempool relationship. Instead of mempool not depending on validation, it might make more sense to have validation not depend on mempool. Not super urgent since `libbitcoinkernel` will include both validation and mempool, but perhaps something for the future.
ACKs for top commit:
glozow:
re ACK cb3e9a1e3f8d72daaa361fc45dd853775e754b9d via `git range-diff 7ae032e...cb3e9a1`
MarcoFalke:
ACK cb3e9a1e3f 🔒
ryanofsky:
Code review ACK cb3e9a1e3f8d72daaa361fc45dd853775e754b9d
Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d
|
|
|
|
Also:
1. Add the newly introduced kernel/mempool_persist.cpp to IWYU CI script
2. Add chrono mapping for iwyu
|
|
Also:
1. Have CChainState::LoadMempool and ::ThreadImport take in paths and
pass it through untouched to LoadMempool.
2. Make LoadMempool exit early if the load_path is empty.
3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass
in an empty path if mempool persistence is disabled.
|
|
Not only does this increase coverage, it is also more correct in that
when ::LoadMempool is called with a mempool and chainstate, it calls
AcceptToMemoryPool with just the chainstate.
AcceptToMemoryPool will then act on the chainstate's mempool via
CChainState::GetMempool, which may be different from the mempool
originally passed to ::LoadMempool. (In this fuzz test's case, it
definitely is different)
Also, move DummyChainstate to its own file since it's now used by the
validation_load_mempool fuzz test to replace CChainState's m_mempool.
|
|
Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions
in node/mempool_persist_args.{h,cpp} which are used by non-kernel
DumpMempool callers to determine whether or not to automatically dump
the mempool and where to dump it to.
|
|
|
|
|