fb9f150759 gui: fix misleading signmessage error with segwit (willcl-ark)
Pull request description:
As described in https://github.com/bitcoin/bitcoin/issues/10542 (and numerous other places), message signing in Bitcoin Core does not support "signing with a segwit address" and likely will not in the foreseeable future, or at least until a new message-signing standard is agreed upon.
Therefore update the possibly misleading error message presented to the user in the GUI to detail more specifically the reason their message cannot be signed, in the case that a non P2PKH address is entered.
This change takes the [suggested wording](https://github.com/bitcoin/bitcoin/issues/10542#issuecomment-1960313569) from @adiabat.
Perhaps with this we can close https://github.com/bitcoin/bitcoin/issues/10542 ?
ACKs for top commit:
hebasto:
ACK fb9f150759.
Tree-SHA512: 5ba8d722ad3632dad2e0a2aa94b0f466b904e7885a247a5d26ebdfce54e3611090b103029d8dfce92adc49e50fe5f4830f687d867b4c56c3ea997e519b4e188d
fa09451f8e Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba82 scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke)
Pull request description:
The `bitcoin-config.h` includes have issues:
* The header is incompatible with iwyu, because symbols may be defined or not defined. So the `IWYU pragma: keep` is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948959711
* Guarding the includes by `HAVE_CONFIG_H` is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853 .
ACKs for top commit:
achow101:
ACK fa09451f8e
TheCharlatan:
ACK fa09451f8e
hebasto:
re-ACK fa09451f8e, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2028864535) (`timedata.cpp` removed in https://github.com/bitcoin/bitcoin/pull/29623).
Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52
a68fed111b net: Fix misleading comment for Discover (laanwj)
7766dd280d net: Replace ifname check with IFF_LOOPBACK in Discover (laanwj)
Pull request description:
Checking the interface name is kind of brittle. In the age of network namespaces and containers, there is no reason a loopback interface can't be called differently.
Check for the `IFF_LOOPBACK` flag to detect loopback interface instead.
Also remove a misleading comment in Discover's doc comment.
ACKs for top commit:
sipa:
utACK a68fed111b
willcl-ark:
utACK a68fed111b
theuni:
utACK a68fed111b. Satoshi-era brittleness :)
Tree-SHA512: e2d7fc541f40f6a6af08286e7bcb0873ff55debdcd8b38b03f274897b673a6fb51d84d6c7241a02a9567ddf2645f50231d91bb1f55307ba7c6e68196c29b0edf
42fb5311b1 rpc: return warnings as an array instead of just a single one (stickies-v)
Pull request description:
The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning (i.e. the latest one that is set) is returned, the other ones are ignored.
Fix that by returning all warnings as an array.
As a side benefit, clean up the GetWarnings() logic.
Since this PR changes the RPC result schema, I've added release notes. Users can temporarily revert to the old results by using `-deprecatedrpc=warnings`, until it's removed in a future version.
---
Some historical context from git log:
- when `GetWarnings` was introduced in 401926283a, it was used in the `getinfo` RPC, where only a [single error/warning was returned](401926283a (diff-7442c48d42cd5455a79915a0f00cce5e13359db46437a32b812876edb0a5ccddR250)) (similar to how it is now).
- later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de25c, with the description [stating](ef2a3de25c (diff-1021bd3c74415ad9719bd764ad6ca35af5dfb33b1cd863c0be49bdf52518af54R411)) that it returned "any network warnings" but in practice still only a single warning was returned
ACKs for top commit:
achow101:
re-ACK 42fb5311b1
tdb3:
Re ACK for 42fb5311b1
TheCharlatan:
ACK 42fb5311b1
maflcko:
ACK 42fb5311b1🔺
Tree-SHA512: 4225ed8979cd5f030dec785a80e7452a041ad5703445da79d2906ada983ed0bbe7b15889d663d75aae4a77d92e302c93e93eca185c7bd47c9cce29e12f752bd3
7c69baf227 depends: pass verbose through to cmake based make (Max Edwards)
Pull request description:
While testing https://github.com/bitcoin/bitcoin/pull/29708 I was not able to enable verbose output to check which flags were being given to the compiler.
With this PR, running depends with V=1 will enable verbose output from makefiles generated by cmake.
How to test:
```shell
make -C depends libnatpmp V=1
```
ACKs for top commit:
hebasto:
ACK 7c69baf227. Tested using the folowing command:
fanquake:
ACK 7c69baf227
Tree-SHA512: 81cd1326e940c5f14cbde96735fd02b03c1150881ed88d1e8dfa9385dfa12284bfa2cdfe097ce5f43a726c1718afb76ae16f71552ab68c207d74fdc1f7bb46ae
9155b733e1 build, msvc: Compile test\fuzz\miniscript.cpp (Hennadii Stepanov)
Pull request description:
This PR resolves the remained point from the https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2028808614:
> What is the issue with the ... miniscript fuzz tests?
From the CI [log](https://github.com/bitcoin/bitcoin/actions/runs/8941546183/job/24562123707?pr=30031#step:29:234):
```
miniscript_script: succeeded against 721 files in 1s.
Run miniscript_script with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_script')]
miniscript_smart: succeeded against 1429 files in 2s.
Run miniscript_smart with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_smart')]
miniscript_stable: succeeded against 1871 files in 2s.
Run miniscript_stable with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_stable')]
miniscript_string: succeeded against 918 files in 3s.
Run miniscript_string with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_string')]
```
ACKs for top commit:
maflcko:
ACK 9155b733e1
TheCharlatan:
ACK 9155b733e1
Tree-SHA512: 967f199aac41733265532518ff7b1d881ba5a7bbde9f827d6a0b6d984c94a65b20d5854ce0ea247158eaa17b21d4c9f7d25c79bac17960788bacd2586112630b
This also changes behavior if ReadBlockFromDisk or
ReadRawBlockFromDisk fails. Previously, the node would crash
due to an assert. This has been replaced with logging the error,
disconnecting the peer, and returning early.
63317103c9 miniscript: make operator_mst consteval (Pieter Wuille)
Pull request description:
It seems modern compilers don't realize that all invocations of operator""_mst can be evaluated at compile time, despite the `constexpr` keyword.
Since C++20, we can force them to evaluate at compile time using `consteval`, turning all the miniscript type constants into actual compile-time constants.
This should give a nice but not very important speedup for miniscript logic, but it's also a way to start testing C++20 features.
ACKs for top commit:
hebasto:
re-ACK 63317103c9.
theuni:
utACK 63317103c9
Tree-SHA512: bdc9f1a6499b8bb3ca04f1a158c31e6876ba97206f95ee5718f50efd58b5b4e6b8867c07f791848430bfaa130b9676d8a68320b763cda9a340c75527acbfcc9e
976e5d8f7b test: Fix `test/streams_tests.cpp` compilation on SunOS / illumos (Hennadii Stepanov)
Pull request description:
On systems where `int8_t` is defined as `char`, the `{S,Uns}erialize(Stream&, signed char)` functions become undefined.
This PR resolves the issue by testing `{S,Uns}erialize(Stream&, int8_t)` instead.
No behavior change on systems where `int8_t` is defined as `signed char`, which is the case for most other systems.
Fixes https://github.com/bitcoin/bitcoin/issues/29884.
An alternative approach is mentioned in https://github.com/bitcoin/bitcoin/issues/29884#issuecomment-2058434577 as well.
ACKs for top commit:
maflcko:
lgtm ACK 976e5d8f7b
theuni:
ACK 976e5d8f7b. Nice to have the serialization concept actually tested :)
Tree-SHA512: 1033863e584fa8e99a281b236fa01fc919f610a024bcec792116762e28c1c16ee481bd01325c3a0ca9dd9d753176aa63bd9ac7e08a9bbce772db2949d06f6e61
f0e22be69a build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (fanquake)
b088062e68 ci: remove -Wdocumentation from -Werror in multiprocess CI (fanquake)
bec6a88fbc ci: remove -Warray-bounds from -Werror for win64 (fanquake)
7469ac7032 ci: disable -Werror=maybe-uninitialized for Windows builds (fanquake)
Pull request description:
Now that `CXXFLAGS` are [back in user control](https://github.com/bitcoin/bitcoin/pull/24391), I don't think there's a
reason to no-longer use our warning flags when `CXXFLAGS` has been
overriden (this includes, by default, when building from depends).
Anyone can suppress warnings from third-party code by
passing the relevant `-Wno-` options in `CXXFLAGS`.
Closes: #18092.
ACKs for top commit:
maflcko:
utACK f0e22be69a🍡
hebasto:
ACK f0e22be69a.
theuni:
ACK f0e22be69a. It'll be nice to have this fixed.
TheCharlatan:
ACK f0e22be69a
Tree-SHA512: dcef4bd4a57bab6f586ca015fad725e7a38bf24b7a08808a74d8c8ca47cf68c5fca7b04ed38649a047c6929fb708e2c97f2000fc46d5a8d25da49951c5bb0f66
ffc674595c Replace remaining "520" magic numbers with MAX_SCRIPT_ELEMENT_SIZE (Jon Atack)
Pull request description:
Noticed these while reviewing BIPs yesterday.
It would be clearer and more future-proof to refer to their constant name.
ACKs for top commit:
instagibbs:
ACK ffc674595c
sipa:
ACK ffc674595c
achow101:
ACK ffc674595c
glozow:
ACK ffc674595c, agree it's clearer for these comments to refer to the greppable name of the limit rather than the number
Tree-SHA512: 462afc1c64543877ac58cb3acdb01d42c6d08abfb362802f29f3482d75401a2a8adadbc2facd222a9a9fefcaab6854865ea400f50ad60bec17831d29f7798afe
It seems modern compilers don't realize that all invocations of operator""_mst
can be evaluated at compile time, despite the constexpr keyword.
Since C++20, we can force them to evaluate at compile time, turning all the
miniscript type constants into actual compile-time constants.
It appears that MSVC does not support consteval operator"" when used inside
certain expressions. For the few places where this happens, define a
constant outside the operator call.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
bd2de7ac59 refactor, test: Always initialize pointer (Hennadii Stepanov)
Pull request description:
This change fixes MSVC warning [C4703](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4703).
All `DisableSpecificWarnings` dropped from `test_bitcoin.vcxproj` as all remained are inherited from `common.init.vcxproj`.
Required to simplify warning suppression porting to the CMake-based build system.
ACKs for top commit:
maflcko:
utACK bd2de7ac59
sipsorcery:
utACK bd2de7ac59.
ryanofsky:
Code review ACK bd2de7ac59
Tree-SHA512: 006db041d3c3697a77d9df14de86cf7c8a10804b45789df01268b2236cf6452e77dc57e89f5d5a6bc26d4b5cd483f0722d6035649c8a523b57954bb1fc810d0c
As described in #10542 (and numerous other places), message signing in
Bitcoin Core only supports message signing using P2PKH addresses, at
least until a new message-signing standard is agreed upon.
Therefore update the possibly-misleading error message presented to the
user in the GUI to detail more specifically the reason their message
cannot be signed, in the case that a non P2PKH address is entered.
Now that CXXFLAGS are back in user control, I don't think there's a
reason to no-longer use our warning flags when CXXFLAGS has been
overriden (this includes when building from depends).
Anyone can suppress warnings from third-party code by
passing the relevant `-Wno-` options in CXXFLAGS.
Fixes: #18092.
This produces false positives, such as:
```bash
torcontrol.cpp: In static member function ‘static void TorControlConnection::readcb(bufferevent*, void*)’:
torcontrol.cpp:94:28: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
94 | self->message.code = ToIntegral<int>(s.substr(0, 3)).value_or(0);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./netaddress.h:18,
from ./torcontrol.h:11,
from torcontrol.cpp:6:
./util/strencodings.h:184:7: note: ‘result’ was declared here
184 | T result;
| ^~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:11088: libbitcoin_node_a-torcontrol.o] Error 1
```
b50d127a77 refactor: Make 64-bit shift explicit (Hennadii Stepanov)
Pull request description:
This PR fixes MSVC warning [C4334](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334) in the fuzzing code. Similar to https://github.com/bitcoin/bitcoin/pull/26252.
All `DisableSpecificWarnings` dropped from `fuzz.vcxproj` as all remained are inherited from `common.init.vcxproj`.
Required to simplify warning suppression porting to the CMake-based build system.
ACKs for top commit:
maflcko:
utACK b50d127a77
sipsorcery:
utACK b50d127a77
Tree-SHA512: 18f6082b4234506ad2f9df54e577031b97cdf9f7ef64cad4162f275660716ab73587a97d3af0f778dfd48d2751d8676b5d3381d0aa837fcc60a09704473a9209
ec1f1abfef test:Validate UTXO snapshot with coin_height > base_height & amount > money_supply (jrakibi)
Pull request description:
### Ensure snapshot loading fails for coins exceeding base height
**Objective**: This test verifies that snapshot loading is correctly rejected for coins with a height greater than the base height.
**Update**:
- Added `test_invalid_snapshot_wrong_coin_code` to `feature_assumeutxo.py`.
- The test artificially sets a coin's height above 299 in a snapshot and checks for load failure.
- Edit: Added a test case for outputs whose amounts surpass the MAX_MONEY supply limit.
This implementation addresses the request for enhancing `assumeutxo` testing as outlined in issue #28648
---
**Edit: This is an explanation on how I arrive at content values: b"\x84\x58" and b"\xCA\xD2\x8F\x5A"**
You can use this tool to decode the utxo snapshot https://github.com/jrakibi/utxo-live
Here’s an overview of how it’s done:
The serialization format for a UTXO in the snapshot is as follows:
1. Transaction ID (txid) - 32 bytes
2. Output Index (outnum)- 4 bytes
3. VARINT (code) - A varible-length integer encoding the height and whether the transaction is a coinbase. The format of this VARINT is (height << 1) | coinbase_flag.
4. VARINT (amount_v) - A variable-length integer that represents a compressed format of the output amount (in satoshis).
For the test cases mentioned:
* **`b"\x84\x58"`** - This value corresponds to a VARINT representing the height and coinbase flag. Once we decode this code, we can extract the height and coinbase using `height = code_decoded >> 1` and `coinbase = code_decoded & 0x01`. In our case, with code_decoded = 728, it results in `height = 364` and `coinbase = 0`.
* **`b"\xCA\xD2\x8F\x5A"`** - This byte sequence represents a compressed amount value. The decompression function takes this value and translates it into a full amount in satoshis. In our case, the decompression of this amount translates to a number larger than the maximum allowed value of coins (21 million BTC)
ACKs for top commit:
fjahr:
re-ACK ec1f1abfef
maflcko:
ACK ec1f1abfef👑
achow101:
ACK ec1f1abfef
Tree-SHA512: 42b36fd1d76e9bc45861028acbb776bd2710c5c8bff2f75c751ed505995fbc1d4bc698df3be24a99f20bcf6a534615d2d9678fb3394162b88133eaec88ca2120
8b52e7f628 update comments in cpp-subprocess (check_output references) (Sebastian Falbesoner)
97f159776e remove unused method `Popen::kill` from cpp-subprocess (Sebastian Falbesoner)
908c51fe4a remove commented out code in cpp-subprocess (Sebastian Falbesoner)
ff79adbe05 remove unused templates from cpp-subprocess (Sebastian Falbesoner)
Pull request description:
This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.
ACKs for top commit:
fjahr:
Code review ACK 8b52e7f628
achow101:
ACK 8b52e7f628
hebasto:
ACK 8b52e7f628.
Tree-SHA512: 14c1cd2216185d941923f06fdc7acbeed66cd87e2691d9a352f7309b3e07fe4877b580f598a2e4106f9c48395ed6de00a0bfb5d3c3af9c4624d1956a0f543e99
fa9be2f795 lint: [doc] Clarify Windows line endings (CR LF) not to be used (MarcoFalke)
Pull request description:
It has been this case since the linter was introduced years ago. Given a misunderstanding (https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2088028856), clarify the docs.
ACKs for top commit:
brunoerg:
ACK fa9be2f795
Tree-SHA512: be714db9df533e0962ed84102ffdb72717902949b930d58cf5a79cba36297f6b2b1f75e65a2c1f46bcb8e2f4ad5d025f3d15210f468a5ec9631a580b74f923ea
5195baa600 depends: fix miniupnpc snprintf usage on Windows (fanquake)
3c2d440f14 depends: switch miniupnpc to CMake (Cory Fields)
f5618c79d9 depends: add upstream CMake patch to miniupnpc (fanquake)
6866b571ab depends: miniupnpc 2.2.7 (fanquake)
Pull request description:
This picks up one of the changes from #29232, which is a switch to building miniupnpc with CMake. It includes an update to the most recent version of miniupnpc (2.2.7), which means we can drop one patch from that commit, and includes a new patch for a change I've upstreamed https://github.com/miniupnp/miniupnp/pull/721, as well as some suggestions from the previous PR.
ACKs for top commit:
theuni:
ACK 5195baa600.
TheCharlatan:
utACK 5195baa600
Tree-SHA512: 5b27e132cd5eed285e9be34c8b96893417d92a1ae55c99345c9a89e1c1c5e40e4bc840bc061b879758b2b11fcb520cd98c3da985c1e153f2e5380cf63efe2d69
e504b1fa1f test: Add test case for spending bare multisig (Brandon Odiwuor)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/29113
ACKs for top commit:
ajtowns:
ACK e504b1fa1f ; LGTM and just checking the 1-of-3 case seems fine
maflcko:
utACK e504b1fa1f
achow101:
ACK e504b1fa1f
willcl-ark:
reACK e504b1fa1f
Tree-SHA512: 641a12599efa34e1a3eb65b125318df326628fef3e6886410ea9e63a044664fad7bcad46d1d6f41ddc59630746b9963cedb569c2682b5940b32b9225883da8f2
Adds the following fixups in txorphan fuzz tests:
- Don't bond the output count of the created orphans based on the number of available coins
- Allow duplicate inputs, when applicable, but don't store duplicate outpoints
Rationale
---------
The way the test is currently written, duplicate inputs are allowed based on a random flag (`duplicate_input`).
If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection,
and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not.
This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop.
Additionally, both the input and output count of the transaction and bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.
