💬 vasild commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2546377030)
Would be nice to wrap the commit mesage lines to 72 chars.

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3558487859)
Only rebased again

👍 hebasto approved a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851#pullrequestreview-3488345544)
ACK f541b92cf2bb7f8f708f147bd320becf48587d14, due to the last commit, I also tested an LTO build on Ubuntu 25.10 using GCC 15.2.0.

🚀 hebasto merged a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851)

🤔 hodlinator reviewed a pull request: "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation"
(https://github.com/bitcoin/bitcoin/pull/33878#pullrequestreview-3486433210)
Reviewed b41f5a29f7d56da8fd157770ad29b5776c3684c6

Sorry for not finding these earlier, getting closer to A-C-K.

💬 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_r2545107996)
in 6b9ce8cff58d7214fca51a8024ec4efb8694e059 on `CheckAsmap`:
1. Justification for existence: I dislike default parameter values, so appreciate a distinct function from `SanityCheckASMap`.
2. Comment: Nothing makes it specialized for only embedded data?
3. Location: Maybe it could be added directly after `SanityCheckASMap` in both .h + .cpp since they are so closely related? (Could place it before but that would put it between `Interpret` and `SanityCheckASMap` which are nice to have after eac
...

💬 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_r2545022654)
meganit in d278d6193b8d754eb108bc3ccc256260085b32c9 "refactor: Unify asmap version calculation and naming":
As `uint256 AsmapVersion(const std::vector<std::byte>& data)` is new in this PR, it could be introduced as `uint256 AsmapVersion(std::span<const std::byte> data)` from the beginning to avoid line churn.

💬 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_r2546283406)
nit: In other cases we have
* `DecodeBits(size_t& bitpos, const std::span<const std::byte>& data...` and
* `DecodeType(size_t& bitpos, const std::span<const std::byte>& data)`
* ...

So it might be nice to have the same argument order here:
```suggestion
inline bool GetBitLE(uint32_t bitpos, std::span<const std::byte> bytes) noexcept
{
return (std::to_integer<uint8_t>(bytes[bitpos / 8]) >> (bitpos % 8)) & 1;
}

/**
* Extract a single bit from byte array using big-endian bit ord
...

💬 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_r2546124184)
nit: Here are some changes that bring the `Interpret()` implementation even closer to `SanityCheckAsmap()` while also decreasing variable scope, making the code more readable. Maybe something to include in the last commit which aligns the casing of `SanityCheckAsmap`?
```diff
--- a/src/util/asmap.cpp
+++ b/src/util/asmap.cpp
@@ -183,18 +183,16 @@ uint32_t Interpret(const std::span<const std::byte> asmap, const std::span<const
const size_t endpos{asmap.size() * 8};
uint8_t bits = ip.siz
...

💬 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_r2544942080)
Thank you! In adfd800626a7910a603c34ab733fc1e91d438bc5 the `ASMAP_DATA` type is still unnecessarily changed from `constexpr auto` to `static const auto`.

💬 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
...