💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2545151023)
nit in 6b9ce8cff58d7214fca51a8024ec4efb8694e059:

<details><summary>Could avoid repeating type name here and in other files.</summary>

```diff
diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
index 1e93b9c395..2f474c2411 100644
--- a/src/bench/addrman.cpp
+++ b/src/bench/addrman.cpp
@@ -24,7 +24,7 @@
static constexpr size_t NUM_SOURCES = 64;
static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;

-static NetGroupManager EMPTY_NETGROUPMAN{NetGroupManager::NoAsmap()};

...

💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2545416624)
Here's a reduced version:
```suggestion
return (std::to_integer<uint8_t>(bytes[(bytes.size() * 8 - bitpos) / 8]) >> ((7 + bitpos) % 8)) & 1;
```

---

I'm surprised by the way the shift amount changes for different bit positions. Would have expected something closer to this for big endian...

```suggestion
return (std::to_integer<uint8_t>(bytes[(bytes.size() * 8 - bitpos) / 8]) >> (7 - (bitpos % 8))) & 1;
```

....but it fails the unit tests.

Godbolt experiment https://go
...

💬 m3dwards commented on pull request "Clear out space on GHA jobs":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3558627387)
Hitting this again on my and @TheCharlatan's fork: https://github.com/m3dwards/bitcoin/actions/runs/19539048875/job/55940226257

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2546530265)
Deleting the conversions instead of adding a new method does not work because overload resolution considers these functions even though they are deleted and then the compiler errors that `copy` is ambiguous:

```
/Users/eugenesiegel/btc/bitcoin-clone/src/test/util/setup_common.cpp:210:9: error: call to 'copy' is ambiguous
210 | fs::copy(cached_dir, m_path_root, fs::copy_options::recursive);
| ^~~~~~~~
/Users/eugenesiegel/btc/bitcoin-clone/src/util/fs.h:142:13: note:
...

💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3558690701)
thank you for the reviews! I've pushed an [update](https://github.com/bitcoin/bitcoin/compare/016ab85a13408ad980c3dbce4e041e14a4fcf3b8..858f49bcb3590b211f014e8d290008bb13f5b17f) changing the long key names.
also retained the original key name in -addrinfo's result (`addresses_known`) in case some other scripts/tools rely on it.
```
$ build/bin/bitcoin-cli -addrinfo
{
"addresses_known": {
"ipv4": 52037,
"ipv6": 8855,
"onion": 3465,
"i2p": 0,
"cjdns": 0,
"tota
...

💬 ajtowns commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2546566487)
FYI, you could also make this a `string_view` literal, and leave the function as expecting a `span<const char>`, with

```c++
#include <string_view>

using namespace std::string_view_literals;

auto descs = Parse("pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)"sv, keys, err, /*require_checksum=*/false);
```

Using `string_view` in the function declaration seems better though.

📝 maflcko opened a pull request: "clang-format: Set Bitcoin Core IncludeCategories"
(https://github.com/bitcoin/bitcoin/pull/33917)
Replace the default llvm include categories with the ones specific to Bitcoin Core.

Ref: https://releases.llvm.org/17.0.1/tools/clang/docs/ClangFormatStyleOptions.html#includecategories

Also, format a file as example. To test this, the diff in src/test needs
to be reverted. Also `IncludeBlocks: Regroup` needs to be set. Then
`clang-format -i src/test/blockchain_tests.cpp` should recreate the
diff.

```diff
diff --git a/src/.clang-format b/src/.clang-format
index 15335fe9ae..57907909
...

💬 maflcko commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2546647446)
Done in https://github.com/bitcoin/bitcoin/pull/33917

💬 TheBlueMatt commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3558962920)
There was a long back-and-forth in the IRC meeting today on this topic, seemingly there's some very differing thoughts on high-level design for the block template IPC. IMO it makes sense to schedule a live call to discuss this so that we end up on the same page rather than going in circles in text-based communication.

💬 hebasto commented on issue "depends: fallback server missing Qt downloads":
(https://github.com/bitcoin/bitcoin/issues/33898#issuecomment-3559079200)
I forced fallback downloads using the following patch:
```diff
--- a/depends/funcs.mk
+++ b/depends/funcs.mk
@@ -36,8 +36,7 @@ define fetch_file_inner
endef

define fetch_file
- ( $(call fetch_file_inner,$(1),$(2),$(3),$(4),$(5)) || \
- $(call fetch_file_inner,$(1),$(FALLBACK_DOWNLOAD_PATH),$(4),$(4),$(5)))
+ $(call fetch_file_inner,$(1),$(FALLBACK_DOWNLOAD_PATH),$(4),$(4),$(5))
endef

# Shell script to create a source tarball in $(1)_source from local directory
```

Everythin
...

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3559086531)
[936bd87](https://github.com/bitcoin/bitcoin/commit/936bd874ac1581de532317741f5ab26e875935b3) to [55662b2](https://github.com/bitcoin/bitcoin/commit/55662b28e5c484ad161eb4293b582dca389d176f): rebased

FYI https://github.com/bitcoin/bitcoin/commit/c76797481155754329ec6a6f58e8402569043944 removed the `std::move` in `coinstatsindex.cpp` but not in `blockfilterindex.cpp`, which is another reason to deduplicate this.

💬 fanquake commented on issue "depends: fallback server missing Qt downloads":
(https://github.com/bitcoin/bitcoin/issues/33898#issuecomment-3559092421)
Seems like `https://code.qt.io` also doesn't work with (some?) VPN providers, so we should probably move away from using that.

💬 hebasto commented on issue "depends: fallback server missing Qt downloads":
(https://github.com/bitcoin/bitcoin/issues/33898#issuecomment-3559121324)
> Seems like `https://code.qt.io` also doesn't work with (some?) VPN providers, so we should probably move away from using that.

I can see two options:
1. Switch to https://raw.githubusercontent.com/qt/qt5.
2. Treat the three files as patches.

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3559158385)
> Since `DBHeightKey` and `DBHashKey` are pretty similar. What do you think about a generic template for both? [furszy@e5dd2b0](https://github.com/furszy/bitcoin-core/commit/e5dd2b02f15f9f68fbe84cc30255573ad23dbf2c). (Only needed to specialize the serialization for the value)

That's clever, but I'm not convinced it is worth the downside of more complex / less readable code - we just have two different Key types, and I don't think it's likely that there will be other added in the future. Thoug
...

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3559168135)
Rebased.

💬 hebasto commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3559173941)
Friendly ping @davidgumberg @hodlinator :)

💬 enirox001 commented on pull request "test: Retry download in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/33915#discussion_r2546937787)
nano nit: The single retry seems reasonable to start with. If CI flakiness persists after this change, increasing to 2-3 retries or adding exponential backoff can be considered

✅ fanquake closed an issue: "ci: windows-native-dll-vcpkg-* cache does not work?"
(https://github.com/bitcoin/bitcoin/issues/33685)

🚀 fanquake merged a pull request: "ci: Consistenly only cache on the default branch"
(https://github.com/bitcoin/bitcoin/pull/33905)

🤔 enirox001 reviewed a pull request: "test: Retry download in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/33915#pullrequestreview-3488992622)
ACK fad06f3

This looks good to me. I tested locally by running the script with
valid releases (v25.0, v24.0.1) and an invalid one (v0.1.0) to verify
the retry logic works correctly. The retry mechanism should significantly
reduce CI failures from transient network issues.

optional nit: Consider adding an explicit timeout to `urlopen()`
(e.g., `timeout=60`) to fail faster in case of network hangs, rather
than relying on OS socket defaults. This would make the script more
predict
...