b1318dcc56 test: change `m_submitted_in_package` input to fuzz data provider boolean (ismaelsadeeq)
5615e16b70 tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` (ismaelsadeeq)
fcd4296648 doc: fix typo and update incorrect comment (ismaelsadeeq)
562664d263 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 b1318dcc56
glozow:
utACK b1318dcc56
Tree-SHA512: 45268729bc044da4748fe004524e0df696d2ec92c5bd053db9aad6e15675f3838429b2a7b9061a6b694be4dc319d1782a876b44df506ddd439d62ad07252d0e1
e03d6f7ed5 fuzz: set `m_fallback_fee`/`m_fee_mode` in `wallet_fees` target (brunoerg)
Pull request description:
`m_fallback_fee` and `m_fee_mode` are used in `GetMinimumFeeRate` but we're not setting any value for them in `wallet_fees` target. That's the reason fuzzing is never reaching the following code:
This PR fixes it.
ACKs for top commit:
maflcko:
review ACK e03d6f7ed5
achow101:
ACK e03d6f7ed5
murchandamus:
ACK e03d6f7ed5
Tree-SHA512: 5d364f5351d65762a3ddf88e3abb7bda401b7e4955285e083031d216fb50082b1ea98e2c065aff75a5a8a3d1bc4c2e5e3ca9f9478d902ee8f8d4347b6cbe53af
In reality some mempool transaction might be submitted in a package,
so change m_submitted_in_package to fuzz data provider boolean just like
m_has_no_mempool_parents.
The boolean indicates whether the transaction was added without enforcing mempool
fee limits. m_mempool_limit_bypassed is the correct variable name.
Also changes NewMempoolTransactionInfo booleans descriptions to the format that
is consistent with the codebase.
Making the `GenerateRandomKey` helper available to other modules via
key.{h.cpp} allows us to create random private keys directly at
instantiation of CKey, in contrast to the two-step process of creating
the instance and then having to call `MakeNewKey(...)`.
fae526345d Allow std::byte C-style array serialization (MarcoFalke)
fa898e6836 refactor: Print verbose serialize compiler error messages (MarcoFalke)
Pull request description:
Currently, trying to serialize an object that can't be serialized will fail with a short error message. For example, the diff and the error message:
```diff
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
index d75eb499b4..773f49845b 100644
--- a/src/test/serialize_tests.cpp
+++ b/src/test/serialize_tests.cpp
@@ -62,6 +62,8 @@ public:
BOOST_AUTO_TEST_CASE(sizes)
{
+ int b[4];
+ DataStream{} << b << Span{b};
BOOST_CHECK_EQUAL(sizeof(unsigned char), GetSerializeSize((unsigned char)0));
BOOST_CHECK_EQUAL(sizeof(int8_t), GetSerializeSize(int8_t(0)));
BOOST_CHECK_EQUAL(sizeof(uint8_t), GetSerializeSize(uint8_t(0)));
```
```
./serialize.h:765:6: error: member reference base type 'const int[4]' is not a structure or union
765 | a.Serialize(os);
| ~^~~~~~~~~~
```
```
./serialize.h:277:109: error: no matching function for call to 'UCharCast'
277 | template <typename Stream, typename B> void Serialize(Stream& s, Span<B> span) { (void)/* force byte-type */UCharCast(span.data()); s.write(AsBytes(span)); }
| ^~~~~~~~~
```
This is fine. However, it would be more helpful for developers and more accurate by the compiler to explain why each function is not selected.
Fix this by using C++20 concepts where appropriate.
ACKs for top commit:
ajtowns:
reACK fae526345d
achow101:
ACK fae526345d
TheCharlatan:
Re-ACK fae526345d
Tree-SHA512: e03a684ccfcc5fbcad7f8a4899945a05989b555175fdcaebdb113aff46b52b4ee7b467192748edf99c5c348a620f8e52ab98bed3f3fca88280a64dbca458fe8a
e1281f1bbd wallet: fix key parsing check for miniscript expressions in `ParseScript` (brunoerg)
Pull request description:
In `ParseScript`, when processing miniscript expressions, the way we check for key parsing error is wrong, the actual code is unreachable because we're checking it into `if (node)` (successful parsing) statement.
ACKs for top commit:
sipa:
utACK e1281f1bbd
RandyMcMillan:
utACK e1281f1bbd
achow101:
ACK e1281f1bbd
Tree-SHA512: c4b3765d32673928a1f6d84ecbaa311870da9a9625753ed15ea57c802a9f16ddafa48c1dc66c0e4be284c5862e7821ed94135498ed9b9f3d7342a080035da289
cd810075ed fuzz: coinselection, improve `min_viable_change`/`change_output_size` (brunoerg)
Pull request description:
Instead of "randomly" fuzzing `min_viable_change` and `change_output_size`, and since they're correlated, this PR changes the approach to fuzz them according to the logic in `CreateTransactionInternal`.
ACKs for top commit:
murchandamus:
ACK cd810075ed
achow101:
ACK cd810075ed
furszy:
Code ACK cd810075ed
Tree-SHA512: 4539b469f00cdf666078d80c07ed062726f804e390400348148cd3092db9cdc178c6d00ead39aef19acf97badfb6576ce23546d8967387e81c5398d52d7f4404
19bb65bf25 [doc]: add doxygen return comment for CheckPackageLimits (ismaelsadeeq)
Pull request description:
This PR adds a doxygen comment on `CheckPackageLimits` describing what the method returns.
Fixes https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1429805433
ACKs for top commit:
Sjors:
utACK 19bb65bf25
Zero-1729:
utACK 19bb65bf25
Tree-SHA512: ccf1cc00a44d3fff60f28ad6766019a9f61b349729eab3cb02bc76b13c2e55441348a1602d806e60e4b2eabeb1f5d1ddacddf86c0bcdb78b078bb3a863b650c2
8dec9c560b wallet, mempool: propagete `checkChainLimits` error message to wallet (ismaelsadeeq)
Pull request description:
* Requested in [#28391 comment](https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382997719)
* The error message is static when a new transaction is created and package limit is reached.
`Transaction has too long of a mempool chain`
While the [`CTxMempool::CheckPackageLimits`](5800c558eb/src/txmempool.cpp (L199)) provide explicit information about the error message.
* This PR updates [`CTxMempool::CheckPackageLimits`](5800c558eb/src/txmempool.cpp (L199)) return type to `util::Result<void>`, `CheckPackageLimits` now returns void when package limit is not hit, and returns the error string whenever package limit is hit instead of using out parameter `errString`.
* The PR updates [`checkChainLimits`](5800c558eb/src/node/interfaces.cpp (L703)) return type to `util::Result<void>`.
* Now the wallet `CreateTransactionInternal` will have access to the package limit error string whenever its hit.
* Also Updated functional test to reflect the error message from `CTxMempool::CheckPackageLimits` output.
ACKs for top commit:
glozow:
utACK 8dec9c560b
Sjors:
utACK 8dec9c560b
TheCharlatan:
Re-ACK 8dec9c560b
Tree-SHA512: ddeac18aeba6f8e3be0e3fe76bf3db655352e3b415169f1f83ea1e8976a2f3e3de021c8da6880eb8382ab52d545e418e3f4d57adcc68ecb4f390339710ee6f30
b2fc7a2eda [fuzz] Improve fuzzing stability for minisketch harness (dergoegge)
Pull request description:
The `minisketch` harness has low stability due to:
* Rng internal to minisketch
* Benchmarkning for the best minisketch impl
Fix this by seeding the rng and letting the fuzzer choose the impl.
Also see #29018.
ACKs for top commit:
maflcko:
review ACK b2fc7a2eda
Tree-SHA512: 3d81414299c6803c34e928a53bcf843722fa8c38e1d3676cde7fa80923f9058b1ad4b9a2941f718303a6641b17eeb28b4a22eda09678102e9fb7c4e31d06f8f2
Update CheckPackageLimits to use util::Result to pass the error message
instead of out parameter.
Also update test to reflect the error message from `CTxMempool`
`CheckPackageLimits` output.
7b45744df3 tests: ensure functional tests set permitbaremultisig=1 when needed (Anthony Towns)
7dfabdcf86 tests: test both settings for permitbaremultisig in p2sh tests (Anthony Towns)
Pull request description:
Update unit and functional tests so that they continue to work if the default for `-permitbaremultisig` is changed.
ACKs for top commit:
maflcko:
lgtm ACK 7b45744df3
instagibbs:
crACK 7b45744df3
ajtowns:
> crACK [7b45744](7b45744df3)
achow101:
ACK 7b45744df3
glozow:
ACK 7b45744df3, changed default locally and all tests passed
Tree-SHA512: f89f9e2bb11f07662cfd57390196df9e531064e1bd662e1db7dcfc97694394ae5e8014e9d209b9405aa09195bf46fc331b7fba10378065cdb270cbd0669ae904
6666713041 refactor: Rename fs::path::u8string() to fs::path::utf8string() (MarcoFalke)
856c88776f ArgsManager: return path by value from GetBlocksDirPath() (Vasil Dimov)
fa3d9304e8 refactor: Remove pre-C++20 fs code (MarcoFalke)
fa00098e1a Add tests for C++20 std::u8string (MarcoFalke)
fa2bac08c2 refactor: Avoid copy/move in fs.h (MarcoFalke)
faea30227b refactor: Use C++20 std::chrono::days (MarcoFalke)
Pull request description:
This:
* Removes dead code.
* Avoids unused copies in some places.
* Adds copies in other places for safety.
ACKs for top commit:
achow101:
ACK 6666713041
ryanofsky:
Code review ACK 6666713041. Just documentation change since last review.
stickies-v:
re-ACK 6666713041
Tree-SHA512: 6176e44f30b310d51632ec2d3827c3819905d0ddc6a4b57acfcb6cfa1f9735176da75ee8ed4a4abd1296cb0b83bee9374cc6f91ffac87c19b63c435eeadf3f46
1ce45baed7 rpc: getwalletinfo, return wallet 'birthtime' (furszy)
83c66444d0 test: coverage for wallet birth time interaction with -reindex (furszy)
6f497377aa wallet: fix legacy spkm default birth time (furszy)
75fbf444c1 wallet: birth time update during tx scanning (furszy)
b4306e3c8d refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy)
Pull request description:
Fixing #28897.
As the user may have imported a descriptor with a timestamp newer
than the actual birth time of the first key (by setting 'timestamp=now'),
the wallet needs to update the birth time when it detects a transaction
older than the oldest descriptor timestamp.
Testing Notes:
Can cherry-pick the test commit on top of master. It will fail there.
ACKs for top commit:
Sjors:
re-utACK 1ce45baed7
achow101:
ACK 1ce45baed7
Tree-SHA512: 10c2382f87356ae9ea3fcb637d7edc5ed0e51e13cc2729c314c9ffb57c684b9ac3c4b757b85810c0a674020b7287c43d3be8273bcf75e2aff0cc1c037f1159f9
6db04be102 Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly (Ryan Ofsky)
213542b625 refactor: Add InitContext function to initialize NodeContext with global pointers (Ryan Ofsky)
feeb7b816a refactor: Remove calls to StartShutdown from KernelNotifications (Ryan Ofsky)
6824eecaf1 refactor: Remove call to StartShutdown from stop RPC (Ryan Ofsky)
1d92d89edb util: Get rid of uncaught exceptions thrown by SignalInterrupt class (Ryan Ofsky)
ba93966368 refactor: Remove call to ShutdownRequested from IndexWaitSynced (Ryan Ofsky)
42e5829d97 refactor: Remove call to ShutdownRequested from HTTPRequest (Ryan Ofsky)
73133c36aa refactor: Add NodeContext::shutdown member (Ryan Ofsky)
f4a8bd6e2f refactor: Remove call to StartShutdown from qt (Ryan Ofsky)
f0c73c1336 refactor: Remove call to ShutdownRequested from rpc/mining (Ryan Ofsky)
263b23f008 refactor: Remove call to ShutdownRequested from chainstate init (Ryan Ofsky)
Pull request description:
This change drops `shutdown.h` and `shutdown.cpp` files, replacing them with a `NodeContext::shutdown` member which is used to trigger shutdowns directly. This gets rid of an unnecessary layer of indirection, and allows getting rid of the `kernel::g_context` global.
Additionally, this PR tries to improve error handling of `SignalInterrupt` code by marking relevant methods `[[nodiscard]]` to avoid the possibility of uncaught exceptions mentioned https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.
Behavior is changing In a few cases which are noted in individual commit messages. Particularly: GUI code more consistently interrupts RPCs when it is shutting down, shutdown state no longer persists between unit tests, the stop RPC now returns an RPC error if requesting shutdown fails instead of aborting, and other failed shutdown calls now log errors instead of aborting.
This PR is a net reduction in lines of code, but in some cases the explicit error handling and lack of global shutdown functions do make it more verbose. The verbosity can be seen as good thing if it discourages more code from directly triggering shutdowns, and instead encourages code to return errors or send notifications that could be translated into shutdowns. Probably a number of existing shutdown calls could just be replaced by better error handling.
ACKs for top commit:
achow101:
ACK 6db04be102
TheCharlatan:
Re-ACK 6db04be102
maflcko:
ACK 6db04be102👗
stickies-v:
re-ACK 6db04be102
Tree-SHA512: 7a34cb69085f37e813c43bdaded1a0cbf6c53bd95fdde96f0cb45346127fc934604c43bccd3328231ca2f1faf712a7418d047ceabd22ef2dca3c32ebb659e634
98afe78661 doc: Update bitcoin-tx replaceable documentation (Kashif Smith)
94feaf2b66 tests: Add unit tests for bitcoin-tx replaceable command (Kashif Smith)
c2b836b119 bitcoin-tx: Make replaceable value optional (Kashif Smith)
Pull request description:
This fixes#28638. The issue was originally raised by dooglus, who also suggested the patch found in this code. Additionally, test coverage has been added and documentation has been updated.
ACKs for top commit:
achow101:
ACK 98afe78661
pinheadmz:
ACK 98afe78661
hernanmarino:
Tested ACK 98afe78661
instagibbs:
untested ACK 98afe78661
Tree-SHA512: ea1384aba7b0014c8cbeb7280d66b1e617d406fb02471dff33873057132b80518c94c7caa4b0426c26d17ce8aa393107de319dde781ace8df72f0314c8c75159
37c75c5820 test: wallet, fix change position out of range error (furszy)
Pull request description:
Fixes#29061. Only the benchmark is affected.
Since #25273, the behavior of 'inserting change at a random position'
is instructed by passing ´std::nullopt´ instead of -1.
Also, added missing documentation about the meaning of
'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'
ACKs for top commit:
achow101:
ACK 37c75c5820
kevkevinpal:
ACK [37c75c5](37c75c5820)
BrandonOdiwuor:
ACK 37c75c5820
Tree-SHA512: d9a8d8533540455716a5090fcf407573cad9f0d0018a05f903f89e51620302f9b256318db6f7338b85c047f7fab372d724e916b1721d7ed302dbf3d845b08734
fa3da629a1 Remove DirIsWritable, GetUniquePath (MarcoFalke)
fad3a9793b Return LockResult::ErrorWrite in LockDirectory (MarcoFalke)
fa0afe7408 refactor: Return enum in LockDirectory (MarcoFalke)
Pull request description:
`GetUniquePath` is only used in tests and in `DirIsWritable`. The check by `DirIsWritable` is redundant with the check done in `LockDirectory`.
Fix the redundancy by removing everything, except `LockDirectory`.
ACKs for top commit:
TheCharlatan:
Re-ACK fa3da629a1
hebasto:
ACK fa3da629a1, I have reviewed the code and it looks OK.
Tree-SHA512: e95f18cd586de7582e9c08ac7ddb860bfcfcbc8963804f45c5784c5e4c0598dc59ae7e45dd4daf30a5020dbf8433f5db2ad06e46a8676371982003790043c6c9
Since #25273, the behavior of 'inserting change at a random
position' is instructed by passing std::nullopt instead of -1.
Also, added missing documentation about the meaning of
'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'
576bee88fd fuzz: disable BnB when SFFO is enabled (furszy)
05e5ff194c test: add coverage for BnB-SFFO restriction (furszy)
0c5755761c wallet: create tx, log resulting coin selection info (furszy)
5cea25ba79 wallet: skip BnB when SFFO is active (Murch)
Pull request description:
Solves #28918. Coming from https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1838626406 discussion.
The intention is to decouple only the bugfix relevant commits from #28985, allowing them to be included in the 26.x release. This way, we can avoid disabling the coin selection fuzzing test for an entire release.
Note:
Have introduced few changes to the bug fix commit so that the unit tests pass without the additional burden introduced in #28985.
ACKs for top commit:
josibake:
ACK 576bee88fd
murchandamus:
ACK 576bee88fd
achow101:
ACK 576bee88fd
Tree-SHA512: f5d90eb3f3f524265afe4719495c9bf30f98b9af26cf039f7df5a7db977abae72caa7a3478cdd0ab10cd143bc1662e8fc5286b5bc10fc10f0dd582a45b45c31a
bd7f5d33e3 wallet: Assert that the wallet is not initialized in LoadWallet (Andrew Chow)
fb0b6ca4e5 tests, bench: Remove incorrect LoadWallet() calls (Andrew Chow)
Pull request description:
`CWallet::LoadWallet()` expects to be called after a `CWallet` is constructed, but before any of its member functions called. Doing so invalidates pointers which causes issues with some PRs and branches that I am working on. This was being used incorrectly in a few tests and benchmarks, resulting in segfaults.
As a precaution for this kind of issue in the future, I've also added a few asserts to `LoadWallet()` so that developers will notice when it is used incorrectly.
As similar issue was fixed in #27666
ACKs for top commit:
S3RK:
ACK bd7f5d33e3
furszy:
ACK bd7f5d33
Tree-SHA512: 7664f12b8452994e7fc4d7d4f77697fb5f75edb0dba95ba99a4a23ec03d5b8e0ecbdcb7635547a0e8b4f89f708f98dcb5d039df0559e24b1ae411ed630e16e14
Verify the transaction creation process does not produce
a BnB solution when SFFO is enabled.
This is currently problematic because it could require a
change output. And BnB is specialized on changeless solutions.
Co-authored-by: Andrew Chow <achow101@gmail.com>
Co-authored-by: Murch <murch@murch.one>
LoadWallet() must only be called immediately after a CWallet is
constructed, or not at all. Doing so after any other CWallet member
functions have been called may cause pointers and other objects
setup by other those functions to become invalidated.
Since these tests and benchmarks are using completely new wallets with
mock databases, it's not necessary to call LoadWallet() anyways, so
these can be dropped.
`ArgsManager::m_cached_blocks_path` is protected by
`ArgsManager::cs_args` and returning a reference to it after releasing
the mutex is unsafe.
To resolve this, return a copy of the path. This has some performance
penalty which is presumably ok, given that paths are a few 100s bytes
at most and `GetBlocksDirPath()` is not called often.
This silences the following (clang 18):
```
common/args.cpp:288:31: error: returning variable 'm_cached_blocks_path' by reference requires holding mutex 'cs_args' [-Werror,-Wthread-safety-reference-return]
288 | if (!path.empty()) return path;
| ^
```
Do the same with
`ArgsManager::GetDataDir()`,
`ArgsManager::GetDataDirBase()` and
`ArgsManager::GetDataDirNet()`.
Treating std::string as UTF-8 is deprecated in std::filesystem::path
since C++20.
However, it makes this codebase easier to read and maintain to retain
the ability for std::string to hold UTF-8.
fa8adbe7c1 build: Enable -Wunreachable-code (MarcoFalke)
Pull request description:
It seems a bit confusing to write code after a `return`. This can even lead to bugs, or incorrect code, such as https://github.com/bitcoin/bitcoin/pull/28830/files#r1415372320 . (Edit: The linked instance is not found by clang's `-Wunreachable-code`).
Fix all issues by enabling `-Wunreachable-code`.
This flag also enables `-Wunreachable-code-loop-increment`, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code, so remove that.
ACKs for top commit:
ajtowns:
> ACK [fa8adbe](fa8adbe7c1)
stickies-v:
ACK fa8adbe7c1
jonatack:
ACK fa8adbe7c1 tested with arm64 clang 17.0.6
Tree-SHA512: 12a2f74b69ae002e62ae08038f7458837090a12051a4c154d05ae4bb26fb19fc1fa76c63aedf2b3fbb36f048c593ca3b8c0efe03fe93cf07a0fd114fc84ce1e7
