9620cb449374f234f72c1a9e1bae3d4b8c0ff171 assumeutxo: fail early if snapshot block hash doesn't match AssumeUTXO parameters (Sebastian Falbesoner)
Pull request description:
Right now the `loadtxoutset` RPC call treats literally all files with a minimum size of 40 bytes (=size of metadata) as potential valid snapshot candidates and the waiting loop for seeing the metadata block hash in the headers chain is always entered, e.g.:
```
$ ./src/bitcoin-cli loadtxoutset ~/.vimrc
<wait>
bitcoind log:
...
2023-10-15T14:55:45Z [snapshot] waiting to see blockheader 626174207465730a7265626d756e207465730a656c62616e65207861746e7973 in headers chain before snapshot activation
...
```
There is no point in doing any further action though if we already know from the start that the UTXO snapshot loading won't be successful. This PR adds an assumeutxo parameter check immediately after the metadata is read in, so we can fail immediately on a mismatch:
```
$ ./src/bitcoin-cli loadtxoutset ~/.vimrc
error code: -32603
error message:
Unable to load UTXO snapshot, assumeutxo block hash in snapshot metadata not recognized (626174207465730a7265626d756e207465730a656c62616e
65207861746e7973)
```
This way, users who mistakenly try to load files that are not snapshots don't have to wait 10 minutes (=the block header waiting timeout) anymore to get a negative response. If a file is loaded which is a valid snapshot (referencing to an existing block hash), but one which doesn't match the parameters, the feedback is also faster, as we don't have to wait anymore to see the hash in the headers chain before getting an error.
This is also partially fixes#28621.
ACKs for top commit:
maflcko:
lgtm ACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171
ryanofsky:
Code review ACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171. This should fix an annoyance and bad UX.
pablomartin4btc:
tACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171
Tree-SHA512: f88b865e9d46254858e57c024463f389cd9d8760a7cb30c190aa1723a931e159987dfc2263a733825d700fa612e7416691e4d8aab64058f1aeb0a7fa9233ac9c
faa5e061c2f63b868367015561fd376b6ef6052c fuzz: Allow multiple --m_dir args (MarcoFalke)
Pull request description:
This allows to merge the result from several servers (or just several folders) at the same time, instead of having to iterate over them.
This should also allow the fuzz engine (libFuzzer) to optimize the final merge result more, because all fuzz inputs from all folders are available at the same time.
ACKs for top commit:
dergoegge:
tACK faa5e061c2f63b868367015561fd376b6ef6052c
Tree-SHA512: bf0da418b1f7b8a8af16bb7cc1e148b1ccd0f17062ce70758d1ca5b35c3eee77c0c30377d376befdd55480adfd1f1a1073cfc47118e7a710e6760e020abe24bb
348e79f7c6030c9bcbec6bd25e246a779081152a lint: Include test_utxo_snapshots in lint_shell (Fabian Jahr)
Pull request description:
jamesob excluded `test_utxo_snapshots.sh` from the shell linter with this explanation: "Add the script to the shellcheck exception list since the quoted variables rule needs to be violated in order to get bitcoind to pick up on $EARLY_IBD_FLAGS." However, macrofake pointed out that single lines can be excluded from linting.
This fixes one fixable rule violation, excludes the rest of the offending lines from the linter and then removes the exclusion of the `test_utxo_snapshots.sh` file. Also adds documentation.
ACKs for top commit:
Empact:
ACK 348e79f7c6030c9bcbec6bd25e246a779081152a
maflcko:
lgtm ACK 348e79f7c6030c9bcbec6bd25e246a779081152a
pablomartin4btc:
tACK 348e79f7c6030c9bcbec6bd25e246a779081152a
Tree-SHA512: a904cc1cc3c94488dfbd39ea69a3ef17259f991708a797009001669448fef81eed086ecbce1ec433988d88baef293849698e2e0eb86a969b949cc7ef93af7b4b
092daa2f9524e371ee6b505519d9b722a2c6361a contrib: add test for macOS linker version to symbol-check (fanquake)
cefbf0bc20dfe932240877b3096300c8c241ad61 depends: update LD64_VERSION to 711 (fanquake)
Pull request description:
I forgot to do this in 7d5815293e.
Add a test so it's impossible to forget.
ACKs for top commit:
TheCharlatan:
utACK 092daa2f9524e371ee6b505519d9b722a2c6361a
achow101:
ACK 092daa2f9524e371ee6b505519d9b722a2c6361a
jarolrod:
ACK 092daa2f9524e371ee6b505519d9b722a2c6361a
hebasto:
ACK 092daa2f9524e371ee6b505519d9b722a2c6361a.
laanwj:
ACK 092daa2f9524e371ee6b505519d9b722a2c6361a
Tree-SHA512: 37f0bdfd6607a7760eabe5efe279532ba0c59c0915161e08d5e3b9a0b7705839d62537d6e17406062f6a0a1db5407575da7cd671e9cb916e422e77f5649c6e2b
fa05a726c225dc65dee79367bb67f099ae4f99e6 tidy: modernize-use-emplace (MarcoFalke)
Pull request description:
Constructing a temporary unnamed object only to copy or move it into a container seems both verbose in code and a strict performance penalty.
Fix both issues via the `modernize-use-emplace` tidy check.
ACKs for top commit:
Sjors:
re-utACK fa05a726c2
hebasto:
ACK fa05a726c225dc65dee79367bb67f099ae4f99e6.
TheCharlatan:
ACK fa05a726c225dc65dee79367bb67f099ae4f99e6
Tree-SHA512: 4408a094f406e7bf6c1468c2b0798f68f4d952a1253cf5b20bdc648ad7eea4a2c070051fed46d66fd37bce2ce6f85962484a1d32826b7ab8c9baba431eaa2765
8b6470a90652fcffc45b8d7998af7c8ad6251332 gui: disable top bar menu actions during shutdown (furszy)
7066e8996d0ac090535cc97cdcb54a219986460f gui: provide wallet controller context to wallet actions (furszy)
Pull request description:
Small follow-up to #751.
Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list.
Future Note:
This surely happen in other places as well, we should re-work the way we connect signals. Register
lambas without any precaution can leave dangling pointers.
ACKs for top commit:
hebasto:
ACK 8b6470a90652fcffc45b8d7998af7c8ad6251332, I've tested each commit separately on macOS Sonoma 14.0 (Apple M1).
Tree-SHA512: 6fbd1bcd6717a8c1633beb9371463ed22422f929cccf9b791ee292c5364134c501e099329cf77a06b74a84c64c1c3d22539199ec49ccd74b3950036316c0dab3
All callers of `LookupSubNet()` need the result to be of CJDNS type if
`-cjdnsreachable` is set and the address begins with `fc`:
* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails
to white list CJDNS addresses: when a CJDNS peer connects to us, it
will be matched against IPv6 `fc...` subnet and the match will never
succeed.
* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in
`banlist.json`. Upon reading from disk they have to be converted back
to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6)
would not match a peer (`fc00::1`, CJDNS).
* `setban()` (in `rpc/net.cpp`): otherwise `setban fc.../mask add` would
add an IPv6 entry to BanMan. Subnetting does not make sense for CJDNS
addresses, thus treat `fc.../mask` as invalid `CSubNet`. The result of
`LookupHost()` has to be converted for the case of banning a single
host.
* `InitHTTPAllowList()`: not necessary since before this change
`-rpcallowip=fc...` would match IPv6 subnets against IPv6 peers even
if they started with `fc`. But because it is necessary for the above,
`HTTPRequest::GetPeer()` also has to be adjusted to return CJDNS peer,
so that now CJDNS peers are matched against CJDNS subnets.
e6e444c06cbf09380f9924dff3d21c1be15d1753 refactor: add and use EnsureAnyAddrman in rpc (stratospher)
bf589a50a0d6a7b94f1ba1ddf24a1497fd35ad44 doc: add release notes for #27511 (stratospher)
3931e6abc39b8aee1472028dbf76eeb10708d2b4 rpc: `getaddrmaninfo` followups (stratospher)
Pull request description:
- make `getaddrmaninfo` RPC public since it's not for development purposes only and regular users might find it useful. [#26988 (comment)](https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1738371584)
- add missing `all_networks` key to RPC help. [#27511 (comment)](https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1335084087)
- fix clang format spacing
- add and use `EnsureAddrman` in RPC code. [#27511 (comment)](https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1331501491)
ACKs for top commit:
0xB10C:
Code Review re-ACK e6e444c06cbf09380f9924dff3d21c1be15d1753
theStack:
Code-review ACK e6e444c06cbf09380f9924dff3d21c1be15d1753
pablomartin4btc:
tested ACK e6e444c06cbf09380f9924dff3d21c1be15d1753
Tree-SHA512: c14090d5c64ff15e92d252578de2437bb2ce2e1e431d6698580241a29190f0a3528ae5b013c0ddb76a9ae538507191295c37cab7fd93469941cadbde44587072
fa858d63a0a5d794aab38c26f60c593513fe08de fuzz: Merge with -set_cover_merge=1 (MarcoFalke)
Pull request description:
This should be less controversial than commit 151a2b189c3561dda2bb7de809306c1cfeb40e23. The overall size of the qa-assets repo is reduced further from 1.9GB to 1.6GB. Also, the runtime to iterate on the resulting folder is reduced further from ~1699s to ~1149s (N=1).
ACKs for top commit:
murchandamus:
crACK fa858d63a0a5d794aab38c26f60c593513fe08de
dergoegge:
ACK fa858d63a0a5d794aab38c26f60c593513fe08de
Tree-SHA512: e23fa93bd48f01d11c551b035004c678bd6d76bc24ac7d0d0a7883060804e6711763cbd0cd0ded3aad3e4c40da764decae81c2703388cc11961def3c89a4f9ba
e44c574650827f18e12ac0ba378c0a19d23a07b4 ci: always prune all dangling bitcoin-ci-test images (stickies-v)
ce1699706e98201db73209ee495051359b1d0703 ci: add label to docker images (stickies-v)
Pull request description:
Follow-up from https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210209382.
Labeling the docker images produced by the CI allows us/the user to apply batch operations to all images (including dangling ones) produced by the ci without affecting other, non-bitcoin-ci images. With labeling, we can safely always prune dangling bitcoin-ci-test images without checking for `RESTART_CI_DOCKER_BEFORE_RUN`, which we enable on our persistent runners.
ACKs for top commit:
fanquake:
utACK e44c574650827f18e12ac0ba378c0a19d23a07b4
Tree-SHA512: 1009fb1be78fbc80b5341ba92eac2991e77d050e1ab6048d1d9a65af73413a6be7afc1e1c764eb3f347f363af31245b93fdb38f6ac016d775aad4a0f36e4c98f
Opening the top bar menu when the app is being destroyed
freezes the GUI shutdown process for no reason. No menu
action can be executed.
Note:
This behavior is consistent with how the tray icon menu
is cleared too.
ac4caf3366a85617641394a97aa9f029550d77d4 test: fix `assert_debug_log` call-site bugs, add type checks (Sebastian Falbesoner)
Pull request description:
Two recently added tests (PR #28625 / commit 2e31250027ac580a7a72221fe2ff505b30836175 and PR #28634 / commit 3bb51c29df596aab2c1fde184667cee435597715) introduced bugs by wrongly using the `assert_debug_log` helper:
5ea4fc05ed/test/functional/feature_assumeutxo.py (L84-L85) (already fixed in https://github.com/bitcoin/bitcoin/pull/28639)
5ea4fc05ed/test/functional/p2p_v2_transport.py (L148)5ea4fc05ed/test/functional/p2p_v2_transport.py (L159)
Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. Thanks to maflcko for discovering: https://github.com/bitcoin/bitcoin/pull/28625#discussion_r1356489861
In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists, as discussed in https://github.com/bitcoin/bitcoin/pull/28625#discussion_r1356864233. Using mypy might be an alternative, but I guess it takes quite a bit of effort to properly integrate this into CI for the whole functional test suite (including taking care of false-positives), so I decided to go with the simpler "manual asserts" hack. Suggestions are very welcome of course.
ACKs for top commit:
achow101:
ACK ac4caf3366a85617641394a97aa9f029550d77d4
maflcko:
lgtm ACK ac4caf3366a85617641394a97aa9f029550d77d4
dergoegge:
ACK ac4caf3366a85617641394a97aa9f029550d77d4
Tree-SHA512: a9677af76a0c370e71f0411339807b1dc6b2a81763db4ec049cd6d766404b916e2bdd002883db5a79c9c388d7d8ebfcbd5f31d43d50be868eeb928e3c906a746
Two recently added tests (PR #28625 / commit 2e31250027ac580a7a72221fe2ff505b30836175
and PR #28634 / commit 3bb51c29df596aab2c1fde184667cee435597715)
introduced a bug by wrongly using the `assert_debug_log` helper.
Instead of passing the expected debug string in a list as expected, it
was passed as bare string, which is then interpretered as a list of
characters, very likely leading the debug log assertion pass even if the
intended message is not appearing.
In order to avoid bugs like this in the future, enforce that the
`{un}expected_msgs` parameters are lists.
Since all bitcoin-ci-test images are now labeled, we can always
prune all dangling images, regardless of whether we are in
RESTART_CI_DOCKER_BEFORE_RUN.
To be safe, still prune all images if RESTART_CI_DOCKER_BEFORE_RUN
in case the filtering doesn't work, or if images were created on
an earlier version that did not assign labels.
This reverts commit 057750c09d0a8331c33966d2cc2285ef82f08af8.
It is not needed anymore in the GHA CI.
This change will make the code much simpler in the following commit.
This allows us or the user to perform batch operations on all
images produced by the ci, e.g. to prune all dangling images,
without affecting non-ci images.
36a3004a41aea58f50f3348c5de4eb5a23268788 devtools: test_utxo_snapshots.sh sleep cleanup and documentation (Fabian Jahr)
Pull request description:
There were bare sleep statements in the script where it was unclear why they were needed and I think they could also be replaced by existing helpers.
ACKs for top commit:
Sjors:
utACK 36a3004a41aea58f50f3348c5de4eb5a23268788
pablomartin4btc:
utACK 36a3004a41
Tree-SHA512: b6e2fc69cd7babcfa7f413f11304f4d648d6d64b3b526862664ccedb0016ad182b9e886aa4e8c33315e4c18824512e11a4fd6365f9c3c95093967d3ef7687e62
61a6c3b0e9a8dab5c5f845af4becde817539133c build: add `-mbranch-protection=bti` to aarch64 hardening flags (fanquake)
Pull request description:
This is a simpler (less hardening) version of https://github.com/bitcoin/bitcoin/pull/24123.
You can inspect binaries using `readelf -n`, and look for BTI in a `.note.gnu.property`. i.e
```bash
readelf -n src/bitcoin-cli
Displaying notes found in: .note.gnu.property
Owner Data size Description
GNU 0x00000010NT_GNU_PROPERTY_TYPE_0
Properties: AArch64 feature: BTI
```
Related to https://github.com/bitcoin/bitcoin/issues/19075.
ACKs for top commit:
TheCharlatan:
utACK 61a6c3b0e9a8dab5c5f845af4becde817539133c
Tree-SHA512: 64504de44e91d853165daf4111dca905d8eb9ef3f4bfb0d447c677b02c9100dbd56f13e6fe6539fb06c2343a094229591ac5d1bd9e184b32b512c0ac3f9bac36
fa2c894cbb41a64371717139fb3c3ddfb9bb8b19 ci: move-only CI_CONTAINER_ID to 02_run_container.sh (MarcoFalke)
fa695b4df069425414fd26b2ddc08d72a6b506f6 ci: Work around podman stop bug (MarcoFalke)
fa09a031c1eb8abcb9a04cacdf5629f95ffc77f8 ci: Add set -ex to 02_run_container.sh (MarcoFalke)
fac9abbf475a1de6f9f39ddede9a6a59bbd1cff4 ci: Rename 04_install to 02_run_container (MarcoFalke)
Pull request description:
Sometimes, it seems that `podman stop` does not work. Presumably, it falls back to `podman kill`, which is async.
Try to work around this intermittent issue by using the `rm --force` over `stop`.
Example failing log https://cirrus-ci.com/task/4549784611061760?logs=ci#L238:
```
Restart docker before run to stop and clear all containers started with --rm
++ podman container stop --all
e4eca0766f87864d89fc230aa884a238c214cfbcd44cf76a4dbdb2a30c982009
++ echo 'Prune all dangling images'
Prune all dangling images
++ docker image prune --force
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
+++ docker run --cap-add LINUX_IMMUTABLE --rm --interactive --detach --tty --mount type=bind,src=/tmp/cirrus-build-1970593815,dst=/tmp/cirrus-build-1970593815,readonly --mount type=volume,src=ci_macos_cross_ccache,dst=/tmp/ccache_dir --mount type=volume,src=ci_macos_cross_depends,dst=/ci_container_base/depends --mount type=volume,src=ci_macos_cross_previous_releases,dst=/ci_container_base/prev_releases --env-file /tmp/env --name ci_macos_cross ci_macos_cross
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
time="2023-09-27T20:55:39Z" level=warning msg="The input device is not a TTY. The --tty and --interactive flags might not work properly"
Error: creating container storage: the container name "ci_macos_cross" is already in use by e4eca0766f87864d89fc230aa884a238c214cfbcd44cf76a4dbdb2a30c982009. You have to remove that container to be able to reuse that name: that name is already in use
ACKs for top commit:
hebasto:
ACK fa2c894cbb41a64371717139fb3c3ddfb9bb8b19, I have reviewed the code and tested it locally.
Tree-SHA512: 31fca340c6bedaadf4dd51fa745d9b3969042cebc0c7c904ef18af3f2f986039ec4354ccdff1422fbf77cf223e4423857368dce53cfa67ef15c76b78d007eace
faa190b1efbdfdb9b12a7bfa7f732b5471a02e64 test: Fuzz merge with -use_value_profile=0 for now (MarcoFalke)
Pull request description:
Seems odd that this has to be done, but for now there are (unknown) size limits on the qa-assets repo. Also, a larger size means that cloning and iterating over the files takes a longer time.
Not sure how to measure the net impact of this, but with some backups reverting this commit, it can be limited on the downside?
ACKs for top commit:
dergoegge:
ACK faa190b1efbdfdb9b12a7bfa7f732b5471a02e64
Tree-SHA512: 9f8b3f4526f60e4ff6fca97859a725d145a8339c216bd15c92fad7e53f84308745fee47727527de459c0245ef9d474a9dc836fee599ab2b556b519bd900b9a33
fa2843eba4f195fcc9fdda2d3673fae0d7fc6282 ci: Bump asan (MarcoFalke)
Pull request description:
Needed to bump the EOL date and unlock clang-17.
ACKs for top commit:
fanquake:
ACK fa2843eba4f195fcc9fdda2d3673fae0d7fc6282
Tree-SHA512: 7d8b3b30ed65bd26f9640db8d06de8c27a9ad8a8160f9e645a97b99df867aa30508491dd8957a54edad724f8672ef1054041106d90ee826ba3e856176ab52afc
79789ccafe101d3bb05fffe08610d2103e3f3060 ci: Drop no longer needed `NOLINTNEXTLINE` (Hennadii Stepanov)
Pull request description:
After recent tool updates in the "tidy" CI task, the one instance of `NOLINTNEXTLINE` is not required anymore.
ACKs for top commit:
fanquake:
ACK 79789ccafe101d3bb05fffe08610d2103e3f3060
Tree-SHA512: 382f3e0570f36a481498ac118ee30a34aabfd6285b21a67c19c74b240305315c5a118fb70f172b843a086262433462d45998e78306697be0ddfb20094733ee98
