For JSON-RPC 2.0 requests we need to distinguish between
a missing "id" field and "id":null. This is accomplished
by making the JSONRPCRequest id property a
std::optional<UniValue> with a default value of
UniValue::VNULL.
A side-effect of this change for non-2.0 requests is that request which do not
specify an "id" field will no longer return "id": null in the response.
Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the
RPC method failed but the HTTP request was otherwise valid. Batch requests
already did not return HTTP errors previously.
Refactors the helper functions in the test to provide more
fine-grained control over RPC requests and responses than
the usual AuthProxy methods.
No change in test behavior, the same RPC requests are made.
b335710782 depends: patch around non-determinism in qt (fanquake)
e8ecec4575 build: rename native_clang to native_llvm (fanquake)
b0c290340c Revert "build: Patch Qt to handle minimum macOS version properly" (fanquake)
558250dec1 guix: use clang-toolchain-17 for macOS build (fanquake)
5ddd7c65b4 build: Bump `native_clang` up to 17.0.6 (Hennadii Stepanov)
Pull request description:
This is the last step before #21778. We need LLVM 17.x so that lld has `-fixup_chains`.
ACKs for top commit:
theuni:
ACK b335710782.
TheCharlatan:
ACK b335710782
Tree-SHA512: 8142956196a481178f360258c2e4d924178d552966b713323f29f2deba7e5ec73a3da1c9d79d97c9e7f6aa18ed7429cd6660826aa633e6dde1ac56000b9ad57f
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
ff896d2581 contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check (fanquake)
Pull request description:
Reflect the actual symbols used, i.e:
```bash
bitcoind: symbol __bswapsi2 from unsupported version GCC_4.3.0(7)
```
ACKs for top commit:
TheCharlatan:
ACK ff896d2581
Tree-SHA512: b38ff8f4dd78d2d1c9063c53544dc4f240c3043f142e1581f7ba42f088a509293f6f17cc402c60ac82bff3b36668866b87e0e9e4d10d929484bb4c7a3e654f25
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
43c3246af7 ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid failures (Hennadii Stepanov)
Pull request description:
Homebrew attempts to check for outdated dependents or those with broken linkage. Such behavior might lead to failures when Homebrew updates them on old macOS images. For example, https://github.com/bitcoin/bitcoin/actions/runs/7199058794/job/19609891263 using the macOS image version `20231025.2`.
This PR prevents such behavior.
ACKs for top commit:
maflcko:
lgtm ACK 43c3246af7
ismaelsadeeq:
re ACK 43c3246af7
Tree-SHA512: cbe3cef5adf3f00eb618ba17aad3dc76c0c5d11142122a26b93619ae47dc50771e9e095caa898213325ed6ff41c07119429c0a9094bb98ead5601855d07bb2ea
Homebrew attempts to check for outdated dependents or those with broken
linkage. Such behavior might lead to failures when Homebrew updates them
on old macOS images.
This change prevents such behavior.
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
49a90915aa build: Bump minimum required Boost to 1.73.0 to support C++20 (Hennadii Stepanov)
Pull request description:
Boost versions <1.73 have C++20-specific bugs that were fixed in the following commits:
- 15fcf21356
- 495c095dc0
I tested [`libboost1.71-dev`](https://packages.ubuntu.com/focal/libboost1.71-dev) in Ubuntu 20.04 and Boost 1.71, 1.72, 1.73 in our depends build system.
Closes https://github.com/bitcoin/bitcoin/issues/29063.
ACKs for top commit:
fanquake:
ACK 49a90915aa
Tree-SHA512: b8ebc08af85abfa3fda70961bd1136ee9e5149dd76a3f901e43acba624d231971873cba5cbf30837f9e5ab58790b8330f241a76cb76d8cf5dce5ad0cca33fba8
