aboutsummaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2024-09-12gui: fix crash when closing walletfurszy
The crash occurs because 'WalletController::removeAndDeleteWallet' is called twice for the same wallet model: first in the GUI's button connected function 'WalletController::closeWallet', and then again when the backend emits the 'WalletModel::unload' signal. This causes the issue because 'removeAndDeleteWallet' inlines an erase(std::remove()). So, if 'std::remove' returns an iterator to the end (indicating the element wasn't found because it was already erased), the subsequent call to 'erase' leads to an undefined behavior.
2024-09-12Merge bitcoin/bitcoin#30546: util: Use consteval checked format string in ↵Ryan Ofsky
FatalErrorf, LogConnectFailure fa5bc450d5d4c1d2daf7b205f1678402c3c21678 util: Use compile-time check for LogConnectFailure (MarcoFalke) fa7087b896c0150c29d7a27c53e0533831a2bf3b util: Use compile-time check for FatalErrorf (MarcoFalke) faa62c0112f2b7ab69c80a5178f5b79217bed0a6 util: Add ConstevalFormatString (MarcoFalke) fae7b83eb58d22ed83878561603991131372cdd7 lint: Remove forbidden functions from lint-format-strings.py (MarcoFalke) Pull request description: The `test/lint/lint-format-strings.py` was designed to count the number of format specifiers and assert that they are equal to the number of parameters passed to the format function. The goal seems reasonable, but the implementation has many problems: * It is written in Python, meaning that C++ code can not be parsed correctly. Currently it relies on brittle regex and string parsing. * Apart from the parsing errors, there are also many logic errors. For example, `count_format_specifiers` allows a mix of positional specifiers and non-positional specifiers, which can lead to runtime format bugs. Also, `count_format_specifiers` silently skipped over "special" format specifiers, which are valid in tinyformat, which again can lead to runtime format bugs being undetected. * The brittle logic has a history of breaking in pull requests that are otherwise fine. This causes the CI to fail and the pull request being blocked from progress until the bug in the linter is fixed, or the code is rewritten to work around the bug. * It is only run in the CI, or when the developer invokes the script. It would be better if the developer got the error message at compile-time, directly when writing the code. Fix all issues by using a `consteval` checked format string in `FatalErrorf` and `LogConnectFailure`. This is the first step toward https://github.com/bitcoin/bitcoin/issues/30530 and a follow-up will apply the approach to the other places. ACKs for top commit: stickies-v: re-ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678 l0rinc: ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678 hodlinator: ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678 ryanofsky: Code review ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678 Tree-SHA512: d6189096b16083143687ed1b1559cf4f92f97dd87bc5d00673e44f4fb9fce7bb7b215cfdfc39b6e6a24f0b75a79a03ededce966639e554f7172e1fc22cf015ae
2024-09-12Merge bitcoin/bitcoin#30618: test: support std::optional in BOOST_CHECK_* ↵Ryan Ofsky
and increase FromUserHex fuzz feature coverage 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa Compare FromUserHex result against other hex validators and parsers (Lőrinc) 19947863e16425e6a119e7a4819867292b1235ee Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160 (Lőrinc) 743ac30e349e181c26a2d2af0bcb93b0835ce521 Add std::optional support to Boost's equality check (Lőrinc) Pull request description: Enhanced `FromUserHex` coverage by: * Added `std::optional` support to `BOOST_CHECK_EQUAL`, allowing direct comparisons of `std::optional<T>` with other `T` expected values. * Increased fuzz testing for hex parsing to validate against other hex validators and parsers. ---- * Use BOOST_CHECK_EQUAL for https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706637780 arith_uint256, uint256, uint160 Example error before: > unknown location:0: fatal error: in "validation_chainstatemanager_tests/chainstatemanager_args": std::bad_optional_access: bad_optional_access test/validation_chainstatemanager_tests.cpp:781: last checkpoint after: > test/validation_chainstatemanager_tests.cpp:801: error: in "validation_chainstatemanager_tests/chainstatemanager_args": check set_opts({"-assumevalid=0"}).assumed_valid_block == uint256::ZERO has failed [std::nullopt != 0000000000000000000000000000000000000000000000000000000000000000] ACKs for top commit: stickies-v: re-ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa ryanofsky: Code review ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa. Only changes since last review were auto type and fuzz test tweaks. hodlinator: ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa Tree-SHA512: f1d2c65f0ee4e97830700be5b330189207b11ed0c89a8cebf0f97d43308402a6b3732e10130c79a0c044f7d2eeabfb5359990825aadf02c4ec19428dcd982b00
2024-09-12Merge bitcoin/bitcoin#30814: kernel: Create usable static kernel librarymerge-script
0dd16d7118f10ac291a45644769121cbdfd2352f build: Add a pkg-config file for libbitcoinkernel (TheCharlatan) 45be32f8384398ad8d590137d05a6373aa827b75 build: Produce a usable static kernel library (TheCharlatan) Pull request description: Since the move to cmake, the kernel static library that is installed after a cmake --install build is unusable. It lacks symbols for the internal libraries, besides those defined in the kernel library target. Fix this by explicitly installing all the required internal static libraries. To make usage of these installed libraries easy, add a pkg-config file that can be used during linking. This patch can be tested with: ``` cmake -B build -DBUILD_SHARED_LIBS=OFF -DBUILD_KERNEL_LIB=ON cmake --build build cmake --install build g++ -std=c++20 -o test_chainstate src/bitcoin-chainstate.cpp -I/home/drgrid/bitcoin/src $(pkg-config --libs --static libbitcoinkernel) ``` Attempts to solve #30801 ACKs for top commit: hebasto: ACK 0dd16d7118f10ac291a45644769121cbdfd2352f. fanquake: ACK 0dd16d7118f10ac291a45644769121cbdfd2352f - this looks like a good place to start. ryanofsky: Code review ACK 0dd16d7118f10ac291a45644769121cbdfd2352f Tree-SHA512: 92f7bc959584bdc595f4aa6d0ab133355481075fe8564224fd7ac122fd7bdd75f98cf26ef0a6a7d84fd552d2258ddca1b674eca91122469a58bacc5f0a0ec2ef
2024-09-12build: Skip secp256k1 ctime tests when tests are not being builtHennadii Stepanov
Co-authored-by: fanquake <fanquake@gmail.com>
2024-09-12util: Use compile-time check for LogConnectFailureMarcoFalke
2024-09-12util: Use compile-time check for FatalErrorfMarcoFalke
2024-09-12util: Add ConstevalFormatStringMarcoFalke
The type is used to wrap a format string once it has been compile-time checked to contain the right number of format specifiers.
2024-09-12Merge bitcoin/bitcoin#30847: test: Drop no longer needed workaroundsmerge-script
5c80192ff6b982ee3a75be4142fe942b8206f2cd test: Drop no longer needed workarounds (Hennadii Stepanov) Pull request description: This PR deletes the workarounds introduced in https://github.com/bitcoin/bitcoin/pull/16564 and https://github.com/bitcoin/bitcoin/pull/15382, as `ctest` skips these cases gracefully: https://github.com/bitcoin/bitcoin/blob/5c80192ff6b982ee3a75be4142fe942b8206f2cd/src/test/CMakeLists.txt#L201-L203 ACKs for top commit: kevkevinpal: ACK [5c80192](https://github.com/bitcoin/bitcoin/pull/30847/commits/5c80192ff6b982ee3a75be4142fe942b8206f2cd) fanquake: ACK 5c80192ff6b982ee3a75be4142fe942b8206f2cd. Looks correct: Tree-SHA512: c47c606ecf7d64016b3c6353c3d4898350edc2caeac494dfd44484417f500a73f0c88c39f0f24651f3a02ef31ed9ca5c70d938bb9a8ca1eea54927e4d6a8fcd2
2024-09-12Merge bitcoin/bitcoin#30835: build: Introduce "Kernel" installation componentmerge-script
7b04fabe2d95f05a295b1ef30c9aeab7f753ed46 build: Introduce "Kernel" installation component (Hennadii Stepanov) Pull request description: This PR enables building and installing only `libbitcoinkernel`, without the need to disable other targets during the project build system generation: ``` $ rm -rf build && cmake -B build -DBUILD_KERNEL_LIB=ON $ cmake --build build --target bitcoinkernel $ cmake --install build --component Kernel --prefix /home/hebasto/INSTALL -- Install configuration: "RelWithDebInfo" -- Installing: /home/hebasto/INSTALL/lib/libbitcoinkernel.so ``` Please note, that only the `bitcoinkernel` target is being built. Related to https://github.com/bitcoin/bitcoin/issues/30801 and https://github.com/bitcoin/bitcoin/pull/30814. ACKs for top commit: TheCharlatan: ACK 7b04fabe2d95f05a295b1ef30c9aeab7f753ed46 ryanofsky: Code review ACK 7b04fabe2d95f05a295b1ef30c9aeab7f753ed46 Tree-SHA512: eac114dde059e47c91938a4a9108fc0fc693b5342ed3b6ecb971615be8ad3225b9985aae12d6ad18e673edf1bd39a5ecf259c1b61734f221669091bf2ce93a67
2024-09-12Merge bitcoin/bitcoin#30803: build: Minor build system fixes and amendmentsmerge-script
1cc93fe7b40f10a7d1d1189058af98a2bce31381 build: Delete dead code that implements `IF_CHECK_FAILED` option (Hennadii Stepanov) 341ad238091d4df520c70f1757b017e6f6620f24 build: Delete MSVC special case for `BUILD_FOR_FUZZING` option (Hennadii Stepanov) fdad128b528bc8622bc6d8343026c28b18260f64 build: Stop enabling CMake's CMP0141 policy (Hennadii Stepanov) b2a6f545b4f6e3442ae51f66a6f3c1de92d00a1b doc: Drop `ctest` command from Windows cross-compiling instructions (Hennadii Stepanov) 73b618582dcf06dd01be062fe0f81060cfcb48d8 build: Print `CMAKE_CXX_COMPILER_ARG1` in summary (Hennadii Stepanov) f03c9420958de31fdfecec5fa3e23134aac61803 build, test: Add missed log options (Hennadii Stepanov) 6f2cb0eafdef81fb9464a4679c3a5905d19e5103 doc: Amend comment about ZeroMQ config files (Hennadii Stepanov) Pull request description: This PR addresses the following comments: - https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742342524 - https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1728692369 - https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1736110362 - https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742931121 - https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1747723657 - https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742328675 - https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1723106474 ACKs for top commit: sipsorcery: tACK 1cc93fe7b40f10a7d1d1189058af98a2bce31381 (win11 msvc). maflcko: re-ACK 1cc93fe7b40f10a7d1d1189058af98a2bce31381 Tree-SHA512: a390797bb4d3b7eb9163653b6c9c324e7a01090f6cdda74df7349a24a5c4a2084e5912878747f56561315afc70cae9adb1c363f47ceb0af96004ea591d25171b
2024-09-11Merge bitcoin/bitcoin#30807: Fix peers abruptly disconnecting from ↵Ava Chow
AssumeUTXO nodes during IBD 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac test: add coverage for assumeUTXO honest peers disconnection (furszy) 6d5812e5c852c233bd7ead2ceef051f8567619ed assumeUTXO: fix peers disconnection during sync (furszy) Pull request description: Because AssumeUTXO nodes prioritize tip synchronization, they relay their local address through the network before completing the background chain sync. This, combined with the advertising of full-node service (`NODE_NETWORK`), can result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing) and requesting an historical block the node does not have. This behavior leads to an abrupt disconnection due to perceived unresponsiveness from the AssumeUTXO node. This lack of response occurs because nodes ignore `getdata` requests when they do not have the block data available (further discussion can be found in #30385). Fix this by refraining from signaling full-node service support while the background chain is being synced. During this period, the node will only signal `NODE_NETWORK_LIMITED` support. Then, full-node (`NODE_NETWORK`) support will be re-enabled once the background chain sync is completed. Thanks mzumsande for a post-#30385 convo too. Testing notes: Just cherry-pick the second commit (bb08c22) on master. It will fail there, due to the IBD node requesting historical blocks to the snapshot node - which is bad because the snapshot node will ignore the requests and stall + disconnect after some time. ACKs for top commit: achow101: ACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac naumenkogs: ACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac mzumsande: ACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac Tree-SHA512: fef525d1cf3200c2dd89a346be9c82d77f2e28ddaaea1f490a435e180d1a47a371cadea508349777d740ab56e94be536ad8f7d61cc81f6550c58b609b3779ed3
2024-09-11Compare FromUserHex result against other hex validators and parsersLőrinc
2024-09-11Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160Lőrinc
Example error before: > unknown location:0: fatal error: in "validation_chainstatemanager_tests/chainstatemanager_args": std::bad_optional_access: bad_optional_access test/validation_chainstatemanager_tests.cpp:781: last checkpoint after: > test/validation_chainstatemanager_tests.cpp:801: error: in "validation_chainstatemanager_tests/chainstatemanager_args": check set_opts({"-assumevalid=0"}).assumed_valid_block == uint256::ZERO has failed [std::nullopt != 0000000000000000000000000000000000000000000000000000000000000000] Also added extra minimum_chainwork test to make it symmetric with assumevalid Co-authored-by: Ryan Ofsky <ryan@ofsky.org> Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
2024-09-10assumeUTXO: fix peers disconnection during syncfurszy
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local address through the network before completing the background chain sync. This, combined with the advertising of full-node service (NODE_NETWORK), can result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing) and requesting an historical block the node does not have. This behavior leads to an abrupt disconnection due to perceived unresponsiveness (lack of response) from the AssumeUTXO node. This lack of response occurs because nodes ignore getdata requests when they do not have the block data available (further discussion can be found in PR 30385). Fix this by refraining from signaling full-node service support while the background chain is being synced. During this period, the node will only signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK') support will be re-enabled once the background chain sync is completed.
2024-09-10Merge bitcoin/bitcoin#30773: Remove unsafe uint256S() and test-only uint160S()Ryan Ofsky
43cd83b0c7ba436b8ffc83d8a65e935714dffe70 test: move uint256_tests/operator_with_self to arith_uint256_tests (stickies-v) c6c994cb2b9af58b1c0e554255d1a3be785e8d56 test: remove test-only uint160S (stickies-v) 62cc4656e2bb38f80485a13d75b42add09a6b731 test: remove test-only uint256S (stickies-v) adc00ad728dd54084671398f8fa5c12be92d2bab test: remove test-only arith_uint256S (stickies-v) f51b237723b87db706cbce2939d20eb193332799 refactor: rpc: use uint256::FromHex for ParseHashV (stickies-v) Pull request description: _Continuation of #30569._ Since https://github.com/bitcoin/bitcoin/commit/fad2991ba073de0bd1f12e42bf0fbaca4a265508, `uint256S()` has been [deprecated](https://github.com/bitcoin/bitcoin/pull/30482/commits/fad2991ba073de0bd1f12e42bf0fbaca4a265508#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in https://github.com/bitcoin/bitcoin/pull/30482. Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. (see also https://github.com/bitcoin/bitcoin/pull/30532) This PR removes `uint256S()` (and `uint160S()`) completely, with no non-test behaviour change. Specifically, the main changes in this PR are: - the (minimal) last non-test usage of `uint256S()` in `ParseHashV()` is removed without behaviour change, which can partially be verified by cherry-picking and/or modifying [this test commit](https://github.com/stickies-v/bitcoin/commit/1f2b0fa86db2ed65476b68417aa1bf4c26026a19)). - the test usage of `uint{160,256}S()` is removed, largely replacing it with `uint{160,256}::FromHex()` where applicable, potentially modifying the test by removing non-hex characters or dropping the test entirely if removing non-hex characters makes it redundant - the now unused `uint{160,256}S()` functions are removed completely. - unit test coverage on converting `uint256` <-> `arith_uint256` through `UintToArith256()` and `ArithToUint256()` is beefed up, and `arith_uint256` tests are moved to `arith_uint256_tests.cpp`, removing the `uint256_tests.cpp` dependency on `uint256h`, mirroring how the code is structured. _Note: `uint256::FromUserHex()` exists to more leniently construct uint256 from user input, allowing "0x" prefixes and too-short-input, as safer alternative to `uint256S()` where necessary._ ACKs for top commit: l0rinc: reACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70 hodlinator: re-ACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70 ryanofsky: Code review ACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70. Only code change is a small refactoring which looks good. The rest of the PR is all test changes, which I only lightly reviewed, but seem to be positive and do what's described Tree-SHA512: 48147a4c6af671597df0f72c1b477ae4631cd2cae4645ec54d0e327611ff302c9899e344518c81242cdde82930f6ad23a3a7e6e0b80671816e9f457b9de90a5c
2024-09-10Merge bitcoin/bitcoin#30065: init: fixes file descriptor accountingRyan Ofsky
d4c7c4009da14c84576d43ab4a1241967cfa5ffc init: error out if -maxconnections is negative (Sergi Delgado Segura) c7736494813ad11e4ad53789104e70a19819f412 init: improves file descriptors accounting and docs (Sergi Delgado Segura) 29008a7ff43cf712a8438663e79cebdf698efae9 init: fixes fd accounting regarding poll/select (Sergi Delgado Segura) Pull request description: The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR). Redefine some of the constants, plus document what are we accounting for so this can be extended more easily Fixes #18911 ACKs for top commit: sr-gi: > ACK [d4c7c40](https://github.com/bitcoin/bitcoin/commit/d4c7c4009da14c84576d43ab4a1241967cfa5ffc) naumenkogs: ACK d4c7c4009da14c84576d43ab4a1241967cfa5ffc vasild: ACK d4c7c4009da14c84576d43ab4a1241967cfa5ffc TheCharlatan: ACK d4c7c4009da14c84576d43ab4a1241967cfa5ffc Tree-SHA512: 1524d10c8ad8f354f6ab9c244699adbcdae2dd7aba37de5b8f9e177c629e8a2cce0f6e8117e076dde3a592f5283bd30a4201db96a3c011e335c02d1fde7414bc
2024-09-10Merge bitcoin/bitcoin#30791: build: Use correct variable namemerge-script
2d68c3b1c2e4f8fb881efc3569506d426ee5155d build: Use correct variables when passing `-fsanitize` to libsecp256k1 (Hennadii Stepanov) Pull request description: This was overlooked after https://github.com/bitcoin-core/secp256k1/pull/1546. Also see: - https://github.com/bitcoin-core/secp256k1/pull/1600 - https://github.com/bitcoin/bitcoin/pull/30845 - https://github.com/hebasto/oss-fuzz/pull/9 ACKs for top commit: fanquake: ACK 2d68c3b1c2e4f8fb881efc3569506d426ee5155d Tree-SHA512: 1a149e2072fd471c3af2f8591ccd69bddc8060eb04246c7f5596d179608fb097293c4c7b17f237fcf9014d8fc1ddc727497554fa9535777243ac989672ab1a75
2024-09-09Merge bitcoin/bitcoin#30509: multiprocess: Add -ipcbind option to bitcoin-nodeAva Chow
30073e6b3a24cbe417c45cd5df6a3a2de0251e9d multiprocess: Add -ipcbind option to bitcoin-node (Russell Yanofsky) 73fe7d723084653671f2178ea1177a8627edfafa multiprocess: Add unit tests for connect, serve, and listen functions (Ryan Ofsky) 955d4077aac621697246bcb20a854ba97e37c519 multiprocess: Add IPC connectAddress and listenAddress methods (Russell Yanofsky) 4da20434d4d68b7933e39aca36faa6fd2264cc45 depends: Update libmultiprocess library for CustomMessage function and ThreadContext bugfix (Ryan Ofsky) Pull request description: Add `-ipcbind` option to `bitcoin-node` to make it listen on a unix socket and accept connections from other processes. The default socket path is `<datadir>/node.sock`, but this can be customized. This option lets potential wallet, gui, index, and mining processes connect to the node and control it. See examples in #19460, #19461, and #30437. Motivation for this PR, in combination with #30510, is be able to release a bitcoin core node binary that can generate block templates for a separate Stratum v2 mining service, like the one being implemented in https://github.com/Sjors/bitcoin/pull/48, that connects over IPC. Other things to know about this PR: - While the `-ipcbind` option lets other processes to connect to the `bitcoin-node` process, the only thing they can actually do after connecting is call methods on the [`Init`](https://github.com/bitcoin/bitcoin/blob/master/src/ipc/capnp/init.capnp#L17-L20) interface which is currently very limited and doesn't do much. But PRs [#30510](https://github.com/bitcoin/bitcoin/pull/30510), [#29409](https://github.com/bitcoin/bitcoin/pull/29409), and [#10102](https://github.com/bitcoin/bitcoin/pull/10102) expand the `Init` interface to expose mining, wallet, and gui functionality respectively. - This PR is not needed for [#10102](https://github.com/bitcoin/bitcoin/pull/10102), which runs GUI, node, and wallet code in different processes, because [#10102](https://github.com/bitcoin/bitcoin/pull/10102) does not use unix sockets or allow outside processes to connect to existing processes. [#10102](https://github.com/bitcoin/bitcoin/pull/10102) lets parent and child processes communicate over internal socketpairs, not externally accessible sockets. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). ACKs for top commit: achow101: ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d TheCharlatan: Re-ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d itornaza: Code review ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d Tree-SHA512: 2b766e60535f57352e8afda9c3748a32acb5a57b2827371b48ba865fa9aa1df00f340732654f2e300c6823dbc6f3e14377fca87e4e959e613fe85a6d2312d9c8
2024-09-09Add std::optional support to Boost's equality checkLőrinc
Also moved the operators to the bottom of the file since they're less important and to group them together. Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> Co-authored-by: stickies-v <stickies-v@protonmail.com>
2024-09-09Merge bitcoin/bitcoin#30817: test: Add coverage for dumptxoutset failure ↵Ava Chow
robustness c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7 refactor: Manage dumptxoutset RAII classes with std::optional (Fabian Jahr) 4b5bf335adabd1586043caa72a98356a8255bc29 test: Add coverage for failing dumptxoutset behavior (Fabian Jahr) Pull request description: This adds a test that checks that network activity is not suspended if dumptxoutset fails in the middle of its process which is implemented with the `NetworkDisable` RAII class. I would have liked to add coverage for the `TemporaryRollback` RAII class but that seems a lot more tricky since the failure needs to happen at some point after the rollback and on the scale of our test chain here I couldn't find a way to do it yet. This was requested by pablomartin4btc here: https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2280450117. To test the test you can comment out the content of the destructor of `NetworkDisable`. It also addresses the feedback by ryanofsky to use `std::optional` instead of `std::unique_ptr` for the management of the RAII object: https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228 ACKs for top commit: achow101: ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7 pablomartin4btc: cr & tACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7 tdb3: ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7 BrandonOdiwuor: Code Review ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7 theStack: ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7 Tree-SHA512: 9556e75014a2599bb870b70faf887608b332f2312626333f771d4ec11c04f863a2cf17e223ec473d4e8b0c9e8008394a4e0c321561f7ef3a2eec713dcfaea58a
2024-09-09Merge bitcoin/bitcoin#30684: init: fix init fatal error on invalid negated ↵Ava Chow
option value ee47ca29d6ef55650a0af63bca817c5d494f31ef init: fix fatal error on '-wallet' negated option value (furszy) Pull request description: Currently, if users provide a double negated value such as '-nowallet=0' or a non-boolean convertible value to a negated option such as '-nowallet=not_a_boolean', the initialization process results in a fatal error, causing an unclean shutdown and displaying a poorly descriptive error message: "JSON value of type bool is not of expected type string." (On bitcoind. The GUI does not display any error msg - upcoming PR -). This PR fixes the issue by ensuring that only string values are returned in the the "wallet" settings list, failing otherwise. It also improves the clarity of the returned error message. Note: This bug was introduced in https://github.com/bitcoin/bitcoin/pull/22217. Where the `GetArgs("-wallet")` call was replaced by `GetSettingsList("-wallet")`. ACKs for top commit: achow101: ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef ryanofsky: Code review ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef, just adding the suggested test since last review TheCharlatan: ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef ismaelsadeeq: Tested ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef Tree-SHA512: 5f01076f74a048019bb70791160f0accc2db7a457d969cb23687bed81ccbbdec1dda68311e7c6e2dd56250e23e8d926d4066e5014b2a99a2fc202e24ed264fbd
2024-09-09Merge bitcoin/bitcoin#30401: fix: increase consistency of rpcauth parsingAva Chow
27c976d11a68d66db97d9a7a30c6a6a71c6ab586 fix: increase consistency of rpcauth parsing (tdb3) 2ad3689512a36eaff957df9ac28e65b2fedbc36c test: add norpcauth test (tdb3) 67df0dec1abe547773e532aa60aff0317e018123 test: blank rpcauth CLI interaction (tdb3) ecc98ccff25b7e758337e764e59d764076772fec test: add cases for blank rpcauth (tdb3) Pull request description: The current `rpcauth` parsing behavior is inconsistent and unintuitive (see https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-1972085251 and additional details below). The current behavior inconsistently treats empty `rpcauth` as an error (or not) depending on the location within CLI/bitcoin.conf and the location of adjacent valid `rpcauth` params. Empty `rpcauth` is now consistently treated as an error and prevents bitcoind from starting. Continuation of the upforgrabs PR #29141. ### Additional details: Current `rpcauth` behavior is nonsensical: - If an empty `rpcauth` argument was specified as the last command line argument, it would cause all other `rpcauth` arguments to be ignored. - If an empty `rpcauth` argument was specified on the command line followed by any nonempty `rpcauth` argument, it would cause an error. - If an empty `rpcauth=` line was specified after non-empty rpcauth line in the config file it would cause an error. - If an empty `rpcauth=` line in a config file was first it would cause other rpcauth entries in the config file to be ignored, unless there were `-rpcauth` command line arguments and the last one was nonempty, in which case it would cause an error. New behavior is simple: - If an empty rpcauth config line or command line argument is used it will cause an error ACKs for top commit: naiyoma: Tested ACK [https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586](https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586) achow101: ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586 ryanofsky: Code review ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586. Since last review commit message was just tweaked to clarify previous behavior. Tree-SHA512: af2e9dd60d1ad030409ae2c3805ab139c7435327823d9f8bbeede815f376cb696a5929b08a6e8c8b5f7278ed49cfb231789f9041bd57f1f03ec96501b669da5b
2024-09-09build: Use correct variables when passing `-fsanitize` to libsecp256k1Hennadii Stepanov
This was overlooked after https://github.com/bitcoin-core/secp256k1/pull/1546
2024-09-09Merge bitcoin/bitcoin#30845: Update libsecp256k1 subtree to latest mastermerge-script
611562806cf3fd3028e24e6c5a8e8dcb8805be38 Squashed 'src/secp256k1/' changes from 642c885b61..2f2ccc4695 (Hennadii Stepanov) Pull request description: This PR updates the libsecp256k1 subtree to https://github.com/bitcoin-core/secp256k1/commit/2f2ccc469540fde6495959cec061e95aab033148, which includes the following changes: - https://github.com/bitcoin-core/secp256k1/pull/1577 - https://github.com/bitcoin-core/secp256k1/pull/1578 - https://github.com/bitcoin-core/secp256k1/pull/1583 - https://github.com/bitcoin-core/secp256k1/pull/1586 - https://github.com/bitcoin-core/secp256k1/pull/1600 The latter is required for https://github.com/bitcoin/bitcoin/pull/30791. ACKs for top commit: l0rinc: utACK ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f real-or-random: utACK https://github.com/bitcoin/bitcoin/pull/30845/commits/ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f no blockers from libsecp256k1 side, and these commits touch only build/docs/tests and are thus particularly harmless fanquake: ACK ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f Tree-SHA512: 77cf1ba9aa24efdcf01e99850ea8bed54f847307a3c98c10289c9b7fd9e70c9219f28bee0f00ef7d69979d95a0ddac1e937d3d183ebc9d9b8e6cce0db1d507c9
2024-09-09Merge bitcoin/bitcoin#30823: cmake: add `USE_SOURCE_PERMISSIONS` to all ↵merge-script
`configure_file()` usage 1f054eca4e779cfa5f9f6e9278071adf65e5eafe cmake: add USE_SOURCE_PERMISSIONS to all configure_file usage (fanquake) Pull request description: `USE_SOURCE_PERMISSIONS` is the default, so this should not change behaviour. However, being explicit makes it clear what we are doing. Related to #30815. See https://cmake.org/cmake/help/latest/command/configure_file.html#options. ACKs for top commit: hebasto: ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe. TheCharlatan: ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe Tree-SHA512: efed91b8aa0813100304ee58e169bbf5cfbb7db465ec4f7e6cbbae6053f09a36757bf96b4d1cb9ddf4c1cab0ceb3ab18805ebefa122535518ffb501c9b489d3d
2024-09-08test: Drop no longer needed workaroundsHennadii Stepanov
`ctest` skips "no test cases matching filter" tests gracefully.
2024-09-07Update secp256k1 subtree to latest masterHennadii Stepanov
2024-09-07Squashed 'src/secp256k1/' changes from 642c885b61..2f2ccc4695Hennadii Stepanov
2f2ccc4695 Merge bitcoin-core/secp256k1#1600: cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable 421ed1b46f cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable 1988855079 Merge bitcoin-core/secp256k1#1586: fix: remove duplicate 'the' from header file comment b307614401 Merge bitcoin-core/secp256k1#1583: ci: Bump GCC_SNAPSHOT_MAJOR to 15 fa67b6752d refactor: Use array initialization for unterminated strings 9b0f37bff1 fix: remove duplicate 'the' from header file comment e34b476730 ci: Bump GCC_SNAPSHOT_MAJOR to 15 3fdf146bad Merge bitcoin-core/secp256k1#1578: ci: Silent Homebrew's noisy reinstall warnings f8c1b0e0e6 Merge bitcoin-core/secp256k1#1577: release cleanup: bump version after 0.5.1 7057d3c9af ci: Silent Homebrew's noisy reinstall warnings c3e40d75db release cleanup: bump version after 0.5.1 git-subtree-dir: src/secp256k1 git-subtree-split: 2f2ccc469540fde6495959cec061e95aab033148
2024-09-06build, test: Add missed log optionsHennadii Stepanov
2024-09-06build: Add a pkg-config file for libbitcoinkernelTheCharlatan
2024-09-06build: Produce a usable static kernel libraryTheCharlatan
Since the move to cmake, the kernel static library that is installed after a cmake --install build is unusable. It lacks symbols for the internal libraries, besides those defined in the kernel library target. This is because cmake, unlike the libtool archiver, does not combine multiple static libraries into a single static library on installation. This is likely an intentional design choice, since there were a bunch of caveats to the way libtool calculated these libraries. Fix this problem by installing all the required libraries. The user must then link all of them along with the bitcoin kernel library.
2024-09-06test: move uint256_tests/operator_with_self to arith_uint256_testsstickies-v
move/formatting-only change. These tests do not cover uint256, so move them to the appropriate test suite. Additionally, apply clang-format suggestions.
2024-09-06test: remove test-only uint160Sstickies-v
uint160S is a test-only function, and testing input that is not allowed in uint160::FromHex() is superfluous. Tests that can't use uint160::FromHex() because they use input with non-hex digit characters are a) modified by dropping the non-hex digit characters if that provides useful test coverage. b) dropped if the test without non-hex digit characters does not provide useful test coverage, e.g. because it is now duplicated.
2024-09-06test: remove test-only uint256Sstickies-v
uint256S was previously deprecated for being unsafe. All non-test usage has already been removed in earlier commits. 1. Tests now use uint256::FromHex() or other constructors wherever possible without further modification. 2. Tests that can't use uint256::FromHex() because they use input with non-hex digit characters are a) modified by dropping the non-hex digit characters if that provides useful test coverage. b) dropped if the test without non-hex digit characters does not provide useful test coverage, e.g. because it is now duplicated. Additionally, use BOOST_CHECK_EQUAL where relevant on touched lines to make error messages more readable.
2024-09-06test: remove test-only arith_uint256Sstickies-v
Tests that are solely testing constructing from a hex string are dropped, others are modified to use a uint256 constructor or the arith_uint256 uint64_t constructor. Since an arith_uint256 can not be constructed from a string directly, we need to ensure that test coverage on UintToArith256(uint256::FromHex()) is not reduced. uint256::FromHex() already has good test coverage, but the test coverage on UintToArith256() and ArithToUint256() is increased in this commit by upgrading the `conversion` test case. Moreover, since `uint256.h` does not have any dependencies on `arith_uint256.h`, the conversion tests are moved to `arith_uint256_tests.cpp` so the dependency can be cleaned up entirely in a future commit.
2024-09-06build: Introduce "Kernel" installation componentHennadii Stepanov
This change enables building and installing only `libbitcoinkernel`, without the need to disable other targets during the project build system generation.
2024-09-06test: Work around boost compilation errorMarcoFalke
2024-09-06Revert "build: work around issue with Boost <= 1.80 and Clang >= 18"MarcoFalke
This reverts commit cd062d6684908d526be7423f8f1057b891254a3c.
2024-09-06multiprocess: Add -ipcbind option to bitcoin-nodeRussell Yanofsky
Add `-ipcbind` option to `bitcoin-node` to listen on an IPC socket and accept connections from other processes. In the future, there will be an `-ipcconnect` option added to `bitcoin-wallet` and `bitcoin-node` to allow wallet and gui processes to connect to the node and access it. Example usage: src/bitcoin-node -regtest -debug -ipcbind=unix src/bitcoin-wallet -regtest -ipcconnect=unix info src/bitcoin-gui -regtest -ipcconnect=unix src/bitcoin-mine -regtest -ipcconnect=unix
2024-09-06multiprocess: Add unit tests for connect, serve, and listen functionsRyan Ofsky
2024-09-06multiprocess: Add IPC connectAddress and listenAddress methodsRussell Yanofsky
Allow listening on and connecting to unix sockets.
2024-09-06fuzz: Don't compile BDB-specific code on MSVC in `wallet_bdb_parser.cpp`Hennadii Stepanov
2024-09-06cmake: add USE_SOURCE_PERMISSIONS to all configure_file usagefanquake
`USE_SOURCE_PERMISSIONS` is the default, so this should not change behaviour. However, being explicit makes it clear what we are doing. Related to #30815. See https://cmake.org/cmake/help/latest/command/configure_file.html#options.
2024-09-06Merge bitcoin/bitcoin#30790: bench: Remove redundant logging benchmarksmerge-script
fadbcd51fc77a3f4e877851463f3c7425fb751d2 bench: Remove redundant logging benchmarks (MarcoFalke) fa8dd952e279a87f6027ddd2e2119bf2ae2f9943 bench: Use LogInfo instead of the deprecated alias LogPrintf (MarcoFalke) Pull request description: `LogPrint*ThreadNames` is redundant with `LogWith(out)ThreadNames`, because they all measure toggling the thread names (and check that it has no effect on performance). Fix it by removing the redundant ones. This also allows to drop a deprecated logging alias. ACKs for top commit: stickies-v: ACK fadbcd51fc77a3f4e877851463f3c7425fb751d2 Tree-SHA512: 4fe137f374aa4ee1aa0e1da4a1f9839c0e52c23dbb93198ecafee98de39d311cc47304bba4191f3807aa00c51b1eae543e3f270f03d341c84910e5e341a1d475
2024-09-06Merge bitcoin/bitcoin#30748: test: Pin and document TEST_DIR_PATH_ELEMENT, ↵merge-script
SeedRand::FIXED_SEED fa84f9decd224ea1c25dc7095bad70a48fa1a534 test: Pin and document TEST_DIR_PATH_ELEMENT (MarcoFalke) 2222f7a87404078984c7189768a3422deb114302 test: Rename SeedRand::SEED to FIXED_SEED for clarity (MarcoFalke) Pull request description: Two small test changes: * A refactor to update the name and documentation around `SeedRand::FIXED_SEED`. * A change to extract and document `TEST_DIR_PATH_ELEMENT`, and to change its value to better match the `TMPDIR_PREFIX` in functional tests. The value previously included `PACKAGE_NAME`, which is cute, but doesn't explain why it was used (to include a space). So just use `test_common bitcoin` to achieve the same with less effort. ACKs for top commit: hodlinator: ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534 ryanofsky: Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534 Tree-SHA512: eb35d6598bb08f9b996e3a4762d8f26b2441c0ca00780798e473015af735dfc9997120895a922b94d4b6ada45adadba4a686e9cf9c285ddf688848e764c64840
2024-09-05Merge bitcoin/bitcoin#30821: build: work around issue with Boost <= 1.80 and ↵Ava Chow
Clang >= 18 cd062d6684908d526be7423f8f1057b891254a3c build: work around issue with Boost <= 1.80 and Clang >= 18 (fanquake) Pull request description: Our current minimum supported Boost is `1.73.0`. However, when compiling with Boost `1.74.0` (Debian Stable), using Clang `18`, compilation fails with: ```bash In file included from /usr/include/boost/mpl/integral_c.hpp:32: /usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'udt_builtin_mixture_enum' [-Wenum-constexpr-conversion] 73 | typedef AUX_WRAPPER_INST( BOOST_MPL_AUX_STATIC_CAST(AUX_WRAPPER_VALUE_TYPE, (value - 1)) ) prior; | ^ /usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST' 24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr) | ^ In file included from ../../../src/test/validation_chainstatemanager_tests.cpp:8: In file included from ../../../src/node/chainstatemanager_args.h:9: In file included from ../../../src/validation.h:28: In file included from ../../../src/txmempool.h:26: In file included from /usr/include/boost/multi_index/hashed_index.hpp:38: In file included from /usr/include/boost/multi_index/detail/node_handle.hpp:22: In file included from /usr/include/boost/multi_index_container_fwd.hpp:18: In file included from /usr/include/boost/multi_index/indexed_by.hpp:17: In file included from /usr/include/boost/mpl/vector.hpp:36: In file included from /usr/include/boost/mpl/vector/vector20.hpp:18: In file included from /usr/include/boost/mpl/vector/vector10.hpp:18: In file included from /usr/include/boost/mpl/vector/vector0.hpp:24: In file included from /usr/include/boost/mpl/vector/aux_/clear.hpp:18: In file included from /usr/include/boost/mpl/vector/aux_/vector0.hpp:22: In file included from /usr/include/boost/mpl/vector/aux_/iterator.hpp:19: In file included from /usr/include/boost/mpl/plus.hpp:19: In file included from /usr/include/boost/mpl/aux_/arithmetic_op.hpp:17: In file included from /usr/include/boost/mpl/integral_c.hpp:32: /usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'int_float_mixture_enum' [-Wenum-constexpr-conversion] /usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST' 24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr) | ^ 2 errors generated. ``` Work around this issue by ignoring this diagnostic for this include. I did attempt to just downgrade the error into a warning, but that did not seem to work. Not a huge fan of inline warning/issue suppression, but this seems like the cleanest thing to do here (and easy to backport to `28.x`). Can be tested with something like: ```bash docker pull debian:bookworm docker run -it debian:bookworm /bin/bash apt update && apt install ccache cmake git pkg-config libboost-dev libevent-dev python3 libsqlite3-dev lsb-release wget software-properties-common gnupg git clone https://github.com/bitcoin/bitcoin wget https://apt.llvm.org/llvm.sh chmod +x llvm.sh ./llvm.sh 18 cd bitcoin cmake -B build -DCMAKE_C_COMPILER=clang-18 -DCMAKE_CXX_COMPILER=clang++-18 cmake --build build -j17 <snip> In file included from /usr/include/boost/mpl/integral_c.hpp:32: /usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'int_float_mixture_enum' [-Wenum-constexpr-conversion] /usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST' 24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr) | ^ 2 errors generated. Apply the patch cmake --build build -j17 ctest --test-dir build -j17 ``` Fixes #30751. ACKs for top commit: achow101: ACK cd062d6684908d526be7423f8f1057b891254a3c hebasto: ACK cd062d6684908d526be7423f8f1057b891254a3c, tested on Fedora 40 using the downloaded [Boost 1.74](https://archives.boost.io/release/1.74.0/source/) and commands as follows: Tree-SHA512: 13e5b3a544496ed2a6529ad45d03a2d872ebf41caaa06d0eec23a639d678ae1c55d73f2d4b164a4cc9e2c163264e736cd85eae90fde8089ca999cd810b16ecb5
2024-09-05init: error out if -maxconnections is negativeSergi Delgado Segura
2024-09-05init: improves file descriptors accounting and docsSergi Delgado Segura
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in he previous commit). Redefine some of the constants, plus document what are we accounting for so this can be extended more easily Remove FreeBSD workaround to #2695
2024-09-05init: fixes fd accounting regarding poll/selectSergi Delgado Segura
We are computing our file descriptors limits based on whether we use poll or select. However, we are taking that into account only partially (subtracting from fd_max in one case, but from nFD later on). Moreover, nBind is also only accounted for partially. Simplify and fix this logic
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-14 16:27:21 +0000