💬 fanquake commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#discussion_r1728767510)
I'll take a look, and we can backport something if relevant.

💬 fanquake commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2306840123)
> the statement from the PR description:
> is not accurate.

That text is from the GCC documentation, and in this context, of, the compiler being configured for CET, after configuring with `--enable-cet`, as far as I'm aware, it is accurate. If you think it isn't, can you file an issue upstream, and link it here it?

The point of this PR has never been to add any additional metadata to the binaries/make any other changes, the point is just to explictly use the hardening option when configur
...

💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306867512)
> Yeah it might be too much zeal, yet I’ve been surprised when the option was introduced with `24.x` how much services were still claiming to rely zero-conf acceptance.

Only one service, GAP600, claimed to still rely on zero-conf and they refused to actually provide any examples of clients actually using them. I think it's highly likely that GAP600 was actually a scam that took peoples' money and provided nothing of value. Us continuing to claim that zeroconf services still exist is actually
...

💬 fanquake commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306877771)
I'm just going to backport the other two fixes here, and will follow up with any other changes before the next 27.x.

💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306879047)
@fanquake That's perfectly reasonable. Concept ACK.

👍 stickies-v approved a pull request: "[27.x] Even more backports"
(https://github.com/bitcoin/bitcoin/pull/30558#pullrequestreview-2257069799)
ACK b06c4c6550545351610fc3278dffdd63d5954cf8

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728863544)
```suggestion
const std::unique_ptr<TxDownloadManagerImpl> m_impl;
```

👉👈

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728864722)
```suggestion
FUZZ_TARGET(txdownloadman_impl, .init = initialize)
```

(if you take the other naming suggestion)

👉👈

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728864031)
```suggestion
FUZZ_TARGET(txdownloadman_one_honest_peer, .init = initialize)
```

👉👈

👍 fanquake approved a pull request: "test: Enable detect_leaks=1 in ASAN_OPTIONS explicitly"
(https://github.com/bitcoin/bitcoin/pull/30665#pullrequestreview-2257083551)
ACK cccc5bfd35a008adf08d99ed463fe00d6a6f29c0

🚀 fanquake merged a pull request: "test: Enable detect_leaks=1 in ASAN_OPTIONS explicitly"
(https://github.com/bitcoin/bitcoin/pull/30665)

📝 maflcko opened a pull request: "test: Avoid duplicate curl call in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/30703)
Seems odd having to translate `404` to "Binary tag was not found". Also, it seems odd to write a for-loop over a list with one item.

Fix both issues by just using a single call to `curl --fail ...`.

Can be tested with: `test/get_previous_releases.py -b v99.99.99`

Before:

```
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-99.99.99/bitcoin-99.99.99-x86_64-linux-gnu.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
...

🤔 dergoegge reviewed a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2257087050)
Code review ACK 4e87dcc205ba2bb7996dc077ad46b1b1e309dd8e

[Coverage report](http://bitcoind-fuzz.dergoegge.de:8000/bitcoin-pr30661/harnesses/p2p_headers_sync/coverage_report/coverage/workdir/bitcoin/index.html).

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1728873486)
I think the use of `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` is a good approach. `CheckProofOfWorkImpl` will always be a blocker in fuzz tests, so getting it out of the way for all harnesses that aren't testing the check itself makes sense.

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728880703)
Any reason not to let the fuzzer decide the order?

If the order is interesting (i.e. a specific order might trigger a bug) then I think it'd make more sense to let the fuzzer choose the order.

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728869229)
nit: requires `#include <util/check.h>`

👍 stickies-v approved a pull request: "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2257080546)
ACK 2cba0bed5be8a25e1635b7cde0a98285f64b5469, left non-blocking nits but happy to quickly re-ACK.

Would suggest updating the commit message to describe the change a bit better, add `bugfix` to the title, and remove the double `[]` and `:` notation:

> wallet: bugfix: lock wallet context before adding/removing wallet settings
>
> When performing certain wallet operations simultaneously, a race condition can be
> triggered. For example, simultaneously loading two wallets can lead to only
...

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728873933)
```suggestion
// concurrently, ensuring no race conditions occur during either process.
```

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728871804)
nit: while touching adjacent line, could help a neighbour out
```suggestion
#include <cstdint>
#include <future>
#include <memory>
#include <thread>
```

💬 fanquake commented on pull request "guix: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-2306986439)
Rebased on master and dropped a commit, also bumped the glibc 2.33 branch to the latest commit. Still based on #30433, but the main blocker here remains the glibc bump.