| Age | Commit message (Collapse) | Author |
|---|
|
|
|
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.
|
|
This further encapsulates syscalls inside the `Sock` class.
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
|
|
This makes the callers mockable.
|
|
|
|
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.
|
|
|
|
|
|
|
|
own translation unit
0101d2bc3c3bcf698d6cc2a237a680fc52395987 [net] Move eviction logic to its own file (dergoegge)
c741d748d4d9836940b99091cc7be09c65efcb79 [net] Move ConnectionType to its own file (Cory Fields)
a3c27070396ab8c2941c437e8099547e8fc9c110 [net] Add connection type to NodeEvictionCandidate (dergoegge)
42aa5d5b6269d27af525d5001907558442e96023 [net] Add NoBan status to NodeEvictionCandidate (dergoegge)
Pull request description:
This PR splits of the first couple commits from #25268 that move the inbound eviction logic from `net.{h,cpp}` to `eviction.{h,cpp}`.
Please look at #25268 for motivation and conceptual review.
ACKs for top commit:
jnewbery:
utACK 0101d2bc3c3bcf698d6cc2a237a680fc52395987
theuni:
utACK 0101d2bc3c3bcf698d6cc2a237a680fc52395987. I quickly verified with `git --color-moved` that the move-only changes are indeed move-only.
Tree-SHA512: e0c345a698030e049cb22fe281b44503c04403c5be5a3750ca14bfcc603a162ac6bac9a39552472feb57c460102b7ca91430b8ad6268f2efccc49b5e8959331b
|
|
6eb0909cb7d5883a258f76ad6cf2c989fc6f892f fuzz: add low-level target for txorphanage (chinggg)
Pull request description:
This adds a low-level fuzz target for orphan transaction handling by creating random transactions and calling all functions in `TxOrphanage`.
It cannot simulate real-world `orphan/unorphan` scenarios effectively since it does not maintain any state about the node and the chain. A high-level fuzz target which construct well-designed transaction graphs will be added later.
ACKs for top commit:
MarcoFalke:
review ACK 6eb0909cb7d5883a258f76ad6cf2c989fc6f892f 🐈
Tree-SHA512: b4d64f5941df77d13981f75ec170cef6ffabe782797c982ede7f34134be01dc0026dd7c0bee614bc1d64715e90a933d2a8c95974d402e32eaba8e24cc928299e
|
|
|
|
|
|
|
|
e866f0d0666664885d4c15c79bf59cc59975887a [functional test] submitrawpackage RPC (glozow)
fa076515b07ac4b10b2134e323bf4f56be5996a8 [rpc] add new submitpackage RPC (glozow)
Pull request description:
It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a `-regtest` only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn't exist.
Note that the functional tests are there to ensure the RPC interface is working properly; they aren't for testing policy itself. See src/test/txpackage_tests.cpp.
ACKs for top commit:
t-bast:
Tested ACK against eclair https://github.com/bitcoin/bitcoin/pull/24836/commits/e866f0d0666664885d4c15c79bf59cc59975887a
ariard:
Code Review ACK e866f0d0
instagibbs:
code review ACK e866f0d0666664885d4c15c79bf59cc59975887a
Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
|
|
|
|
`ArgsManager`
d1684beabe5b738c2cc83de83e1aaef11a761b69 fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30e8e6118d74a4e568744cb9d03f7f5d init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c4124293d948735756f84efc85262ea66f mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b1030182eff92ef91181e17c7dd498c7e164 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf358a72b9457d370282e57f4be1c5c849 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014695ac235c96d48791098168dfdc9db mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd31077bbe99d11a54d6c2c250afc8085 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321deb61b9f6888e4e10752b9d972fd26e scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd8185cb7ad532bc16feb9d302b05d9697 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5eb6fcb60333812c770d80227cf7b64d init: Only determine maxmempool once (Carl Dong)
386c9472c8764738282e6d163b42e15a8feda7ea mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd2eecc478c7660434b1ebf6a64095a0 pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a68d6cda8ed3efd0598c0e4121b366bb scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca604f0221171bfde3059b34f5d0fb1cd ArgsMan: Add Get*Arg functions returning optional (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this.
This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time.
These options are:
- `-maxmempool` -> `CTxMemPool::Options::max_size`
- `-mempoolexpiry` -> `CTxMemPool::Options::expiry`
- `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count`
- `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size`
- `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count`
- `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size`
More context can be gleaned from the commit messages. The important commits are:
- 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions"
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs"
- 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits"
Reviewers: Help needed in the following commits (see commit messages):
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits"
Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.
-----
TODO:
- [x] Use the more ergonomic `CTxMemPool::Options` where appropriate
- [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions`
-----
Questions for Reviewers:
1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?)
2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`?
ACKs for top commit:
MarcoFalke:
ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜
ryanofsky:
Code review ACK d1684beabe5b738c2cc83de83e1aaef11a761b69. Just minor cleanups since last review, mostly switching to brace initialization
Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
|
|
|
|
Reviewers: Note that CTxMemPool now requires a non-defaulted
CTxMemPool::Options for its constructor. Meaning that there's no need to
worry about a stray CTxMemPool constructor somewhere defaulting to
something incorrect. All instances of CTxMemPool construction are
addressed here in this commit.
We set options for CTxMemPool and construct it in many different ways. A
good example can be seen in how we determine CTxMemPool's check_ratio in
AppInitMain(...).
1. We first set the default based on chainparams's
DefaultConsistencyChecks()
2. Then, we apply the ArgsManager option on top of that default
3. Finally, we clamp the result of that between 0 and 1 Million
With this patch, most CTxMemPool construction are along the lines of:
MemPoolOptions mempool_opts{...default overrides...};
ApplyArgsManOptions(argsman, mempool_opts);
...hard overrides...
CTxMemPool pool{mempool_opts};
This "compositional" style of building options means that we can omit
unnecessary/irrelevant steps wherever we want but also maintain full
customizability.
For example:
- For users of libbitcoinkernel, where we eventually want to remove
ArgsManager, they simply won't call (or even know about)
ApplyArgsManOptions.
- See src/init.cpp to see how the check_ratio CTxMemPool option works
after this change.
A MemPoolOptionsForTest helper was also added and used by tests/fuzz
tests where a local CTxMemPool needed to be created.
The change in src/test/fuzz/tx_pool.cpp seemingly changes behaviour by
applying ArgsManager options on top of the CTxMemPool::Options defaults.
However, in future commits where we introduce flags like -maxmempool,
the call to ApplyArgsManOptions is actually what preserves the existing
behaviour. Previously, although it wasn't obvious, our CTxMemPool would
consult gArgs for flags like -maxmempool when it needed it, so it
already relied on ArgsManager information. This patchset just laid bare
the obfuscatory perils of globals.
[META] As this patchset progresses, we will move more and more
CTxMemPool-relevant options into MemPoolOptions and add their
ArgsMan-related logic to ApplyArgsManOptions.
|
