78358ce09d ci: add libzmq3-dev to test-each-commit job (fanquake)
a2b1d2c5ec doc: remove bsdmainutils (fanquake)
36ff336d2e ci: remove bsdmainutils (fanquake)
Pull request description:
This was previously used to install `hexdump` (for the tests). However that isn't used by CMake. I'm not aware of any other tools from this package being used.
ACKs for top commit:
maflcko:
ACK 78358ce09d
hebasto:
ACK 78358ce09d.
Tree-SHA512: 01c1be81feba03a9645e3d382067df4cd7c64de184871c8d9691053a8f871fcedf48d298303554560df2cb1949fb35d5ce9ff20c751ff35789b7689d656c0287
fac587ea07 ci: Use C++23 once for testing (MarcoFalke)
fa053ab7c0 build: Add Centos Stream 9 EOL URL (MarcoFalke)
Pull request description:
There are no plans to switch to C++23 anytime soon in the next couple of years. The only place right now that is known to benefit is `src/compat/byteswap.h`.
However, it is still useful to test with the option, because deprecated, removed or changed language features, as well as compiler changes that are guarded by the language version will be tested and developers can learn about them upfront.
Also includes a minor doc fixup commit.
ACKs for top commit:
davidgumberg:
ACK fac587ea07
TheCharlatan:
ACK fac587ea07
Tree-SHA512: 1b81788eb5b4da77715d8b047279de65ae6b8920d5a21fd8cc94c3b0edb588ab8ffb7eaffb2f8b7806045de1d47ca85ca629f49038eca762f3136bf380cf3c87
7de0c99804 doc: update dev note examples for CMake (fanquake)
Pull request description:
Update the examples in the developer notes to work with CMake.
Also added an explicit `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON` for clarity.
ACKs for top commit:
davidgumberg:
Tested ACK 7de0c99804
TheCharlatan:
ACK 7de0c99804
jonatack:
Tested ACK 7de0c99804 on arm64 macOS 14.6.1
Tree-SHA512: 561fe5e777c5b29a4f26309700c03a730c5bbb2f838630abfaa4174112ced66e733c2109cb429a1927f1f3692bf1945f6386bcaffe604a76ea24633932d39171
fa80d39d82 ci: Re-add configs removed in cmake migration (MarcoFalke)
Pull request description:
In commit 9730288a0c many configs were removed from the CI without explanation.
Fix it by adding them back.
Can be reviewed by looking at:
* the parity table https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e
* the installed packages
* the CI logs from before the cmake migration and the CI logs of this pull request
ACKs for top commit:
fanquake:
ACK fa80d39d82
Tree-SHA512: a33335e117750e6c2e1490bb621f67c466f901793e43abe1bd0e263ef16fdcbc9e88be55c206167f3a5ddb39c1df6989c0fb7a96d9240243c000ba2e7f5e2747
Previously they may have taken more than 10 seconds. Now they should
finish in less than one second.
This also allows to drop one dependency to be installed.
The rules have many issues:
* Most are redundant, because Python already has a built-in
IndentationError, a subclass of SyntaxError, to enforce whitespace.
* They are not enforced consistently anyway, see for examples [1][2]
below.
* They are stylistic rules where the author intentionally formatted the
code to be easier to read. Starting to enforce them now would make the
code harder to read and create frustration in the future.
Fix all issues by removing them.
[1]:
test/functional/feature_cltv.py:63:35: E272 [*] Multiple spaces before keyword
|
61 | # | Script to prepend to scriptSig | nSequence | nLockTime |
62 | # +-------------------------------------------------+------------+--------------+
63 | [[OP_CHECKLOCKTIMEVERIFY], None, None],
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E272
[2]:
contrib/asmap/asmap.py:395:13: E306 [*] Expected 1 blank line before a nested definition, found 0
|
393 | prefix.pop()
394 | hole = not fill and (lhole or rhole)
395 | def candidate(ctx: Optional[int], res0: Optional[list[ASNEntry]],
| ^^^ E306
Any kind of syntax error is already reported, so there is no need to
enumerate all possible types of syntax errors of ancient versions of
Python 2 or 3.
Ideally all call sites should accept std::byte instead of uint8_t but those transformations are left to future PRs.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/\bParseHex\(("[^"]*")\)/\1_hex_u8/g' $(git grep -l ParseHex -- :src ':(exclude)src/test/util_tests.cpp')
sed -i --regexp-extended 's/\bParseHex<std::byte>\(("[^"]*")\)/\1_hex/g' $(git grep -l ParseHex -- :src ':(exclude)src/test/util_tests.cpp')
sed -i --regexp-extended 's/\bScriptFromHex\(("[^"]*")\)/ToScript(\1_hex)/g' src/test/script_tests.cpp
-END VERIFY SCRIPT-
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
- Adds using namespace.
- Extracts ToScript helper function from ScriptFromHex, to be used heavily in the next commit.
- Changes ScriptFromHex from using ParseHex to TryParseHex, now asserting the string is valid.
- Use even number of hex digits in comment (and apply replacement from next commit to only touch line once).
The following scripted-diff commit will replace ParseHex("...") with "..."_hex_u8, but this replacement will not work in cases where vectors are needed instead of arrays, and is not ideal in cases where std::byte is accepted.
For example, it is currently necessary to use _hex_v_u8 when calling CScript operator<< because that operator does not currently support std::array or std::byte.
Conversely, it is incorrect to use _hex_v instead of _hex in net_processing.cpp for the MakeAndPushMessage argument, because if the argument is a std::vector it is considered variable-length and serialized with a size prefix, but if the argument is a std::array or Span is it considered fixed length and serialized without a prefix.
By the same logic, it is also safe to change the NUMS_H constant in pubkey.cpp from a std::vector to std::array because it is never serialized.
* Use BOOST_CHECK_EQUAL_COLLECTIONS and BOOST_CHECK_EQUAL instead of deprecated BOOST_CHECK.
* Avoid repeating expected values.
* Break out repeated HEX_PARSE_INPUT and rename ParseHex_expected to HEX_PARSE_OUTPUT.
Done in preparation for adding a couple more tests in the next commit.
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>
Beyond renaming it also adjusts whitespace and adds braces to conform to current doc/developer-notes.md.
TestEncrypt: Change iterator type to auto in ahead of vector -> span conversion.
Only touches functions that will be modified in next commit.
7ee5c3c5b2 Fix a few likely documentation typos (Lőrinc)
Pull request description:
Found them during CMake migration - and ran a quick spellcheck for the rest to cover any remaining ones
ACKs for top commit:
maflcko:
lgtm ACK 7ee5c3c5b2
Tree-SHA512: c6e7aa1e952e0d093745c4e6004c3907b7a215c6f998cc205307c0c68abcc067bf3f56e22af0deb1710186e8a871306f4bae8a35c74581e5299abcbbcddfaa75
948238a683 test: Remove FastRandomContext global (Ryan Ofsky)
fa0fe08eca scripted-diff: [test] Use g_rng/m_rng directly (MarcoFalke)
fa54cab473 test: refactor: Accept any RandomNumberGenerator in RandMoney (MarcoFalke)
68f77dd21e test: refactor: Pass rng parameters to test functions (Ryan Ofsky)
fa19af555d test: refactor: Move g_insecure_rand_ctx.Reseed out of the helper that calls MakeRandDeterministicDANGEROUS (MarcoFalke)
3dc527f460 test: refactor: Give unit test functions access to test state (Ryan Ofsky)
fab023e177 test: refactor: Make unsigned promotion explicit (MarcoFalke)
fa2cb654ec test: Add m_rng alias for the global random context (MarcoFalke)
fae7e3791c test: Correct the random seed log on a prevector test failure (MarcoFalke)
Pull request description:
This is mostly a style-cleanup for the tests' random generation:
1) `g_insecure_rand_ctx` in the tests is problematic, because the name is a leftover when the generator was indeed insecure. However, now the generator is *deterministic*, because the seed is either passed in or printed (c.f. RANDOM_CTX_SEED). Stating that deterministic randomness is insecure in the tests seems redundant at best. Fix it by just using `m_rng` for the name.
2) The global random context has many one-line aliases, such as `InsecureRand32`. This is problematic, because the same line of code may use the context directly and through a wrapper at the same time. For example in net_tests (see below). This inconsistency is harmless, but confusing. Fix it by just removing the one-line aliases.
```
src/test/net_tests.cpp: auto msg_data_1 = g_insecure_rand_ctx.randbytes<uint8_t>(InsecureRandRange(100000));
````
3) The wrapper for randmoney has the same problem that the same unit test uses the context directly and through a wrapper at the same time. Also, it has a single type of Rng hardcoded. Fix it by accepting any type.
ACKs for top commit:
hodlinator:
ACK 948238a683
ryanofsky:
Code review ACK 948238a683. Only changes since last review were changing a comments a little bit.
marcofleon:
Code review ACK 948238a683. Only changes since my last review are the improvements in `prevector_tests`.
Tree-SHA512: 69c6b46a42cb743138ee8c87ff26a588dbe083e3efb3dca49b8a133ba5d3b09e8bf01c590ec7e121a7d77cb1fd7dcacd927a9ca139ac65e1f7c6d1ec46f93b57
a0abcbd382 doc: Mention multipath specifier (Ava Chow)
0019f61fc5 tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4b wallet: Move internal to be per key when importing (Ava Chow)
1692245525 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9 rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd22 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2da descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02 descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b1034 descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae15 descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f72 descriptors: Add PubkeyProvider::Clone (Ava Chow)
Pull request description:
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.
This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.
The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes#17190
ACKs for top commit:
darosior:
re-ACK a0abcbd382
mjdietzx:
reACK a0abcbd382
pythcoiner:
reACK a0abcbd
furszy:
Code review ACK a0abcbd
glozow:
light code review ACK a0abcbd382
Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
04fb085f6b build: remove old MSVC build system (fanquake)
Pull request description:
This has been superseded by CMake, and should not be blocked on anything for removal.
ACKs for top commit:
laanwj:
ACK 04fb085f6b
hebasto:
ACK 04fb085f6b.
TheCharlatan:
ACK 04fb085f6b
vasild:
ACK 04fb085f6b, except:
Tree-SHA512: 219005bd0de1a9e37dabe45c979601df3083368f4c5ea224f70cf4e6f52e49d8f25713015ce005be3b34b56377eaafa339be7c50c63ce7f3ce7841ae55f3221f
680f05ee98 build: fix version number post CMake (fanquake)
Pull request description:
CMake was merged after branching-off for `28.x`.
ACKs for top commit:
hebasto:
ACK 680f05ee98
TheCharlatan:
ACK 680f05ee98
vasild:
ACK 680f05ee98
Tree-SHA512: 18e10a49830b529f54f7c9ed81ff0489f9aac9779f34935ebe0051c742f3b7862ca5d3409a6d6347c2d9c6439dcdd1394333278308012fed473f5b00d80afdd5
