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
Previously, our Android builds were geared towards generating APKs,
which relied on Qt. However, after migrating to C++20, compiling for
Android became unfeasible due to Qt 5.15's compatibility limitations
with NDK only up to r25, which includes an outdated embedded libc++.
All removed stuff will be reinstated after migrating the build system to
CMake and upgrading Qt to version 6.x."
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
Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed
on 'p2sh-segwit' and 'bech32' redeem scripts.
Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for
segwit output types, we don't want to enable this feature in legacy wallets for the
following reasons:
1) It introduces a compatibility-breaking change requiring downgrade protection; older
wallets would be unable to interact with these "new" legacy wallets.
2) Considering the ongoing deprecation of the legacy spkm, this issue adds another
good reason to transition towards descriptors.
This exercises the bug fixed by previous commits, where
we were unable to generate and sign for segwit redeem scripts
(in this case multisig redeem scripts) longer than 520 bytes.
and also, this adds coverage for legacy 15-15 multisig script
generation and signing.
The process currently fails to sign redeem scripts that are longer than
520 bytes. Even when it shouldn't. The 520 bytes redeem scripts limit
is a legacy p2sh rule, and not a segwit limitation.
Segwit redeem scripts are not restricted by the script item size limit.
The reason why this occurs, is the usage of the same keystore used by
the legacy spkm. Which contains blockage for any redeem scripts longer
than the script item size limit.
The multisig script generation process currently fails when
the user exceeds 15 keys, even when it shouldn't. The maximum
number of keys allowed for segwit redeem scripts (p2sh-segwit
and bech32) is 20 keys.
This is because the redeem script placed in the witness is not
restricted by the item size limit.
The reason behind this issue is the utilization of the legacy
p2sh redeem script restrictions on segwit ones. Redeem scripts
longer than 520 bytes are blocked from being inserted into the
keystore, which causes the signing process and the descriptor
inference process to fail.
This occurs because the multisig generation flow uses the same
keystore as the legacy spkm (FillableSigningProvider), which
contains the 520-byte limit.
And also, simplified the test a bit by re-using the already existing 'wallet_multi'
(instead of creating a new one). Plus, removed the 'is_bdb_compiled()' calls
which were there basically to check if the test has the wallet compiled or not.
The function exists merely to check that the node2's wallet
received the transactions created during all the 'do_multisig()'
calls.
It was created as a standalone function because 'getbalance()'
only returns something when transactions are confirmed. So,
the rationale on that time was to have a method mining blocks
to confirm the recently created transactions to be able to
check the incoming balance.
This is why we have the "moved" class field.
This change removes all the hardcoded amounts and verifies
node2 balance reception directly inside 'do_multisig()'.
Cleaning up the test in the following ways:
* Generate priv-pub key pairs used for testing only once (instead of doing it 4 times).
* Simplifies 'wmulti' wallet creation, load and unload process.
* Removes confusing class members initialized and updated inside a nested for-loop.
* Simplifies do_multisig() outpoint detection:
The outpoint index information is already contained in MiniWallet's
`send_to` return value dictionary as "sent_vout".
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
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
```
