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.
Watchonly wallets do not have any private keys to encrypt. It does not
make sense to encrypt such wallets, so disable the option to encrypt
them.
This avoids an assertion that can be hit when encrypting watchonly descriptor
wallets.
m_is_loaded/IsLoaded() doesn't actually indicate whether or not the
mempool was successfully, loaded, but rather if a load has been
attempted and did not result in a catastrophic ShutdownRequested.
-BEGIN VERIFY SCRIPT-
find_regex="\bm_is_loaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@m_load_tried@g"
find_regex="\bIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@GetLoadTried@g"
find_regex="\bSetIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@SetLoadTried@g"
-END VERIFY SCRIPT-
fa277cd55dd105018e7d1220b4c3d96779e6b0f4 univalue: Throw exception on invalid pushes over silent ignore (MacroFake)
ccccc17b91698aa09ac85f7efea298f3938594ad refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL (MacroFake)
Pull request description:
The return value of the `push*` helpers is never used, but important to determine if the operation was successful. One way to fix this would be to add the "nodiscard" attribute. However, this would make the code (and this diff) overly verbose for no reason.
So fix it by removing the never used return value. Also, fail verbosely in case of a programming mistake.
ACKs for top commit:
furszy:
code ACK fa277cd5
Tree-SHA512: ef212a5bf5ae6bbad20acc4dafa3715521e81544185988d1eab724f440e4864a27e686aff51d5bc51b3017892c2eb8e577bcb8f37e8ddbaa0d8833bb622f2f9c
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.
ffc79b8e492c6dd1352e528fd82e45d8d25eaa04 qa: functional test Miniscript watchonly support (Antoine Poinsot)
bfb036756ad6e187fd6d3abfefe5804bb54a5c71 Miniscript support in output descriptors (Antoine Poinsot)
4a082887bee76a96deada5dbd7f991c23b301c54 qa: better error reporting on descriptor parsing error (Antoine Poinsot)
d25d58bf5f301d3bb8683bd67c8847a4957d8e97 miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot)
c38c7c5817b7e73cf0f788855c4aba59c287b0ad 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 ffc79b8e492c6dd1352e528fd82e45d8d25eaa04
achow101:
ACK ffc79b8e492c6dd1352e528fd82e45d8d25eaa04
w0xlt:
reACK ffc79b8e49
Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9
ce8b0f971b94e68db1e902dbd20dd99dcf9bcb0a Use designated initializers for ChainstateManager::Options (Carl Dong)
38377002671d038aadb01e1521ea95c97838cedc Move ChainstateManagerOpts into kernel:: namespace (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
This PR is **_NOT_** dependent on any other PRs.
-----
Places `ChainstateManager::Options` into the `kernel::` namespace and use designated initializers for construction.
ACKs for top commit:
ryanofsky:
Code review ACK ce8b0f971b94e68db1e902dbd20dd99dcf9bcb0a
Tree-SHA512: 16a11b5051a2432ca4b6fa7b253376606fef619ace499dfe64d033c8fbe3e1a1875a7c946d7cd54bd908363886244ddf3a192e2f0c801ffbed40d60aad65e442
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).
As UniValue provides several constructors for integral types, the
compiler is unable to select one if the passed type does not exactly
match. This is unintuitive for developers and forces them to write
verbose and brittle code.
For example, there are many places where an unsigned int is cast to a
signed int. While the cast is safe in practice, it is still needlessly
verbose and confusing as the value can never be negative. In fact it
might even be unsafe if the unsigned value is large enough to map to a
negative signed one.
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>
This is helpful for finer grained descriptor parsing error: when there
are multiple errors to report in a Miniscript descriptor start with the
"smallest" fragments: the ones closer to be a leaf.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
UniValue does not have a constructor for enum values, however the
compiler will decay the enum into an int and select that constructor.
Avoid this compiler magic and clarify the code by explicitly selecting
the int-constructor.
This is needed for the next commit.
fa475e9c7977a952617738f2ee8cf600c07d4df8 refactor: Return BResult from restoreWallet (MacroFake)
fa8de09edc9ec4e6d171df80f746174a0ec58afb Prepare BResult for non-copyable types (MacroFake)
Pull request description:
This avoids the `error` in-out param (and if `warnings` is added to `BResult`, it will avoid passing that in-out param as well).
Also, as it is needed for this change, prepare `BResult` for non-copyable types.
ACKs for top commit:
w0xlt:
reACK fa475e9c79
ryanofsky:
Code review ACK fa475e9c7977a952617738f2ee8cf600c07d4df8. Changes since last review were replacing auto with explicit type and splitting commits
Tree-SHA512: 46350883572f13721ddd198f5dfb88d2fa58ebcbda416f74da3563ea15c920fb1e6ff30558526a4ac91c36c21e6afe27751a4e51b7b8bcbcbe805209f4e9014b
613e2211493ae2c78b71d1f4ea62661438d2cb73 test: remove unnecessary parens (Suhas Daftuar)
e939cf2b7645c2b20a68cb6129f3aebfdf287d61 Remove atomic for m_last_getheaders_timestamp (Suhas Daftuar)
Pull request description:
Eliminate the unnecessary atomic guarding `m_last_getheaders_timestamp`, which is only accessed in a single thread (thanks to MarcoFalke for pointing this out).
Also address a nit that came up in #25454.
ACKs for top commit:
MarcoFalke:
review ACK 613e2211493ae2c78b71d1f4ea62661438d2cb73
vasild:
ACK 613e2211493ae2c78b71d1f4ea62661438d2cb73
Tree-SHA512: 6d6c473735b450b8ad43aae5cf16ed419154d72f4a05c0a6ce6f26caecab9db2361041398b70bf9395611c107d50897f501fa5fdbabb2891144bbc2b479dfdad
faace13b712a349863eaf7c469069db17b6e9dd5 test: Remove duplicate MAX_BIP125_RBF_SEQUENCE constant (MacroFake)
fa0404dbb76abd7ebf7c14bd80ce7c6297e20466 scripted-diff: [test] Rename BIP125_SEQUENCE_NUMBER to MAX_BIP125_RBF_SEQUENCE (MacroFake)
Pull request description:
Not sure why the python constant is named differently than the constant in the C++ source code.
Especially, if we use this in context of MAX+1 (https://github.com/bitcoin/bitcoin/pull/25575#discussion_r918569813) the rename makes sense, in my eyes.
ACKs for top commit:
theStack:
Code-review ACK faace13b712a349863eaf7c469069db17b6e9dd5
glozow:
concept ACK faace13b712a349863eaf7c469069db17b6e9dd5
Tree-SHA512: 1dc8cd0f067717f6ace8121b660e99f2d0f8c485c0d61b80413e07e7830e7dfaf9ce2c922c63ba2c6b42e805d59d88cd0d9c80a4b4a2fca47e77a3aba6cd4ec6
fa8a1c06961f4b1826696e0db8dce81dce627721 rpc: Fix Univalue push_backV OOM in listtransactions (MacroFake)
Pull request description:
Related to, but not intended as a fix for #25229.
Currently the RPC will have the same data stored thrice:
* `UniValue ret` (memory filled by `ListTransactions`)
* `std::vector<UniValue> vec` (constructed by calling `push_backV`)
* `UniValue result` (the actual result, memory filled by `push_backV`)
Fix this by filling the memory only once:
* `std::vector<UniValue> ret` (memory filled by `ListTransactions`)
* Pass iterators to `push_backV` instead of creating a full copy
* Move memory into `UniValue result` instead of copying it
ACKs for top commit:
shaavan:
Code Review ACK fa8a1c06961f4b1826696e0db8dce81dce627721
Tree-SHA512: 1c3ca40fc8497134a4141195160e4aa9fe72b3c00c5998c972b58ad0eb498ebea05013f9105bb80e7264c9db1d0e7a2032396a8a4af1f326d831fbee20f32ea3
Overloading sleep_for is not needed, as
* seconds and minutes can be converted to milliseconds by the compiler,
not needing a duration_cast
* std::condition_variable::wait_for will convert milliseconds to the
duration type of the underlying clock
So simply expose the clock.
07f2c25d04c39a0074e1d9ee1b24b3e359c8153f refactor: add most of src/util to iwyu (fanquake)
Pull request description:
These files change infrequently, and not much header shuffling is required.
We don't add everything in src/util/ yet, because IWYU makes some
dubious suggestions, which I'm going to follow up with upstream.
Soon we'll swap `src/util/xyz.cpp` for just `src/util/`.
ACKs for top commit:
hebasto:
ACK 07f2c25d04c39a0074e1d9ee1b24b3e359c8153f, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 07d76435c2bff1a62c4967eb0efaafe619cc3bbaf4166741d8520927b24336c01aee59822f8082ee2a01e15046a0f5d506b4b23a6e40ceb750f3226ed8167847
2ef5294a5bb68ceb3797d2638567a172cc21699f rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff7b2f5e2820993443a6f868f6b0b20a test: add getblockfrompeer coverage of invalid inputs (Jon Atack)
Pull request description:
The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
Fix all issues.
ACKs for top commit:
brunoerg:
ACK 2ef5294a5bb68ceb3797d2638567a172cc21699f
Tree-SHA512: 454782cf6a44fd0e05483bb152153667ef5c8021358385ddcf89724fbbbd35e187362bdff757e00c99319527bc4c0b20c7187f67241d4585d767a29787142f25
c6c35db0571c2f2bdd34febbfe9acd52a42d7b95 wallet: change `ScanForWalletTransactions` to use `Ticks()` (w0xlt)
Pull request description:
This PR changes `ScanForWalletTransactions()` to use the `Ticks(Dur2 d)` function (introduced in #25456).
ACKs for top commit:
MarcoFalke:
cr ACK c6c35db0571c2f2bdd34febbfe9acd52a42d7b95
Tree-SHA512: 864e136b470baf22293dc03ae3400bbb34955389a1efc83862f006cfac84da9128c3a201ef051606c06f782a1fde84129261dd4b417cbfff854d5c359a92703e
