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."
7c69baf227252511455bc06e315f6a3c7fc5a398 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 7c69baf227252511455bc06e315f6a3c7fc5a398. Tested using the folowing command:
fanquake:
ACK 7c69baf227252511455bc06e315f6a3c7fc5a398
Tree-SHA512: 81cd1326e940c5f14cbde96735fd02b03c1150881ed88d1e8dfa9385dfa12284bfa2cdfe097ce5f43a726c1718afb76ae16f71552ab68c207d74fdc1f7bb46ae
9155b733e153e799f09cc7f7e9199ad776b2cbb1 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 9155b733e153e799f09cc7f7e9199ad776b2cbb1
TheCharlatan:
ACK 9155b733e153e799f09cc7f7e9199ad776b2cbb1
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.
63317103c9f2b0635558da814567bb79c17ae851 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 63317103c9f2b0635558da814567bb79c17ae851.
theuni:
utACK 63317103c9f2b0635558da814567bb79c17ae851
Tree-SHA512: bdc9f1a6499b8bb3ca04f1a158c31e6876ba97206f95ee5718f50efd58b5b4e6b8867c07f791848430bfaa130b9676d8a68320b763cda9a340c75527acbfcc9e
976e5d8f7b2bc77cb1443b8bf0f38cb07db70e9b 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 976e5d8f7b2bc77cb1443b8bf0f38cb07db70e9b
theuni:
ACK 976e5d8f7b2bc77cb1443b8bf0f38cb07db70e9b. Nice to have the serialization concept actually tested :)
Tree-SHA512: 1033863e584fa8e99a281b236fa01fc919f610a024bcec792116762e28c1c16ee481bd01325c3a0ca9dd9d753176aa63bd9ac7e08a9bbce772db2949d06f6e61
f0e22be69a15248c42964d57f44ce8c37a36081d build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (fanquake)
b088062e687d95deff28b0715fd4859449b56584 ci: remove -Wdocumentation from -Werror in multiprocess CI (fanquake)
bec6a88fbcff0bd974e820cefd2be2d00b6f6c56 ci: remove -Warray-bounds from -Werror for win64 (fanquake)
7469ac7032e26fe805c5d15ecded2d44253bc9c1 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 f0e22be69a15248c42964d57f44ce8c37a36081d 🍡
hebasto:
ACK f0e22be69a15248c42964d57f44ce8c37a36081d.
theuni:
ACK f0e22be69a15248c42964d57f44ce8c37a36081d. It'll be nice to have this fixed.
TheCharlatan:
ACK f0e22be69a15248c42964d57f44ce8c37a36081d
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>
ffc674595cb19b2fdc5705b355bdd3e7f724b860 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 ffc674595cb19b2fdc5705b355bdd3e7f724b860
sipa:
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
achow101:
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
glozow:
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860, agree it's clearer for these comments to refer to the greppable name of the limit rather than the number
Tree-SHA512: 462afc1c64543877ac58cb3acdb01d42c6d08abfb362802f29f3482d75401a2a8adadbc2facd222a9a9fefcaab6854865ea400f50ad60bec17831d29f7798afe
af3c18169a075222fe0795ab24b8b20ad5e30ae4 [test]: remove duplicate WITNESS_SCALE_FACTOR (ismaelsadeeq)
Pull request description:
Notice this while working on #29523
- `blocktools.py` and `messages.py` both define `WITNESS_SCALE_FACTOR` constant
99d7538cdb/test/functional/test_framework/blocktools.py (L48)99d7538cdb/test/functional/test_framework/messages.py (L68)
- This PR deletes the one in `blocktools.py` and update the tests to only use `WITNESS_SCALE_FACTOR` from `messages.py`
ACKs for top commit:
maflcko:
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
sipa:
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
achow101:
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
glozow:
lgtm ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
brunoerg:
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
willcl-ark:
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
Tree-SHA512: 6bd8060c9eea10e03940acee2aa4cd08e4e0afb6d26be3e6300ad405fd0af5b373a00e994eb39515a2dcafa1625562bcd57945049a84b9c9dcc7ea60c24f0911
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>
bd2de7ac591d7704b79304089ad1fb57e085da8b 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 bd2de7ac591d7704b79304089ad1fb57e085da8b
sipsorcery:
utACK bd2de7ac591d7704b79304089ad1fb57e085da8b.
ryanofsky:
Code review ACK bd2de7ac591d7704b79304089ad1fb57e085da8b
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
```
b50d127a7710d790c2ba4a08f01b832c2a0b1203 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 b50d127a7710d790c2ba4a08f01b832c2a0b1203
sipsorcery:
utACK b50d127a7710d790c2ba4a08f01b832c2a0b1203
Tree-SHA512: 18f6082b4234506ad2f9df54e577031b97cdf9f7ef64cad4162f275660716ab73587a97d3af0f778dfd48d2751d8676b5d3381d0aa837fcc60a09704473a9209
ec1f1abfefa281e62bb876aa1c4738d576ef9a47 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 ec1f1abfefa281e62bb876aa1c4738d576ef9a47
maflcko:
ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a4 👑
achow101:
ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47
Tree-SHA512: 42b36fd1d76e9bc45861028acbb776bd2710c5c8bff2f75c751ed505995fbc1d4bc698df3be24a99f20bcf6a534615d2d9678fb3394162b88133eaec88ca2120
8b52e7f628304e83b0e36fd97e617de0f71c5a62 update comments in cpp-subprocess (check_output references) (Sebastian Falbesoner)
97f159776ec06f767df1d4990aa7d0859140f52f remove unused method `Popen::kill` from cpp-subprocess (Sebastian Falbesoner)
908c51fe4afeba0af500c6275027b1afa1b3bd19 remove commented out code in cpp-subprocess (Sebastian Falbesoner)
ff79adbe056220202f7a56d67f788c38fc49ef9f 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 8b52e7f628304e83b0e36fd97e617de0f71c5a62
achow101:
ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62
hebasto:
ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62.
Tree-SHA512: 14c1cd2216185d941923f06fdc7acbeed66cd87e2691d9a352f7309b3e07fe4877b580f598a2e4106f9c48395ed6de00a0bfb5d3c3af9c4624d1956a0f543e99
22574046c90c0662f3aa9b1baea074aff54f92a9 doc: add LLVM instruction for macOS < 13 (Sjors Provoost)
Pull request description:
#29208 bumped clang to 14, which users of old macOS versions need to install manually. This PR adds instructions.
Xcode 14.3.1 ships clang 14.0.3 (14.0.0 is broken, see #29918):
https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)
The system requirements for that is macOS Ventura 13.0 or later: https://developer.apple.com/documentation/xcode-release-notes/xcode-14_3_1-release-notes#
Homebrew itself officially supports macOS 12 or later, but _may_ still work on macOS 11: https://docs.brew.sh/Installation
Fwiw macOS 11 Big Sur last got an update in September 2023, so Apple has not _entirely_ written it off: https://en.wikipedia.org/wiki/MacOS_Big_Sur
ACKs for top commit:
maflcko:
utACK 22574046c90c0662f3aa9b1baea074aff54f92a9
TheCharlatan:
ACK 22574046c90c0662f3aa9b1baea074aff54f92a9
Tree-SHA512: 5b4bcc71966d1da84bc4da32da89e0dea9f519f37d9e14e169140c92af044b33f404f01ae7d10f53ab5345dd51ac404c161389efef93da5cacbfd52a43881695
fa9be2f79520fff9cfe2ed35ace05cb322680af3 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 fa9be2f79520fff9cfe2ed35ace05cb322680af3
Tree-SHA512: be714db9df533e0962ed84102ffdb72717902949b930d58cf5a79cba36297f6b2b1f75e65a2c1f46bcb8e2f4ad5d025f3d15210f468a5ec9631a580b74f923ea
5195baa60087ee366290887ad982fc491e14c111 depends: fix miniupnpc snprintf usage on Windows (fanquake)
3c2d440f1497f60bb444326f51383df244dcdfe9 depends: switch miniupnpc to CMake (Cory Fields)
f5618c79d9e7af05c2987044dc2da03697f8bb89 depends: add upstream CMake patch to miniupnpc (fanquake)
6866b571ab96f03ca0775424e45458c5731f230f 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 5195baa60087ee366290887ad982fc491e14c111.
TheCharlatan:
utACK 5195baa60087ee366290887ad982fc491e14c111
Tree-SHA512: 5b27e132cd5eed285e9be34c8b96893417d92a1ae55c99345c9a89e1c1c5e40e4bc840bc061b879758b2b11fcb520cd98c3da985c1e153f2e5380cf63efe2d69
e504b1fa1fa4d014b329dea81bfdf1aa55db238f 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 e504b1fa1fa4d014b329dea81bfdf1aa55db238f ; LGTM and just checking the 1-of-3 case seems fine
maflcko:
utACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
achow101:
ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
willcl-ark:
reACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f
Tree-SHA512: 641a12599efa34e1a3eb65b125318df326628fef3e6886410ea9e63a044664fad7bcad46d1d6f41ddc59630746b9963cedb569c2682b5940b32b9225883da8f2
