fa9ca13f35 refactor: Sort includes of touched source files (MarcoFalke)
facb152697 scripted-diff: Bump copyright headers after include changes (MarcoFalke)
fae71d30f7 clang-tidy: Apply modernize-deprecated-headers (MarcoFalke)
Pull request description:
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).
The check is currently disabled for headers, to exclude subtree headers.
ACKs for top commit:
l0rinc:
ACK fa9ca13f35
achow101:
ACK fa9ca13f35
janb84:
ACK fa9ca13f35
stickies-v:
ACK fa9ca13f35
Tree-SHA512: 6639608308c598d612e24435aa519afe92d71b955874b87e527245291fb874b67f3ab95d3a0a5125c6adce5eb41c0d62f6ca488fbbfd60a94f2063d734173f4d
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
ee045b61ef rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c372 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06 psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337a wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49 psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a118256948 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4a wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923 psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd81532 tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d612 psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61ef
rkrux:
re-ACK ee045b61ef
fjahr:
Code review ACK ee045b61ef
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
a0eed55398 run_command: Enable close_fds option to avoid lingering fds (Luke Dashjr)
c7c356a448 cpp-subprocess: Iterate through /proc/self/fd for close_fds option on Linux (Luke Dashjr)
4f5e04da13 Revert "remove unneeded close_fds option from cpp-subprocess" (Luke Dashjr)
Pull request description:
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).
> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and starting bitcoind again will fail). It's also a potential security issue if a child process is intended to be sandboxed at some point. Not to mention plain ugly :)
>
> cpp-subprocess has a feature to address this called close_fds. Not sure why it was removed in https://github.com/bitcoin/bitcoin/pull/29961 rather than fixing this during the migration, but this PR restores it, enables it for RunCommandParseJSON, and optimises it by iterating over /proc/self/fd/ like most other libraries do these days ([eg, glib]> (487b1fd20c/glib/gspawn.c (L1094))) since iterating all possible fd numbers [has been found to be problematic](https://bugzilla.redhat.com/show_bug.cgi?id=1537564).
>
> (Equivalent to https://github.com/bitcoin/bitcoin/pull/22417 was for boost::process)
ACKs for top commit:
achow101:
ACK a0eed55398
hebasto:
ACK a0eed55398, tested on Ubuntu 25.04:
vasild:
ACK a0eed55398
Tree-SHA512: 7dc1cb6cc1f45ff7c4f53512e400baad1a033b4ebf14ba6f6ffa38588314932d6d01ef67b197f081e8202bb802659ac6a87998277797721d6d7b20efde8e9a6b
SignPSBTInput will need to report the specific things that caused an
error to callers, so change it to return a PSBTError. Additionally some
callers will now check the return value and report an error to the user.
Currently, this should not change any behavior as the things that
SignPBSTInput will error on are all first checked by its callers.
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
iffaddrs.h that implements one, but not the other.
b9d4d5f66a net: Use GetAdaptersAddresses to get local addresses on Windows (laanwj)
Pull request description:
Instead of a `gethostname` hack, which is not guaranteed to return all addresses, use the official way of calling `GetAdaptersAddresses` to get local network addresses on Windows.
Do the same checks as the UNIX path: interface is up, interface is not loopback.
Suggested by Ava Chow.
Addiional changes:
- Cleanup: move out `FromSockAddr` in `netif.cpp` from MacOS and use it everywhere appropriate. This avoids code duplication.
ACKs for top commit:
davidgumberg:
utreACK b9d4d5f66a
achow101:
ACK b9d4d5f66a
Tree-SHA512: e9f0a7ec0c46f21c0377d5174e054a6569f858630727f94dac00c0cb7c241c56892d0b902706d6dd53880cc3b5ae1f2dba9caa1fec40e64cd4cf0d34493a49c1
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
Instead of a `gethostname` hack, use the official way of calling
`GetAdaptersAddresses` to get local network addresses on Windows.
As additional cleanup, move out `FromSockAddr` from MacOS and use it
everywhere appropriate.
Suggested by Ava Chow.
In almost all cases (the only exception is `getifaddrs`), we know the
size of the data passed into SetSockAddr, so we can check this to be
what is expected.
-noconf would previously lead to an ifstream "successfully" being opened to the ".bitcoin"-directory (not a file). (Guards against the general case of directories as configs are added in grandchild commit to this one).
Other users of AbsPathForConfigVal() in combination with negated args have been updated earlier in this PR ("args: Support -nopid" and "args: Support -norpccookiefile...").
e60cecc811 doc: add release note for 31156 (Martin Zumsande)
fc7dfb3df5 test: Don't enforce BIP94 on regtest unless specified by arg (Martin Zumsande)
Pull request description:
The added arg `-test=bip94` is only used in a functional test for BIP94. This is done because the default regtest consensus rules should follow mainnet, not testnet.
Fixes#31137.
ACKs for top commit:
achow101:
ACK e60cecc811
tdb3:
cr and light test ACK e60cecc811
rkrux:
tACK e60cecc811
BrandonOdiwuor:
utACK e60cecc811
laanwj:
Code review ACK e60cecc811
Tree-SHA512: ca2f322f89d8808dfc3565fe020d2615cfcc110e188a02128ad7108fef51c735b33d55b5e6a70c505d78f7291f3c635dc7dfbcd78be1348d4d6e483883be4216
The added regtest option -test=bip94 is only used in the functional
test for BIP94.
This is done because the default regtest consensus rules
should aim to follow to mainnet, not testnet.
33a28e252a Change default help arg to `-help` and mention `-h` and `-?` as alternatives (Lőrinc)
f0130ab1a1 doc: replace `-?` with `-h` for bench_bitcoin help (Lőrinc)
Pull request description:
The question mark is interpreted as a wildcard for any single character in Zsh (see https://www.techrepublic.com/article/globbing-wildcard-characters-with-zsh), so `bench_bitcoin -?` will not show the help message on systems using Zsh, such as macOS.
Since `-h` provides equivalent help functionality (as defined in https://github.com/bitcoin/bitcoin/blob/master/src/common/args.cpp#L684-L693), the `benchmarking.md` documentation has been updated to ensure compatibility with macOS.
----
### -?
> % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -?
zsh: no matches found: -?
### -h
> % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -h
Usage: bench_bitcoin [options]
Options:
...
----
Based on the comments the args help default was also changed to `-help`, mentioning `-h` and `-?` (instead of `-?` being the default)
ACKs for top commit:
edilmedeiros:
tACK 33a28e252a
maflcko:
lgtm ACK 33a28e252a
achow101:
ACK 33a28e252a
rkrux:
tACK 33a28e252a
laanwj:
Code review ACK 33a28e252a
Tree-SHA512: 8c6e27488462be9ba9186b34abe6249c1d93026b3963acc0f42c75496f39407563766ae518cf1839156039cc0047e29d91f70d191cfb97e0fbde85665e88c71e
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
c8e6771af0 test: restrict multiple CLI arguments (naiyoma)
8838c4f171 common/args.h: automate check for multiple cli commands (naiyoma)
Pull request description:
This PR is a continuation of the validation suggested [here](https://github.com/bitcoin/bitcoin/pull/27815) to ensure that only one Request Handler can be specified at a time.
ACKs for top commit:
stratospher:
reACK c8e6771.
achow101:
ACK c8e6771af0
tdb3:
cr re ACK c8e6771af0
Tree-SHA512: f4fe036fee342a54f1a7ac702ac35c40bf3d420fde6ab16313a75327292d5ee5c8ece1be9f852a13fcf73da1148b143b37b4894e294052abdde6eefb2e8c6f3f
6bfa26048d testnet: Add timewarp attack prevention for Testnet4 (Fabian Jahr)
0100907ca1 testnet: Add Testnet4 difficulty adjustment rules fix (Fabian Jahr)
74a04f9e7a testnet: Introduce Testnet4 (Fabian Jahr)
Pull request description:
To supplement the [ongoing conceptual discussion about a testnet reset](https://groups.google.com/g/bitcoindev/c/9bL00vRj7OU/m/9yCPo3uUBwAJ) I have drafted a move to v4 including a fix to the difficulty adjustment mechanism, which was part of the motivation that started the discussion.
Conceptual considerations:
- The conceptual discussion about doing a testnet4 or softforking the fix into testnet3 is outside of the scope of this PR and I would ask reviewers to contribute their opinions on this on the ML instead. However, I am happy to adapt this PR to a softfork change on testnet3 if there is consensus for that instead.
- The difficulty adjustment fix suggested here touches the `CalculateNextWorkRequired` function and uses the same logic used in `GetNextWorkRequired` to find the last previous block that was not mined with difficulty 1 under the exceptionf. An alternative fix briefly mentioned on the mailing list by Jameson Lopp would be to "restrict the special testnet minimum difficulty rule so that it can't be triggered on the block right before a difficulty retarget". That would also fix the issue but I find my suggestion here a bit more elegant.
ACKs for top commit:
jsarenik:
tACK 6bfa26048d
achow101:
ACK 6bfa26048d
murchandamus:
tACK 6bfa26048d
Tree-SHA512: 0b8b69a621406a944da5be551b863d065358ba94d85dd3b80d83c412660e230ee93b27316081fbee9b4851cc4ff8585db64c7dfa26cb5148ac835663f2712c3d
When using `sendrawtransaction` the ALREADY_IN_CHAIN error help string
may be confusing.
Rename TransactionError::ALREADY_IN_CHAIN to
TransactionError::ALREADY_IN_UTXO_SET and update the rpc help string.
Remove backwards compatibility alias as no longer required.
c7376babd1 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334 util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b4 util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9a common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae3 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608d util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad4792 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff4 util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420c build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24d test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd1
TheCharlatan:
Re-ACK c7376babd1
hebasto:
re-ACK c7376babd1.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
84900ac34f doc: add release-notes-27064.md (Matthew Zipkin)
855dd8d592 system: use %LOCALAPPDATA% as default datadir on windows (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2391
This PR changes the default datadir location on Windows from `C:\Users\Username\AppData\Roaming\Bitcoin` to `C:\Users\Username\AppData\Local\Bitcoin`. This change only applies to fresh installs. To preserve backwards compatibility, on startup we check for the existence of `C:\Users\Username\AppData\Roaming\Bitcoin\chainstate` and if it is there, we continue using the "Roaming" directory as the default datadir location.
[Note that in Windows 11 this change may be moot:](https://learn.microsoft.com/en-us/uwp/api/windows.storage.applicationdata.roamingfolder?view=winrt-22621)
> Roaming data and settings is no longer supported as of Windows 11. The recommended replacement is [Azure App Service](https://learn.microsoft.com/en-us/azure/app-service/). Azure App Service is widely supported, well documented, reliable, and supports cross-platform/cross-ecosystem scenarios such as iOS, Android and web. Settings stored here no longer roam (as of Windows 11), but the settings store is still available.
ACKs for top commit:
achow101:
ACK 84900ac34f
BenWestgate:
crACK 84900ac34f
hebasto:
re-ACK 84900ac34f, only addressed feedback since my recent [review](https://github.com/bitcoin/bitcoin/pull/27064#pullrequestreview-2028718273).
Tree-SHA512: 807c6e89571287e2c8f4934229aec91ef28e7d0a675234acf1b7d085c24c7b73a08b6e345fbfc9038e6239187b6b69c08490ddaa1c057de5ea975c4a000bba42
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
Add TransactionError to node namespace and include it directly instead of
relying on indirect include through common/messages.h
This is a followup to a previous commit which moved the TransactionError enum.
These changes were done in a separate followup just to keep the previous commit
more minimal and easy to review.
Move enum and message formatting functions to a common/messages header where
they should be more discoverable, and also out of the util library, so they
will not be a dependency of the kernel
The are no changes in behavior and no changes to the moved code.
Add separate PSBTError enum instead of reusing TransactionError enum for PSBT
operations, and drop unused error codes. The error codes returned by PSBT
operations and transaction broadcast functions mostly do not overlap, so using
an unified enum makes it harder to call any of these functions and know which
errors actually need to be handled.
Define PSBTError in the common library because PSBT functionality is
implemented in the common library and used by both the node (for rawtransaction
RPCs) and the wallet.
Move util/message to common/signmessage so it is named more clearly, and
because the util library is not supposed to depend on other libraries besides
the crypto library. The signmessage functions use CKey, CPubKey, PKHash, and
DecodeDestination functions in the consensus and common libraries.
992c714451 common: Don't terminate on null character in UrlDecode (Fabian Jahr)
099fa57151 scripted-diff: Modernize name of urlDecode function and param (Fabian Jahr)
8f39aaae41 refactor: Remove hooking code for urlDecode (Fabian Jahr)
650d43ec15 refactor: Replace libevent use in urlDecode with our own code (Fabian Jahr)
46bc6c2aaa test: Add unit tests for urlDecode (Fabian Jahr)
Pull request description:
Fixes#29654 (as a side-effect)
Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing it’s usage where it's possible, ideally with the end goal to removing it completely.
This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](e0a4574ba2/http.c (L3542)) we were using in `urlDecode` could be easily emulated in fewer LOC. This also ports the [applicable test vectors over from libevent](https://github.com/libevent/libevent/blob/master/test/regress_http.c#L3430).
ACKs for top commit:
achow101:
ACK 992c714451
theStack:
Code-review ACK 992c714451
maflcko:
ACK 992c714451👈
stickies-v:
ACK 992c714451
Tree-SHA512: 78f76ae7ab3b6710eab2aaac20f55eb0da7803e057eaa6220e865f328666a5399ef1a479702aaf630b2f974ad3aa15e2b6adac9c11bc8c3d4be21e8af1667fea
The previous behavior was the result of casting the result returned from the libevent function evhttp_uridecode to std:string but this was probably not intended.
