Test each component of the RBF policy in isolation. Unlike the RBF
functional tests, these do not rely on things like RPC results, mempool
submission, etc.
`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.
fa74e726c4 refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) (MacroFake)
fa3b3cb9b5 Expose underlying clock in CThreadInterrupt (MacroFake)
Pull request description:
This gets rid of the `value*1000` manual conversion.
ACKs for top commit:
naumenkogs:
utACK fa74e726c4
dergoegge:
Code review ACK fa74e726c4
Tree-SHA512: 90409c05c25f0dd2f1c4dead78f707ebfd78b7d84ea4db9fcefd9c4958a1a3338ac657cd9e99eb8b47d52d4485fa3c947dce4ee1559fb56ae65878685e1ed9a3
fa2247a9f9 refactor: Make CTransaction constructor explicit (MacroFake)
Pull request description:
It involves calculating two hashes, so the performance impact should be
made explicit.
Also, add the module to iwyu.
ACKs for top commit:
aureleoules:
ACK fa2247a9f9.
hebasto:
ACK fa2247a9f9, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: e236c352a472c7edfd4f0319a5a16a59f627b0ab7eb8531b53c75d730a3fa3e990a939978dcd952cd73e647925fc79bfa6d9fd87624bbc3ef180f40f95acef19
fa57c449cf fuzz: Remove no-op SetMempoolConstraints (MacroFake)
Pull request description:
Now that the mempool no longer uses the args manager (after commit e4e201dfd9), 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 fa57c449cf
glozow:
utACK fa57c449cf
Tree-SHA512: d236f9cdcee8c2076272b82c97f8a5942f1ecf119ab36edafd42088ef97554592348a61e1fbe504fd52b30301ef0177813042599ad12e8cb95b4a20586c85bb0
fa28d0f3c3 scripted-diff: Replace NullUniValue with UniValue::VNULL (MacroFake)
fa962103e8 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 fa28d0f3c3
Tree-SHA512: 7d4204cce0a6fc4ecda96973de77d15b7e4c7caa3e0e890e1f5b9a4b9ace8b240b1f7565d6ab586e168a5fa1201b6c60a924868ef34d6abfbfd8ab7f0f99fbc7
3a61fc56a0 refactor: move CBlockIndex#ToString() from header to implementation (Jon Atack)
57865eb512 CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash() (Jon Atack)
99e8ec8721 CDiskBlockIndex: remove unused ToString() class member (Jon Atack)
14aeece462 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 3a61fc56a0
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.
faf9accd66 Use HashWriter where possible (MacroFake)
faa5425629 Add HashWriter 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 `HashWriter`. `CHashWriter` remains in places where it is not yet possible.
ACKs for top commit:
sipa:
utACK faf9accd66
Empact:
utACK faf9accd66
Tree-SHA512: 544cc712436e49f6e608120bcd3ddc5ea72dd236554ce30fb6cfff34a92d7e67b6e6527336ad0f5b6365e2b2884f4c6508aef775953ccd9312f17752729703f2
facc2fa7b8 Use AutoFile where possible (MacroFake)
6666803c89 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 facc2fa7b8
fanquake:
ACK facc2fa7b8
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
7878f97bf1 indexes, refactor: Remove CChainState use in index CommitInternal method (Ryan Ofsky)
ee3a079fab indexes, refactor: Remove CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
dc971be083 indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods (Ryan Ofsky)
bef4e405f3 indexes, refactor: Remove CBlockIndex* uses in index Init methods (Ryan Ofsky)
addb4f2af1 indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function (Ryan Ofsky)
33b4d48cfc indexes, refactor: Pass Chain interface instead of CChainState class to indexes (Ryan Ofsky)
a0b5b4ae5a interfaces, refactor: Add more block information to block connected notifications (Ryan Ofsky)
Pull request description:
Start transitioning index code away from using internal node types like `CBlockIndex` and `CChain` so index code is less coupled to node code and index code will later be able to stop locking cs_main and sync without having to deal with validationinterface race conditions, and so new indexes are easier to write and can run as plugins or separate processes.
This PR contains the first 7 commits from https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-1165625977 which have been split off for easier review. Previous review comments can be found in #24230
ACKs for top commit:
MarcoFalke:
ACK 7878f97bf1 though did not review the last commit 🤼
mzumsande:
Code Review ACK 7878f97bf1
Tree-SHA512: f84ac2eb6dca2c305566ddeb35ea14d0b71c00860c0fd752bbcf1a0188be833d8c2a6ac9d3ef6ab5b46fbd02d7a24cbb8f60cf12464cb8ba208e22287f709989
a02f3f19f5 tidy: use misc-unused-using-decls (fanquake)
d6787bc19b refactor: remove unused using directives (fanquake)
3617634324 validation: remove unused using directives (eugene)
Pull request description:
Adds https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html to our clang-tidy.
PR'd after the discussion in #25433 (which it includes).
ACKs for top commit:
jamesob:
Github ACK a02f3f19f5
Tree-SHA512: 2bb937c1cc90006e69054458d845fb54f287567f4309c773a3fc859f260558c32ff51fc1c2ce9b43207426f3547e7ce226c87186103d741d5efcca19cd355253
8d8eeb422e [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8eea1 [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe18d [net processing] Remove CNode::nServices (John Newbery)
7d1c036934 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d51b [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb528f7 [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47d5c [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 8d8eeb422e🔑
jnewbery:
utACK 8d8eeb422e
mzumsande:
Code Review ACK 8d8eeb422e
Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
cb3e9a1e3f Move {Load,Dump}Mempool to kernel namespace (Carl Dong)
aa30676541 Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong)
06b88ffb8a LoadMempool: Pass in load_path, stop using gArgs (Carl Dong)
b857ac60d9 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong)
b3267258b0 Move FopenFn to fsbridge namespace (Carl Dong)
ae1e8e3756 mempool: Use NodeClock+friends for LoadMempool (Carl Dong)
f9e8e5719f mempool: Improve comments for [GS]etLoadTried (Carl Dong)
813962da0b scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong)
413f4bb52b DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong)
bd4407817e DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong)
c84390b741 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 cb3e9a1e3f via `git range-diff 7ae032e...cb3e9a1`
MarcoFalke:
ACK cb3e9a1e3f🔒
ryanofsky:
Code review ACK cb3e9a1e3f
Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d
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.
ffc79b8e49 qa: functional test Miniscript watchonly support (Antoine Poinsot)
bfb036756a Miniscript support in output descriptors (Antoine Poinsot)
4a082887be qa: better error reporting on descriptor parsing error (Antoine Poinsot)
d25d58bf5f miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot)
c38c7c5817 miniscript: don't check for top level validity at parsing time (Antoine Poinsot)
Pull request description:
This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of #24147 for a description of Miniscript and a rationale of having it in Bitcoin Core.
On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support.
A minified corpus of Miniscript Descriptors for the `descriptor_parse` fuzz target is available at https://github.com/bitcoin-core/qa-assets/pull/92.
The Miniscript descriptors used in the unit tests here and in #24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript.
This PR contains code and insights from Pieter Wuille.
ACKs for top commit:
Sjors:
re-utACK ffc79b8e49
achow101:
ACK ffc79b8e49
w0xlt:
reACK ffc79b8e49
Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9
Prior to this commit, the peer was connected, and then the services and
connectivity fields in the CNode object were manually set. Instead, send
p2p `version` and `verack` messages, and have net_processing's internal
logic set the state of the node.
This ensures that the node's internal state is consistent with how it
would be set in the live code.
Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE`
which was not a problem since `CNode::fClient` and
`CNode::m_limited_node` are default initialised to false. Now that we
are doing the actual version handshake, the values of `fClient` and
`m_limited_node` are set during the handshake and cause the test to fail
if we do not set `dummyNode1.nServices` to a reasonable value
(NODE_NETWORK | NODE_WITNESS).
Miniscript descriptors are defined under P2WSH context (either `wsh()`
or `sh(wsh())`).
Only sane Miniscripts are accepted, as insane ones (although valid by
type) can have surprising behaviour with regard to malleability
guarantees and resources limitations.
As Miniscript descriptors are longer and more complex than "legacy"
descriptors, care was taken in error reporting to help a user determine
for what reason a provided Miniscript is insane.
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
