47c48b5f35b4910fcf87caa6e37407e67d879c80 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz)
77349713b189e80f2c140db4df50177353a1cb83 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz)
86dbd54ae8a8f9c693c0ea67114bbff24a0754df test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz)
Pull request description:
I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds
I noticed this improvement should probably be made when testing performance/runtimes of https://github.com/bitcoin/bitcoin/pull/22698. But I wanted to separate this out from that PR so the affects of each is decoupled
Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here
ACKs for top commit:
theStack:
Tested ACK 47c48b5f35b4910fcf87caa6e37407e67d879c80
Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
fa35efa84bb614df9629076e45e9e974d9e940f6 ci: Run arm task on arm64 hardware (MarcoFalke)
Pull request description:
It will still run cross-compilation to armhf, but run the binaries on the hardware itself, not qemu.
There shouldn't be any significant difference, other than maybe a slight speedup.
ACKs for top commit:
hebasto:
re-ACK fa35efa84bb614df9629076e45e9e974d9e940f6
Tree-SHA512: d03bb2b55d0a6b5021243eedff6e143f4fad03a1a8913000c2c5c74665e4bb1c77cb30aa112dbddbc0bb7c645bec18f6b8b8ba91049587b464bbe5ec07dd46bd
5a9e255e5a324e7aa0b63a9634aa3cfda9a300bd ci: Run fuzzer task for the master branch only (Hennadii Stepanov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/22629#issuecomment-896454270:
> I think we need to decide whether running the fuzzer CI in any branch other than master is something we want to be doing / maintaining. This seems pretty unsustainable unless we at least make changes in regards to the fuzz inputs being used by the different branches. I'm pretty sure Marco has mentioned this before.
This PR makes CI ignore fuzz tests by forcing `RUN_FUZZ_TESTS=false` for all cases when it is not the master branch or a PR based on it.
See #22731 as a demo for the 22.x branch.
ACKs for top commit:
MarcoFalke:
cr ACK 5a9e255e5a324e7aa0b63a9634aa3cfda9a300bd no opinion on the concept, also didn't test
fanquake:
ACK 5a9e255e5a324e7aa0b63a9634aa3cfda9a300bd - didn't test other than to look at #22731.
Tree-SHA512: 48f8f02f1814d4f15293a8804b76d544a08784ea7acd930b5c6d4608564d30aa5a608b1a511386ffda6975feec700c1bbeb86a30a75a7e48a1c5b167a227dbdd
132cae44f2d031bdaa1e459b92ec89ad585dfc9f doc: Mention the flat directory structure for uploads (Andrew Chow)
fb17c99e35e72f3b21ec3b5473e84c21dc964776 guix: Don't include directory name in SHA256SUMS (Andrew Chow)
Pull request description:
The SHA256SUMS file can be used in a sha256sum -c command to verify downloaded binaries. However users are likely to download just a single file and not place this file in the correct directory relative to the SHA256SUMS file for the simple verification command to work. By not including the directory name in the SHA256SUMS file, it will be easier for users to verify downloaded binaries.
ACKs for top commit:
Zero-1729:
re-ACK 132cae44f2d031bdaa1e459b92ec89ad585dfc9f
fanquake:
ACK 132cae44f2d031bdaa1e459b92ec89ad585dfc9f
Tree-SHA512: c9ff416b8dfb2f3ceaf4d63afb84aac9fcaefbbf9092f9e095061b472884ec92c7a809e6530c7132a82cfe3ab115a7328e47994a412072e1d4feb26fc502c8c5
4d2fa97031a6f31da984d4c2c105447ed692c6ed [addrman] Clean up ctor (John Newbery)
7e6e65918f75211b517fc887f5d90df8edd52ced [addrman] inline Clear() into CAddrMan ctor (John Newbery)
406be5ff9699874dc1d38d11f036e33cbdb820c9 [addrman] Remove all public uses of CAddrMan.Clear() from the tests (John Newbery)
ed9ba8af08f857bda3ce2f77413317374c22d7b4 [tests] Remove CAddrMan.Clear() call from CAddrDB::Read() (John Newbery)
e8e7392311edf44278d76743bebe902d4ac94662 [addrman] Don't call Clear() if parsing peers.dat fails (John Newbery)
181a1207ba6bd179d181f3e2534ef8676565ce72 [addrman] Move peers.dat parsing to init.cpp (John Newbery)
Pull request description:
`CAddrMan::Clear()` exists to reset the internal state of `CAddrMan`. It's currently used in two places:
- on startup, if deserializing peers.dat fails, `Clear()` is called to reset to an empty addrman
- in tests, `Clear()` is called to reset the addrman for more tests
In both cases, we can simply destruct the `CAddrMan` and construct a new, empty addrman. That approach is safer - it's possible that `Clear()` could 'reset' the addrman to a state that's not equivalent to a freshly constructed addrman (one actual example of this is that `Clear()` does not clear the `m_tried_collisions` set). On the other hand, if we destruct and then construct a fresh addrman, we're guaranteed that the new object is empty.
This wasn't possible when addrman was initially implemented, since it was a global, and so it would only be destructed on shutdown. However, addrman is now owned by `node.context`, so we have control over its destruction/construction.
ACKs for top commit:
laanwj:
Code review ACK 4d2fa97031a6f31da984d4c2c105447ed692c6ed
vasild:
ACK 4d2fa97031a6f31da984d4c2c105447ed692c6ed
Zero-1729:
crACK 4d2fa97031a6f31da984d4c2c105447ed692c6ed
Tree-SHA512: f715bf2cbff4f8c3a9dbc613f8c7f11846b065d6807faf3c7d346a0b0b29cbe7ce1dc0509465c2c9b88a8ad55299c9182ea53f5f743e47502a69a0f375e09408
e9cf506ab015ba095dbf6a69ec4b7278dbdc548b ci: Make git available for all merge commits (Hennadii Stepanov)
040e4deddcc176ecd8022549474fc79cd6fa05f5 scripted-diff: Rename template to avoid CI configuration parsing warning (Hennadii Stepanov)
Pull request description:
1) Remove Cirrus CI configuration parsing warning:
2) Make `git` available for merge commits, because it is now required for `depends_sources_cache` and `depends_built_cache`.
Top commit has no ACKs.
Tree-SHA512: d5cfb49492d2635fc2019220b0023179e09608bcb5a77c3d445e53c4cd714b145d6107ae02f2e3d0e2b196c2974b4688ae3e4fc233beb0917da29771795ed08e
The uploaded binaries need to match the same flat directory structure of
the SHA256SUMS file in order for torrent downloaders to be able to
verify the download without moving files. Mention this in the release
process doc.
8dcbbbea6486e9ab7d5e7397b82585141f9910bf 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 8dcbbbea6486e9ab7d5e7397b82585141f9910bf
Tree-SHA512: 82837b2e00bd075a7b6b7a31031f4d3ba1ab4bd5967e90488f527fcc618aeb3945d6ae3ed66b9eb11616dda3ef376c5001559d2a7a79a759fc6800358bf52537
f52a72af569526f319d9f1480eb473b9570125c3 ci: Invalidate depends caches when sources have been changed (Hennadii Stepanov)
939640f87ec385d08f1b04bfd7f61c80038c4565 ci: Reorder scripts to make git available before depends_sources_cache (Hennadii Stepanov)
Pull request description:
On master (502d22ceed1f90ed41336260f8eb428d3acaf514) 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 (502d22ceed1f90ed41336260f8eb428d3acaf514)
- with this PR
ACKs for top commit:
MarcoFalke:
cr ACK f52a72af569526f319d9f1480eb473b9570125c3
Zero-1729:
crACK f52a72af569526f319d9f1480eb473b9570125c3
Tree-SHA512: 0d14814ff308317c7c18cc725cf69055139030c207200af003872068f5567bd4b9c1f6783845d11742e314ebcc1dc5cae16aa86475376b623d921af5fcda14dd
Clear() is now only called from the ctor, so just inline the code into
that function.
The LOCK(cs) can be removed, since there can be no data races in the ctor.
Also move the function definition out of the header and into the cpp file.
Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required.
Also make CAddrMan::Clear() private to ensure that no call sites are missed.
1ea11e10acd60807a06adea5ecf553974a1b0346 doc: link to managing-wallets from doc readme (fanquake)
Pull request description:
This was forgotten in #22523.
ACKs for top commit:
achow101:
ACK 1ea11e10acd60807a06adea5ecf553974a1b0346
jarolrod:
ACK 1ea11e10acd60807a06adea5ecf553974a1b0346
Tree-SHA512: b82664b282cc0fe733b752c011621593df0f846d2188f12dbc5fedb7ffed2bd161293ce2a369ca973926030795b5f7acde7a1cbf5e337042a6f665906069c656
62a09a30772141ef4add2f10d29927211abf57eb refactor: remove ::vpwallets and related global variables (Russell Yanofsky)
Pull request description:
Get rid of global wallet list variables by moving them to WalletContext struct
- [`cs_wallets`](e638acf697/src/wallet/wallet.cpp (L56)) is now [`WalletContext::wallet_mutex`](4be544c7ec/src/wallet/context.h (L37))
- [`vpwallets`](e638acf697/src/wallet/wallet.cpp (L57)) is now [`WalletContext::wallets`](4be544c7ec/src/wallet/context.h (L38))
- [`g_load_wallet_fns`](e638acf697/src/wallet/wallet.cpp (L58)) is now [`WalletContext::wallet_load_fns`](4be544c7ec/src/wallet/context.h (L39))
ACKs for top commit:
achow101:
ACK 62a09a30772141ef4add2f10d29927211abf57eb
meshcollider:
re-utACK 62a09a30772141ef4add2f10d29927211abf57eb
Tree-SHA512: 74428180d57b4214c3d96963e6ff43e8778f6f23b6880262d1272f2de67d02714fdc3ebb558f62e48655b221a642c36f80ef37c8f89d362e2d66fd93cbf03b8f
cd37356ff9a1a3c2365c4fe3c716d1ca74185d73 [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 cd37356ff9a1a3c2365c4fe3c716d1ca74185d73
Tree-SHA512: e2165117bfbf7a031060e7376912f9af1c1bfc57916383799a0fa2c040e2caaab0d6aafc3425c083a233b96c84fafec75c938e00ceb6bd7d52607d58607cb145
The SHA256SUMS file can be used in a sha256sum -c command to verify
downloaded binaries. However users are likely to download just a single
file and not place this file in the correct directory relative to the
SHA256SUMS file for the simple verification command to work. By not
including the directory name in the SHA256SUMS file, it will be easier
for users to verify downloaded binaries.
Co-authored-by: Carl Dong <contact@carldong.me>
f12fbad5a1852706184082c68655fafad4bb9fe1 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 f12fbad5a1852706184082c68655fafad4bb9fe1
laanwj:
Code review ACK f12fbad5a1852706184082c68655fafad4bb9fe1
Tree-SHA512: b74a37cbba105d208d4da9264d295d7e052009fdd6b0ed54a0d9968bbe2deeba1766d6d310438b2939a81555faa0cbd67d5e53f0c8a2de669ce56353c1c67d22
68faa87881f5334b2528db4adc72ec19d94316a3 test: use f-strings in mining_*.py tests (fanquake)
c2a5d560df2824df5731100c2584e8ad7a3d7bc2 test: use f-strings in interface_*.py tests (fanquake)
86d958262dff43002820d58ccb8958e2dbfb9d5b test: use f-strings in feature_proxy.py (fanquake)
31bdb33dcb8345df1bb94b28e811252a918d7dcb test: use f-strings in feature_segwit.py (fanquake)
b166d54c3cbb0c028210cee977b3dcde5ac5474f test: use f-strings in feature_versionbits_warning.py (fanquake)
cf6d66bf941d946600047d712c7cd15d7605322e test: use f-strings in feature_settings.py (fanquake)
6651d77f22862716f5bd7d0b31cfbd3937ab7b1d test: use f-strings in feature_pruning.py (fanquake)
961f5813ba65b6a601081912c4ece96c2679794d test: use f-strings in feature_notifications.py (fanquake)
1a546e6f6ca95772f0d7dbc2792477becbb8ea63 test: use f-strings in feature_minchainwork.py (fanquake)
6679eceacc915a8ea7cd7063f103ffc5eb9da884 test: use f-strings in feature_logging.py (fanquake)
fb633933ab570e945d2a366f37eeff39f516c613 test: use f-strings in feature_loadblock.py (fanquake)
e9ca8b254d4b9567831c0e113ce1c0a2b4795a95 test: use f-strings in feature_help.py (fanquake)
ff7e3309995a8960ac371741b2b00c6da40f7490 test: use f-strings in feature_filelock.py (fanquake)
d5a6adc5e478fa5c6e562377eea873dc38e66578 test: use f-strings in feature_fee_estimation.py (fanquake)
a2de33cbdc79202bccddb4beadfde88266ac979f test: use f-strings in feature_dersig.py (fanquake)
a2502cc63fd308be8af840962da9c53339433fa6 test: use f-strings in feature_dbcrash.py (fanquake)
3e2f84e7a96cb4b97b609ac853f78edd0ed43f82 test: use f-strings in feature_csv_activation.py (fanquake)
e2f1fd8ee92fa421b6d293169044d6ddd5a9b8df test: use f-strings in feature_config_args.py (fanquake)
36d33d32b1b498b61f56d552f6e2c1d064f978c3 test: use f-strings in feature_cltv.py (fanquake)
dca173cc044270b30782b1e3355e9dcb8c534295 test: use f-strings in feature_blocksdir.py (fanquake)
5453e8706278918ac51a725e81599cfa18c8cdbc test: use f-strings in feature_backwards_compatibility.py (fanquake)
6f3d5ad67ac8e7b50abae1a2949898d858e38106 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 68faa87881f5334b2528db4adc72ec19d94316a3
Zero-1729:
crACK 68faa87881f5334b2528db4adc72ec19d94316a3
Tree-SHA512: d4e1a42e07d96d2c552387a46da1534223c4ce408703d7568ad2ef580797dd68d9695b8d19666b567af37f44de6e430e8be5db5d5404ba8fcecf9f5b026a6efb
5449d44e37982fcd5251fd47873c5f7d34c39fc9 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 5449d44e37982fcd5251fd47873c5f7d34c39fc9
Tree-SHA512: 3a26c5c3a5f2905fd0dd90892470e241ba625c0af3be2629d06d5da3a97534c1d6a55b796bbdd41e2e6a26a8fab7d981b98c45d4238565b0eb7edf3c5da02007
`bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)` is _only_
called from the tests, and the call to addr.Clear() only exists so that
a test that Clear() is called passes. Remove that test and the call.
Now that we manage the lifespan of node.addrman, we can just reset
node.addrman to a newly initialized CAddrMan if parsing peers.dat
fails, eliminating the possibility that Clear() leaves some old state
behind.
d8ba6327b21c755582ab3c83fc31bc6c523c41c3 scripted-diff: replace clientInterface with m_client_interface in net (fanquake)
f68c6cebe638cc3cf92f4e37650d37ea3899893a 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 d8ba6327b21c755582ab3c83fc31bc6c523c41c3, verified that `uiInterface` is replaced in all sites.
Zero-1729:
crACK d8ba6327b21c755582ab3c83fc31bc6c523c41c3
Tree-SHA512: 9c893d8799b0bc7737836c16e11b77b6f9dffa31041ec6678e63cad958ea06da09a841b99cc61c1b4d9f6f4f1be397ca5a8d2fb2fb7ab08c9437043f8a57c3dc
