8dcbbbea64 test: fix bug in 22686 (S3RK)
Pull request description:
It seems there is a bug in the test in #22686, the code behaviour itself looks correct.
Instead of verifying the scenario from #22670 with both `upper_bound` and `lower_bound` for the transaction amount, the tests verified `lower_bound` two times. This fix is to properly use function parameter instead of a variable from the scope. The test still passes with both values, so no code changes are required.
ACKs for top commit:
achow101:
ACK 8dcbbbea64
Tree-SHA512: 82837b2e00bd075a7b6b7a31031f4d3ba1ab4bd5967e90488f527fcc618aeb3945d6ae3ed66b9eb11616dda3ef376c5001559d2a7a79a759fc6800358bf52537
f52a72af56 ci: Invalidate depends caches when sources have been changed (Hennadii Stepanov)
939640f87e ci: Reorder scripts to make git available before depends_sources_cache (Hennadii Stepanov)
Pull request description:
On master (502d22ceed) the Cirrus CI do _not_ invalidate depends caches when their sources has been changed, i.e. source version updates, a new dependency etc.
This behavior causes:
- breaking CI build for Android APK (see #22708)
- the cache sizes growing
It is assumed that caches could be invalidated via Cirrus CI API/GUI, but it is inconvenient and it does not work as expected in all cases.
The common part of `fingerprint_key`s for both `depends_sources_cache` and `depends_built_cache` is the recent commit that changes sources in the `src/depends` sub-directory.
For `depends_built_cache` additionally `CIRRUS_TASK_NAME` is used to avoid sharing of the same cache between tasks with different OSes and different `DEP_OPTS`.
---
The `depends_sources` cache:
- in master (502d22ceed)
- with this PR
ACKs for top commit:
MarcoFalke:
cr ACK f52a72af56
Zero-1729:
crACK f52a72af56
Tree-SHA512: 0d14814ff308317c7c18cc725cf69055139030c207200af003872068f5567bd4b9c1f6783845d11742e314ebcc1dc5cae16aa86475376b623d921af5fcda14dd
1ea11e10ac doc: link to managing-wallets from doc readme (fanquake)
Pull request description:
This was forgotten in #22523.
ACKs for top commit:
achow101:
ACK 1ea11e10ac
jarolrod:
ACK 1ea11e10ac
Tree-SHA512: b82664b282cc0fe733b752c011621593df0f846d2188f12dbc5fedb7ffed2bd161293ce2a369ca973926030795b5f7acde7a1cbf5e337042a6f665906069c656
cd37356ff9 [crypto] Fix K1/K2 use in ChaCha20-Poly1305 AEAD (Dhruv Mehta)
Pull request description:
BIP324 mentions K1 is used for the associated data and K2 is used for the payload. The code does the opposite. This is not a security problem but will be a problem across implementations based on the HKDF key derivations.
BIP324 author Jonas Schnelli thinks a [code update will be better](https://github.com/bitcoin/bitcoin/pull/15649#discussion_r440780669) than a BIP update.
If this PR is merged:
- [ ] We need to update the test vector 3 in BIP324
ACKs for top commit:
jonasschnelli:
utACK cd37356ff9
Tree-SHA512: e2165117bfbf7a031060e7376912f9af1c1bfc57916383799a0fa2c040e2caaab0d6aafc3425c083a233b96c84fafec75c938e00ceb6bd7d52607d58607cb145
f12fbad5a1 windres: use PACKAGE_VERSION rather than building more version numbers (fanquake)
Pull request description:
Rather than defining more strings, reuse PACKAGE_VERSION, which is already available.
We also already use PACKAGE_VERSION for `ProductVersion` and `FileVersion` in setup.nsi.
ACKs for top commit:
MarcoFalke:
cr ACK f12fbad5a1
laanwj:
Code review ACK f12fbad5a1
Tree-SHA512: b74a37cbba105d208d4da9264d295d7e052009fdd6b0ed54a0d9968bbe2deeba1766d6d310438b2939a81555faa0cbd67d5e53f0c8a2de669ce56353c1c67d22
68faa87881 test: use f-strings in mining_*.py tests (fanquake)
c2a5d560df test: use f-strings in interface_*.py tests (fanquake)
86d958262d test: use f-strings in feature_proxy.py (fanquake)
31bdb33dcb test: use f-strings in feature_segwit.py (fanquake)
b166d54c3c test: use f-strings in feature_versionbits_warning.py (fanquake)
cf6d66bf94 test: use f-strings in feature_settings.py (fanquake)
6651d77f22 test: use f-strings in feature_pruning.py (fanquake)
961f5813ba test: use f-strings in feature_notifications.py (fanquake)
1a546e6f6c test: use f-strings in feature_minchainwork.py (fanquake)
6679eceacc test: use f-strings in feature_logging.py (fanquake)
fb633933ab test: use f-strings in feature_loadblock.py (fanquake)
e9ca8b254d test: use f-strings in feature_help.py (fanquake)
ff7e330999 test: use f-strings in feature_filelock.py (fanquake)
d5a6adc5e4 test: use f-strings in feature_fee_estimation.py (fanquake)
a2de33cbdc test: use f-strings in feature_dersig.py (fanquake)
a2502cc63f test: use f-strings in feature_dbcrash.py (fanquake)
3e2f84e7a9 test: use f-strings in feature_csv_activation.py (fanquake)
e2f1fd8ee9 test: use f-strings in feature_config_args.py (fanquake)
36d33d32b1 test: use f-strings in feature_cltv.py (fanquake)
dca173cc04 test: use f-strings in feature_blocksdir.py (fanquake)
5453e87062 test: use f-strings in feature_backwards_compatibility.py (fanquake)
6f3d5ad67a test: use f-strings in feature_asmap.py (fanquake)
Pull request description:
Rather than using 3 different ways to build/format strings (sometimes all in the same test, i.e [`feature_config_args.py`](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_config_args.py)), consolidate to using [f-strings (3.6+)](https://docs.python.org/3/reference/lexical_analysis.html#f-strings), which are generally more concise / readable, as well as more performant than existing methods.
This deals with the `feature_*.py`, `interface_*.py` and `mining_*.py` tests.
See also: [PEP 498](https://www.python.org/dev/peps/pep-0498/)
ACKs for top commit:
mjdietzx:
reACK 68faa87881
Zero-1729:
crACK 68faa87881
Tree-SHA512: d4e1a42e07d96d2c552387a46da1534223c4ce408703d7568ad2ef580797dd68d9695b8d19666b567af37f44de6e430e8be5db5d5404ba8fcecf9f5b026a6efb
5449d44e37 scripts: prevent GCC optimising test symbols in test-symbol-check (fanquake)
Pull request description:
I noticed in #22381 that when the test-symbol-check target was being built with Clang and run in the CI it would fail due to using a too-new version of `pow` (used [here](d67330d112/contrib/devtools/test-symbol-check.py (L85))). Our CIs use Focal (glibc 2.31) and the version of `pow` was the optimized version introduced in [glibc 2.29](https://lwn.net/Articles/778286/):
```bash
* Optimized generic exp, exp2, log, log2, pow, sinf, cosf, sincosf and tanf.
```
This made sense, except for that if it was failing when built using Clang, why hadn't it also been failing when being built with GCC?
Turns out GCC is optimizing away that call to `pow` at all optimization levels, including `-O0`, see: https://godbolt.org/z/53MhzMxT7, and this has been the case forever, or at least since GCC 5.x. Clang on the other hand, will only optimize away the `pow` call at `-O1` and `-O2`, not `-O0`: https://godbolt.org/z/Wbnqj3q6c. Thus when this test was built with Clang (we don't pass `-O` so we default to `-O0`) it was failing in the CI environment, because it would actually have a call to the "new" `pow`.
Avoid this issue by using a symbol that won't be optimized away, or that we are unlikely to ever have versioning issues with.
ACKs for top commit:
laanwj:
ACK 5449d44e37
Tree-SHA512: 3a26c5c3a5f2905fd0dd90892470e241ba625c0af3be2629d06d5da3a97534c1d6a55b796bbdd41e2e6a26a8fab7d981b98c45d4238565b0eb7edf3c5da02007
d8ba6327b2 scripted-diff: replace clientInterface with m_client_interface in net (fanquake)
f68c6cebe6 net: use clientInterface rather than uiInterface (fanquake)
Pull request description:
First commit fixes two cases where we were using `uiInterface`, rather than `clientInterface`.
Second commit renames `clientInterface` to `m_client_interface`, to match banman.
ACKs for top commit:
hebasto:
ACK d8ba6327b2, verified that `uiInterface` is replaced in all sites.
Zero-1729:
crACK d8ba6327b2
Tree-SHA512: 9c893d8799b0bc7737836c16e11b77b6f9dffa31041ec6678e63cad958ea06da09a841b99cc61c1b4d9f6f4f1be397ca5a8d2fb2fb7ab08c9437043f8a57c3dc
aaa6ad5455 [MOVEONLY] [tests] Move addrman ser/deser tests to addrman_tests.cpp (John Newbery)
Pull request description:
Addrman serialization/deserialization tests are currently in net_tests.cpp.
Move them to addrman_tests.cpp with the rest of the addrman tests.
Reviewer hint: review using `git diff --color-moved=dimmed-zebra`
ACKs for top commit:
MarcoFalke:
review ACK aaa6ad5455📺
mjdietzx:
ACK aaa6ad5455
Saviour1001:
Concept ACK <code>[aaa6ad5](aaa6ad5455)</code>
Tree-SHA512: 9a575e63f8830a9dfba30ef63c83ae4b45813add9973308f0d7a2463886438575a2e8a4a23af5b19ed977fbaab1a212ef832f89a25de03920d493e9ebafa9c30
5c5d0b6264 Add FoundBlock.found member (Russell Yanofsky)
Pull request description:
This change lets IPC serialization code handle FoundBlock arguments more simply and efficiently. Without this change there was no way to determine from a FoundBlock object whether a block was found or not. So in order to correctly implement behavior of leaving FoundBlock output variables unmodified when a block was not found, IPC code would have to read preexisting output variable values from the local process, send them to the remote process, receive output values back from the remote process, and save them to output variables unconditionally. With FoundBlock.found method, the process is simpler. There's no need to read or send preexisting local output variable values, just to read final output values from the remote process and set them conditionally if the block was found.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
fjahr:
Code review ACK 5c5d0b6264
theStack:
Concept and code review ACK 5c5d0b6264
jamesob:
ACK 5c5d0b6264 ([`jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock`](https://github.com/jamesob/bitcoin/tree/ackr/22215.1.ryanofsky.refactor_add_foundblock))
Zero-1729:
crACK 5c5d0b6
Tree-SHA512: d906e1b7100ff72c3aa06d80bd77673887b2db670ebd52dce7c4f6f557a23a1744c6109308228a37fda6c6ea74f05ba0efecff0ef235ab06ea8acd861fbb8675
Addrman serialization/deserialization tests are currently in net_tests.cpp.
Move them to addrman_tests.cpp with the rest of the addrman tests.
Reviewer hint: review using `git diff --color-moved=dimmed-zebra`
Rather than defining more strings, reuse PACKAGE_VERSION, which is
already available.
We already use PACKAGE_VERSION for `ProductVersion` and `FileVersion` in setup.nsi.
4c43b7d41d contrib: use hkps://keys.openpgp.org to retrieve builder keys (fanquake)
Pull request description:
`hkps://hkps.pool.sks-keyservers.net` is essentially no-longer functional,
and a number of distributions and GPG tools have since switched to using
the `keys.openpgp.org` key server as their default.
See this Debian patch for additional context:
https://salsa.debian.org/debian/gnupg2/-/blob/debian/main/debian/patches/Use-hkps-keys.openpgp.org-as-the-default-keyserver.patch
Switch to using keys.openpgp.org in the CI as well.
ACKs for top commit:
MarcoFalke:
cr ACK 4c43b7d41d
Zero-1729:
ACK 4c43b7d41d
Tree-SHA512: e6c72b67778b76f81c659eee0e4195fea9e579587c64921affd35b9d46a077d4e8754b7fb85ca90a9a4bbc5cd5a47b0c6e4c9dbf9a335418a12f774d665e5a19
60e0cbdd57 [addrman] Merge the two Add() functions (Amiti Uttarwar)
Pull request description:
This PR merges the two definitions of this overloaded function to reduce code duplication.
When these functions were introduced in 5fee401fe1, there were multiple places that invoked `Add()` with a single addr and a vector of addrs each, so it made sense to overload the function. I could see how the small difference in log statement was more meaningful when a peer was added via IRC :)
Now, the definition of `Add()` that takes in a single address is only invoked from the hidden/test-only RPC `addpeeraddress`. These changes should not cause any observable difference, and are covered by the existing tests that use this RPC endpoint.
ACKs for top commit:
jnewbery:
Code review ACK 60e0cbdd57
Zero-1729:
crACK 60e0cbd
fanquake:
ACK 60e0cbdd57
Tree-SHA512: 782fb2ac6d2d403ba7d7ff543197ca42b610b9a8806952d271e57e2ee3527ad1a94af4ebbad5371b5e95d77df07c56ccc8c1d5a2c82cdecb0d2b5085b3bdd5ee
2d7534bd93 wallet: use PACKAGE_NAME instead of "Bitcoin" in rpcdump (fanquake)
14b4802405 wallet: use FormatFullVersion instead of CLIENT_BUILD in rpcdump (fanquake)
Pull request description:
The dumpwallet RPC is the last place we're using CLIENT_BUILD directly, rather FormatFullVersion() (which just returns it), so switch to using that. At the same time, use PACKAGE_NAME (Bitcoin Core), rather than just "Bitcoin".
ACKs for top commit:
MarcoFalke:
cr ACK 2d7534bd93
laanwj:
Tested ACK 2d7534bd93
achow101:
ACK 2d7534bd93
Zero-1729:
crACK 2d7534b
Tree-SHA512: b38ee074e317448719d2a628380786ec665413515b38d9ce680c21608bc2acf6a2bf817f78f100a8310477613ae72d6969cc4f595f4f44af0896659d3ebf2671
e2ff385e13 test: check for invalid `-prune` parameters (Sebastian Falbesoner)
Pull request description:
This small PR adds missing test coverage for invalid `-prune` parameter values / combinations:
77e23ca945/src/init.cpp (L926-L928)77e23ca945/src/init.cpp (L935-L937)77e23ca945/src/init.cpp (L844-L849)
Not sure if the tests fit into `feature_config_args.py` or should rather be moved into `feature_pruning.py`; the latter though seems to be run less often due to being very memory-hungry.
ACKs for top commit:
laanwj:
Code review ACK e2ff385e13
Tree-SHA512: bb0db98090058ecac9f8a01301634e9dba9a65fd56b6a0b770f88da28c4f01e240e22b1225f0d231e28bdd4b5b51bff0e6853cccc46ed0190e91b84f7954a9db
