aboutsummaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2023-02-21verify-commits: Mention git v2.38.0 requirementAndrew Chow
2023-02-21wallet, rpc: Update migratewallet help text for encrypted walletsAndrew Chow
2023-02-21tests: Tests for migrating wallets by name, and providing passphraseAndrew Chow
2023-02-21Detailed error message for passphrases with null charsJohn Moffett
Since users may have thought the null characters in their passphrases were actually evaluated prior to this change, they may be surprised to learn that their passphrases no longer work. Give them feedback to explain how to remedy the issue.
2023-02-21doc: Release notes for 27068John Moffett
To reflect the change in behavior.
2023-02-21Test case for passphrases with null charactersJohn Moffett
Add a functional test to make sure the system properly accepts passphrases with null characters.
2023-02-21Pass all characters to SecureString including nullsJohn Moffett
`SecureString` is a `std::string` specialization with a secure allocator. However, it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected behavior. For instance, if a user enters a passphrase with an embedded null character (which is possible through Qt and the JSON-RPC), it will ignore any characters after the null, giving the user a false sense of security. Instead of assigning `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and doesn't make any extraneous copies in memory.
2023-02-21Merge bitcoin/bitcoin#26347: wallet: ensure the wallet is unlocked when ↵Andrew Chow
needed for rescanning 6a5b348f2e526f048d0b448b01f6c4ab608569af test: test rescanning encrypted wallets (ishaanam) 493b813e171a389a8b6750b4f2e42e8363a0267e wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam) 66a86ebabb26a055ca92af846bfa39dbd2f9f722 wallet: keep track of when the passphrase is needed when rescanning (ishaanam) Pull request description: Wallet passphrases are needed to top up the keypool of encrypted wallets during a rescan. The following RPCs need the passphrase when rescanning: - `importdescriptors` - `rescanblockchain` The following RPCs use the information about whether or not the passphrase is being used to ensure that full rescans are able to take place (meaning the following RPCs should not be able to run if a rescan requiring the wallet to be unlocked is taking place): - `walletlock` - `encryptwallet` - `walletpassphrasechange` `m_relock_mutex` is also introduced so that the passphrase is not deleted from memory when the timeout provided in `walletpassphrase` is up and the wallet is still rescanning. Fixes #25702, #11249 Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions. ACKs for top commit: achow101: ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af hernanmarino: ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af furszy: Tested ACK 6a5b348f Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5
2023-02-21Merge bitcoin/bitcoin#27122: script: BIP341 txdata cannot be precomputed ↵Andrew Chow
without spent outputs 95f12de92505522a32ba58acd5251c69e602d160 BIP341 txdata cannot be precomputed without spent outputs (Pieter Wuille) Pull request description: In `PrecomputedTransactionData::Init`, if `force` is set to `true`, `m_bip341_taproot_ready` is always set to true, suggesting that all its BIP341-relevant members (including `m_spent_amounts_single_hash`) are correct. If however no `spent` array of spent previous `CTxOut`s is provided, some of these members will be incorrect. This option was introduced in #21365. That doesn't actually hurt, as without prevout data, it's fundamentally impossible to generate correct BIP341 signatures anyway, and https://github.com/bitcoin/bitcoin/blob/f722a9bd132222d9d5cd503b5af25c905b205cdb/src/script/sign.cpp#L71 should prevent the logic from being used anyway. Still, don't set `m_bip341_taproot_ready` variable when we clearly don't have enough data to compute it. Discovered by Russell O'Connor. ACKs for top commit: ajtowns: ACK 95f12de92505522a32ba58acd5251c69e602d160 achow101: ACK 95f12de92505522a32ba58acd5251c69e602d160 instagibbs: ACK 95f12de92505522a32ba58acd5251c69e602d160 Tree-SHA512: 90acd2bfa50a7a0bde75a15a9f6c1f5c40f48fb5b870b1bbc4082777e24a482c8282463ef7d1245e53201dbcb5c196ef0386352f8e380e68cdf00c2111633b77
2023-02-21Raise PRNG seed log to INFO.roconnor-blockstream
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log of the failed build. For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test. By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
2023-02-21Revert "[contrib] verify-commits: Add MarcoFalke fingerprint"MarcoFalke
This reverts commit fa243293343eb964bfee5b91cc52b91f16232ab6.
2023-02-21github: Switch to yaml issue templateswillcl-ark
These provide more flexibility and can be designed to extract more information from users when submitting issues.
2023-02-20Add release note for PR#25943David Gumberg
Co-authored-by: glozow <gloriajzhao@gmail.com>
2023-02-20Add test for unspendable transactions and parameter 'maxburnamount' to ↵David Gumberg
sendrawtransaction. 'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.
2023-02-20Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX ↵fanquake
with avoidpartialspends 14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) Pull request description: Closes https://github.com/bitcoin/bitcoin/issues/27051 When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain. If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected. I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582 ACKs for top commit: achow101: ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
2023-02-20Merge bitcoin/bitcoin#27127: rpc: fix successful broadcast count in ↵fanquake
`submitpackage` error msg 7554b1fd663fe2010edb0e8a93ab85a6cb10a323 rpc: fix successful broadcast count in `submitpackage` error msg (Sebastian Falbesoner) Pull request description: If a `submitpackage` RPC call errors due to any of the individual tx broadcasts failing, the returned error message is supposed to contain the number of successful broadcasts so far: https://github.com/bitcoin/bitcoin/blob/4395b7f0845d2dca60f3b4e007ef5770ce8e2aa9/src/rpc/mempool.cpp#L848-L849 Right now this is wrongly always shown as zero. Fix this by adding the missing increment of the counter. While touching that area, the variable is also renamed to better reflect its purpose (s/num_submitted/num_broadcast/; the submission has already happened at that point) and named arguments for the `BroadcastTransaction` call are added. (Note that the error should be really rare, as all txs have already been submitted succesfully to the mempool. IIUC this code-path could only hit if somehow a tx is being removed from the mempool between `ProcessNewPackage` and the `BroadcastTransaction` calls, e.g. if a new block is received which confirms any of the package's txs.) ACKs for top commit: glozow: utACK 7554b1fd663fe2010edb0e8a93ab85a6cb10a323, thanks! Tree-SHA512: e362e93b443109888e28d6facf6f52e67928e8baaa936e355bfdd324074302c4832e2fa0bd8745309a45eb729866d0513b928ac618ccc9432b7befc3aa2aac66
2023-02-20Merge bitcoin/bitcoin#27113: rpc: Use a FlatSigningProvider in decodescript ↵fanquake
to allow inferring descriptors for scripts larger than 520 bytes 73ec4b2a8347c796b9aadc1f2576b286c469f9e7 tests: decodescript can infer descriptors for scripts >520 bytes (Andrew Chow) 7cc78223710679c6e7fd40b762798a1f5ca4938e rpc: Use FlatSigningProvider in decodescript (Andrew Chow) Pull request description: `FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded. Fixes #27111 ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/27113/commits/73ec4b2a8347c796b9aadc1f2576b286c469f9e7 Tree-SHA512: c0e6d21025e2da864471989ac94c54e127d05459b9b048f34a0da8d76d8e372d5472a2e667ba2db74d6286e3e6faa55486ffa9232a068b519afa676394031d5a
2023-02-20depends: use FORTIFY_SOURCE=3 with libeventfanquake
2023-02-20Merge bitcoin/bitcoin#27027: build: use _FORTIFY_SOURCE=3fanquake
4faa4e37a6511c6ada303ef7929ac99c7462f083 build: use _FORTIFY_SOURCE=3 (fanquake) Pull request description: [glibc 2.33](https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html) introduced a new fortification level, `_FORTIFY_SOURCE=3`. It improves the coverage of cases where `_FORTIFY_SOURCE` can use `_chk` functions. For example, using GCC 13 and glibc 2.36 (Fedora Rawhide), compiling master: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 33 ``` vs this branch: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __memset_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 61 ``` Usage of level 3 requires LLVM/Clang 9+, or GCC 12+. Older compilers/glibc will still use _FORTIFY_SOURCE=2. For example, in the glibc we currently use for Linux release builds (2.24), `__USE_FORTIFY_LEVEL` is determined using the following: ```c #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 # if !defined __OPTIMIZE__ || __OPTIMIZE__ <= 0 # warning _FORTIFY_SOURCE requires compiling with optimization (-O) # elif !__GNUC_PREREQ (4, 1) # warning _FORTIFY_SOURCE requires GCC 4.1 or later # elif _FORTIFY_SOURCE > 1 # define __USE_FORTIFY_LEVEL 2 # else # define __USE_FORTIFY_LEVEL 1 # endif #endif #ifndef __USE_FORTIFY_LEVEL # define __USE_FORTIFY_LEVEL 0 #endif ``` so any value > 1 will turn on `_FORTIFY_SOURCE=2`. This value detection logic has become slightly more complex in later versions of glibc. https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source ACKs for top commit: theuni: ACK 4faa4e37a6511c6ada303ef7929ac99c7462f083. After playing with this quite a bit I didn't observe any noticeable pitfalls. Tree-SHA512: e84ba49e3872c29fed1e2aea237b0d6bdff0d1274fa3297e2e08317cb62004396ee97b1cd6addb7c8b582498f3fa857a6d84c8e8f5ca97791b93985b47ff7faa
2023-02-20Merge bitcoin/bitcoin#27128: test: fix intermittent issue in ↵fanquake
`p2p_disconnect_ban` 1819564c2130d4d8537ca433c6688b56c769fb79 test: fix intermittent issue in `p2p_disconnect_ban` (brunoerg) Pull request description: Fixes #26808 When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection. ACKs for top commit: MarcoFalke: lgtm ACK 1819564c2130d4d8537ca433c6688b56c769fb79 Tree-SHA512: 53a386fc38e2faa6f6da3536e76857ff4b6f55e2590d73fe857b3fe5d0f3ff92c5c7e4abd50ab4be250cb2106a4d14ad95d4809ea60c6e00ed3ac0e71255b0b0
2023-02-20Merge bitcoin/bitcoin#25950: test: fix test abort for high timeout values ↵fanquake
(and `--timeout-factor 0`) 14302a4802e2dbb41f5189de88f99ddd5dda7736 test: fix test abort for high timeout values (and `--timeout-factor 0`) (Sebastian Falbesoner) Pull request description: On master, the functional tests's option `--timeout-factor 0` (which according to the test docs and parameter description should disable the RPC timeouts) currently fails, same as high values like `--timeout-factor 999999`: ``` $ ./test/functional/wallet_basic.py --timeout-factor 0 2022-08-29T01:26:39.561000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_f24yxzp5 2022-08-29T01:26:40.262000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/honey/bitcoin/test/functional/test_framework/test_framework.py", line 549, in start_nodes node.wait_for_rpc_connection() File "/home/honey/bitcoin/test/functional/test_framework/test_node.py", line 234, in wait_for_rpc_connection rpc.getblockcount() File "/home/honey/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__ response, status = self._request('POST', self.__url.path, postdata.encode('utf-8')) File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request self.__conn.request(method, path, postdata, headers) File "/usr/local/lib/python3.9/http/client.py", line 1285, in request self._send_request(method, url, body, headers, encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1331, in _send_request self.endheaders(body, encode_chunked=encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1280, in endheaders self._send_output(message_body, encode_chunked=encode_chunked) File "/usr/local/lib/python3.9/http/client.py", line 1040, in _send_output self.send(msg) File "/usr/local/lib/python3.9/http/client.py", line 980, in send self.connect() File "/usr/local/lib/python3.9/http/client.py", line 946, in connect self.sock = self._create_connection( File "/usr/local/lib/python3.9/socket.py", line 844, in create_connection raise err File "/usr/local/lib/python3.9/socket.py", line 832, in create_connection sock.connect(sa) OSError: [Errno 22] Invalid argument ``` This is caused by a high timeout value that Python's HTTP(S) client library can't cope with. Fix this by clamping down the connection's set timeout value in AuthProxy. The change can easily be tested by running an arbitrary test with `--timeout-factor 0` on master (should fail), on this PR (should pass) and on this PR with the clamping value increased by 1 (should fail). // EDIT: The behaviour was observed on OpenBSD 7.1 and Python 3.9.12. ACKs for top commit: MarcoFalke: lgtm ACK 14302a4802e2dbb41f5189de88f99ddd5dda7736 Tree-SHA512: 6469e8ac699f1bb7dea11d5fb8b3ae54d895bb908570587c5631144cd41fe980ca0b1e6d0b7bfa07983307cba15fb26ae92e6766375672bf5be838d8e5422dbc
2023-02-20test: fix intermittent issue in `p2p_disconnect_ban`brunoerg
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
2023-02-20Merge bitcoin/bitcoin#26883: src/node/miner cleanups, follow-ups for #26695glozow
6a5e88e5cf06a6b410486cc36aba7afece0d9da9 miner: don't re-apply default Options value if argument is unset (stickies-v) ea72c3d9d594b2ea9b3397e64efd08f8563cb400 refactor: avoid duplicating BlockAssembler::Options members (stickies-v) cba749a9b7a6cd24e8887bddeb0430a1ebc783da refactor: rename local gArgs to args (stickies-v) Pull request description: Two follow-ups for #26695, both refactoring and no observed (*) behaviour change: - Rename `gArgs` to `args` because it's not actually a global - Add `BlockAssembler::Options` as a (private) member to `BlockAssembler` to avoid having to assign all the options individually, essentially duplicating them Reduces LoC and makes the code more readable, in my opinion. --- (*) as [pointed out by ajtowns](https://github.com/bitcoin/bitcoin/pull/26883#discussion_r1068247937), this PR changes the interface of `ApplyArgsManOptions()`, making this not a pure refactoring PR. In practice, `ApplyArgsManOptions()` is never called in such a way that this leads to observed behaviour change. Regardless, I've carved out the potential behaviour change into a separate commit and would be okay with dropping it, should it turn out to be controversial. ACKs for top commit: glozow: ACK 6a5e88e5cf TheCharlatan: Light code review ACK 6a5e88e5cf06a6b410486cc36aba7afece0d9da9 Tree-SHA512: 15c30442ff0e070b1a58dc4c9615550d619ce35b4a2596b2c0a9d790259bbf987cab708f7cbb1057a8cf8b4c3226f3ad981282d3499ac442094806492a5f68ce
2023-02-20rpc: fix successful broadcast count in `submitpackage` error msgSebastian Falbesoner
If a `submitpackage` RPC call errors due to any of the individual tx broadcasts failing, the returned error message is supposed to contain the number of successful broadcasts so far. Right now this is wrongly always shown as zero. Fix this by adding the missing counting. (Note though that the error should be really rare, as all txs have already been submitted succesfully to the mempool.)
2023-02-19Merge bitcoin/bitcoin#26814: refactor: remove windows-only compat.h usage in ↵fanquake
random 621cfb77227b5a240d66547947f73130f0c51f44 random: consolidate WIN32 #ifdefs (fanquake) 75ec6275e6780b9ed18e271e6b24bef46d1af96d random: remove compat.h include (fanquake) 4dc12816ace11eceee05c1ad24dd925f420a0bda random: use int for MAX_TRIES (fanquake) Pull request description: This change is related to removing the use of `compat.h` as a miscellaneous catch-all for unclear/platform specific includes. Somewhat prompted by IWYU-related discussion here: https://github.com/bitcoin/bitcoin/pull/26763/files#r1058861693. The only reason `compat.h` is required in random.cpp for Windows (note the `#ifdef WIN32`), is for `ssize_t` and an "indirect" inclusion of `windows.h`. I say indirect, because `windows.h` isn't actually included in compat.h either, it's dragged in as a side-effect of other windows includes there, i.e `winsock2.h`. Remove this coupling by replacing `ssize_t` with int, just including `windows.h` and removing compat.h. ACKs for top commit: hebasto: re-ACK 621cfb77227b5a240d66547947f73130f0c51f44, rebased only since my [recent](https://github.com/bitcoin/bitcoin/pull/26814#pullrequestreview-1237312144) review. Verified with: john-moffett: ACK 621cfb77227b5a240d66547947f73130f0c51f44 Tree-SHA512: 31e1ed2e7ff7daf6c3ee72e6a908def52f7addf8305ba371c5032f1927cbb8ef5d302785e8de42b5c04a123052f04688cc9fd80decceb04738b5d9153f3d32d7
2023-02-18lint: enable E722 do not use bare exceptLeonardo Lazzaro
2023-02-17test: fix test abort for high timeout values (and `--timeout-factor 0`)Sebastian Falbesoner
2023-02-17Merge bitcoin/bitcoin#26940: test: create random and coins utils, add amount ↵Andrew Chow
helper, dedupe add_coin 4275195606e6f42466d9a8ef766b3035833df4d5 De-duplicate add_coin methods to a test util helper (Jon Atack) 9d92c3d7f42c18939a9a6aa1ee185f1c958360a0 Create InsecureRandMoneyAmount() test util helper (Jon Atack) 81f5ade2a324167c03c5ce765a26bd42ed652723 Move random test util code from setup_common to random (Jon Atack) Pull request description: - Move random test utilities from `setup_common` to a new `random` file, as many tests don't use this code. - Create a helper to generate semi-random CAmounts up to `MONEY_RANGE` rather than only uint32, and use the helper in the unit tests. - De-duplicate a shared `add_coin` method by extracting it to a `coins` test utility. ACKs for top commit: pinheadmz: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 achow101: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 john-moffett: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 Tree-SHA512: 3ed974251149c7417f935ef2f8865aa0dcc33b281b47522b0f96f1979dff94bb8527957f098fe4d210f40d715c00f29512f2ffe189097102229023b7284a3a27
2023-02-17Merge bitcoin/bitcoin#25862: refactor, kernel: Remove gArgs accesses from ↵Andrew Chow
dbwrapper and txdb aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f refactor, validation: Add ChainstateManagerOpts db options (Ryan Ofsky) 0352258148c51572426666d337c7b28d0033376c refactor, txdb: Use DBParams struct in CBlockTreeDB (Ryan Ofsky) c00fa1a7341d3f47f992e0beb043da655cbca777 refactor, txdb: Add CoinsViewOptions struct (Ryan Ofsky) 2eaeded37f3a07c35eef38d9a80c1e5fbd4d41ee refactor, dbwrapper: Add DBParams and DBOptions structs (Ryan Ofsky) Pull request description: Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules. This PR does not change behavior in any way. It is a simpler alternative to #25623 because the only thing it does is remove `gArgs` references from kernel code. It avoids other unnecessary changes like adding options to the kernel API (they can be added separately later). ACKs for top commit: TheCharlatan: Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f achow101: ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f furszy: diff ACK aadd7c5b Tree-SHA512: 46dfd5d99ab3110492e7bba97a87122c831b8344caaf7dd2ebdb6e0ad6aa9174d4d1832d6f3a7465eda9294fe50defaa3c000afbbddc4e72838687df09a63ffd
2023-02-17BIP341 txdata cannot be precomputed without spent outputsPieter Wuille
2023-02-17Merge bitcoin/bitcoin#20018: p2p: ProcessAddrFetch(-seednode) is unnecessary ↵Andrew Chow
if -connect is specified 2555a3950f0304b7af7609c1e6c696993c50ac72 p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta) Pull request description: If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted. Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`. Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`. If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration. ACKs for top commit: mzumsande: Code Review ACK 2555a3950f0304b7af7609c1e6c696993c50ac72 achow101: ACK 2555a3950f0304b7af7609c1e6c696993c50ac72 vasild: ACK 2555a3950f0304b7af7609c1e6c696993c50ac72 Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3
2023-02-17Merge bitcoin/bitcoin#25619: net: avoid overriding non-virtual ToString() in ↵Andrew Chow
CService and use better naming c9d548c91fb12fba516dee896f1f97692cfa2104 net: remove CService::ToStringPort() (Vasil Dimov) fd4f0f41e915d99c9b0eac1afd21c5628222e368 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov) 96c791dd20fea54c17d224000dee677bc158f66a net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov) 944a9de08a00f8273e73cd28b40e46cc0eb0bad1 net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov) 043b9de59aec88ae5e29daac7dc2a8b51a9414ce scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov) Pull request description: Before this PR we had the somewhat confusing combination of methods: `CNetAddr::ToStringIP()` `CNetAddr::ToString()` (duplicate of the above) `CService::ToStringIPPort()` `CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`) `CService::ToStringPort()` Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396). "IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP". Change the above to: `CNetAddr::ToStringAddr()` `CService::ToStringAddrPort()` The changes touch a lot of files, but are mostly mechanical. ACKs for top commit: sipa: utACK c9d548c91fb12fba516dee896f1f97692cfa2104 achow101: ACK c9d548c91fb12fba516dee896f1f97692cfa2104 jonatack: re-ACK c9d548c91fb12fba516dee896f1f97692cfa2104 only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests LarryRuane: ACK c9d548c91fb12fba516dee896f1f97692cfa2104 Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
2023-02-17Merge bitcoin/bitcoin#26889: refactor: wallet, remove global 'ArgsManager' ↵Andrew Chow
dependency 52f4d567d69425dfd514489079db80483024a80d refactor: remove <util/system.h> include from wallet.h (furszy) 6c9b342c306b9e17024762c4ba8f1c64e9810ee2 refactor: wallet, remove global 'ArgsManager' access (furszy) d8f5fc446216258a68e256076c889ec23471855f wallet: set '-walletnotify' script instead of access global args manager (furszy) 3477a28dd3b4bc6c1993554c5ce589d69fa86070 wallet: set keypool_size instead of access global args manager (furszy) Pull request description: Structurally, the wallet class shouldn't access the global `ArgsManager` class, its internal behavior shouldn't be coupled to a global command line args parsing object. So this PR migrates the only two places where we depend on it: (1) the keypool size, and (2) the "-walletnotify" script. And cleans up the, now unneeded, wallet `ArgsManager` ref member. Extra note: In the process of removing the args ref member, discovered and fixed files that were invalidly depending on the wallet header including `util/system.h`. ACKs for top commit: achow101: ACK 52f4d567d69425dfd514489079db80483024a80d TheCharlatan: Re-ACK 52f4d567d69425dfd514489079db80483024a80d hebasto: re-ACK 52f4d567d69425dfd514489079db80483024a80d Tree-SHA512: 0cffd99b4dd4864bf618aa45aeaabbef2b6441d27b6dbb03489c4e013330877682ff17b418d07aa25fbe1040bdf2c67d7559bdeb84128c5437bf0e6247719016
2023-02-17random: consolidate WIN32 #ifdefsfanquake
Order includes Remove // for xyz comments
2023-02-17random: remove compat.h includefanquake
We no-longer need ssize_t. Add windows.h, which was being indirectly included via compat.h. It isn't actually included in compat.h itself, but was being included as a side-effect of other includes, like winsock2.h.
2023-02-17random: use int for MAX_TRIESfanquake
Removing the use of ssize_t, removes the need to include compat.h, just to make Windows happy.
2023-02-17fuzz: avoid redundant dup key checks when creating Miniscript nodesAntoine Poinsot
Check it only once on the top level node. Running libfuzzer with -runs=0 against the qa-assets corpus (1b9ddc96586769d92b1b62775f397b7f1a63f142). Without this patch: miniscript_stable: Done 6616 runs in 118 second(s) miniscript_smart: Done 13182 runs in 253 second(s) With this patch: miniscript_stable: Done 6616 runs in 57 second(s) miniscript_smart: Done 13182 runs in 124 second(s)
2023-02-17build: use _FORTIFY_SOURCE=3fanquake
glibc 2.33 introduced a new fortification level, _FORTIFY_SOURCE=3. Which improves the coverage of cases where _FORTIFY_SOURCE can use _chk functions. For example, using GCC 13 and glibc 2.36 (Fedora Rawhide), compiling master: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 33 ``` vs this branch: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __memset_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 61 ``` Usage of level 3 requires LLVM/Clang 9+, or GCC 12+. Older compilers/glibc will still use _FORTIFY_SOURCE=2. For example, in the glibc we currently use for Linux release builds (2.24), FORTIFY_LEVEL is determined using the following: ```c ``` so any value > 1 will turn on _FORTIFY_SOURCE=2. https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source
2023-02-17Merge bitcoin/bitcoin#27029: guix: consolidate to glibc 2.27 for Linux buildsfanquake
d5d4b75840b4219495ed0fc421a4b71e757224ee guix: combine glibc hardening options into hardened-glibc (fanquake) c49f2b8eb5d70aea76e2aa06cdfcb2cc9fa1cb53 guix: remove no-longer needed powerpc workaround (fanquake) 74c989398971864afc7098818262ff0b76fbcf71 guix: use glibc 2.27 for all Linux builds (fanquake) Pull request description: Build against glibc 2.27 for all Linux builds (previously only used for RISC-V), and at the same time, increase our minimum required glibc to 2.27 (2018). This would drop support for Ubuntu Xenial (16.04) & Debian Stretch (9), from the produced release binaries. Compiling from source on those systems may be possible, assuming you can install a recent enough compiler/toolchain etc. ACKs for top commit: hebasto: ACK d5d4b75840b4219495ed0fc421a4b71e757224ee, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 910f0ef45b4558f2a45d35a5c1c39aaac97e8aff086dc4fc1eddbb80c0b6e4bd23667d64e21d0fd42e4db37b6f26f447ca5d1150bb861128af7e71fb42835cf8
2023-02-17Merge bitcoin/bitcoin#27106: net: remove orphaned CSubNet::SanityCheck()fanquake
30a3230e86dfd49c771432be6219841df5066eb4 script: remove out-of-date snprintf TODO (Jon Atack) 0e015146bd98831290b2b141914e3f93baf5bf8f net: remove orphaned CSubNet::SanityCheck() (Jon Atack) Pull request description: `CSubNet::SanityCheck()` was added in #20140, and not removed in #22570 when it became orphaned code. Also, remove an out-of-date `snprintf` TODO that was resolved in #27036, and fix up 2 words to make the spelling linter green again. ACKs for top commit: fanquake: ACK 30a3230e86dfd49c771432be6219841df5066eb4 pinheadmz: ACK 30a3230e86dfd49c771432be6219841df5066eb4 brunoerg: crACK 30a3230e86dfd49c771432be6219841df5066eb4 Tree-SHA512: f91a2a5af902d3b82ab496f19deeac17d58dbf72a8016e880ea61ad858b66e7ea0ae70b964c4032018eb3252cc34ac5fea163131c6a7f1baf87fc9ec9b5833d8
2023-02-16doc: add release note for #25574Martin Zumsande
2023-02-16validation: report if pruning prevents completion of verificationMartin Zumsande
Now the verifychain RPC returns false if the checks didn't finish because the blocks requested to be queried have been pruned.
2023-02-16init, validation: Improve handling if VerifyDB() fails due to insufficient ↵Martin Zumsande
dbcache The rpc command verifychain now fails if the dbcache was not sufficient to complete the verification at the specified level and depth. In the same situation, the VerifyDB check during Init will now fail (and lead to an early shutdown) if the user has explicitly specified -checkblocks or -checklevel but the check couldn't be executed because of the limited cache. If the user didn't change any of the two and is using the defaults, log a warning but don't prevent the node from starting up.
2023-02-16validation: return VerifyDBResult::INTERRUPTED if verification was interruptedMartin Zumsande
This means that the -verifydb RPC will now return false if it cannot finish due to the node being shutdown.
2023-02-16validation: Change return value of VerifyDB to enum typeMartin Zumsande
This does not change behavior. It is in preparation for special handling of the case where VerifyDB doesn't finish for various reasons, but doesn't fail.
2023-02-16Merge bitcoin/bitcoin#25344: New `outputs` argument for `bumpfee`/`psbtbumpfee`Andrew Chow
4c8ecccdcd813fac3a7ef6a1493ef3977220421d test: add tests for `outputs` argument to `bumpfee`/`psbtbumpfee` (Seibart Nedor) c0ebb9838252fb187db8719755801758d89651f7 wallet: add `outputs` arguments to `bumpfee` and `psbtbumpfee` (Seibart Nedor) a804f3cfc0b4761b9ca7976e6e4472cd93599bbf wallet: extract and reuse RPC argument format definition for outputs (Seibart Nedor) Pull request description: This implements a modification of the proposal in #22007: instead of **adding** outputs to the set of outputs in the original transaction, the outputs given by `outputs` argument **completely replace** the outputs in the original transaction. As noted below, this makes it easier to "cancel" a transaction or to reduce the amounts in the outputs, which is not the case with the original proposal in #22007, but it seems from the discussion in this PR that the **replace** behavior is more desirable than **add** one. ACKs for top commit: achow101: ACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d 1440000bytes: Code Review ACK https://github.com/bitcoin/bitcoin/pull/25344/commits/4c8ecccdcd813fac3a7ef6a1493ef3977220421d ishaanam: reACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d Tree-SHA512: 31361f4a9b79c162bda7929583b0a3fd200e09f4c1a5378b12007576d6b14e02e9e4f0bab8aa209f08f75ac25a1f4805ad16ebff4a0334b07ad2378cc0090103
2023-02-16wallet: Be able to unlock the wallet for migrationAndrew Chow
Since migration reloads the wallet, the wallet will always be locked unless the passphrase is given. migratewallet can now take the passphrase in order to unlock the wallet for migration.
2023-02-16rpc: Allow users to specify wallet name for migratewalletAndrew Chow
2023-02-16wallet: Allow MigrateLegacyToDescriptor to take a wallet nameAndrew Chow
An overload of MigrateLegacyToDescriptor is added which takes the wallet name. The original that took a wallet pointer is still available, it just gets the name, closes the wallet, and calls the new overload.
2023-02-16verify-commits: Skip checks for commits older than trusted rootsAndrew Chow
generated by cgit v1.2.3 (git 2.26.2) at 2025-03-16 04:26:56 +0000