💬 brunoerg commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#issuecomment-3558285614)
Coverage report is available at: https://brunoerg.xyz/bitcoin-core-coverage/33916/

👍 hebasto approved a pull request: "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`"
(https://github.com/bitcoin/bitcoin/pull/33824#pullrequestreview-3488101273)
ACK fae83611b8ef358ea7aca7070fd7e82dc06f9755, I have reviewed the code and it looks OK.

🚀 hebasto merged a pull request: "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`"
(https://github.com/bitcoin/bitcoin/pull/33824)

🤔 hebasto reviewed a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851#pullrequestreview-3488133192)
My Guix build:
```
aarch64
17cd36aa7eaae8cbe242efa364e55699b1568a6d0cb2c2e1ac82781b46a5f174 guix-build-f541b92cf2bb/output/aarch64-linux-gnu/SHA256SUMS.part
69985b56e1c6ff521f4f49ba102cb53275106dfd076be98361068ef113379fa3 guix-build-f541b92cf2bb/output/aarch64-linux-gnu/bitcoin-f541b92cf2bb-aarch64-linux-gnu-debug.tar.gz
4feb585aaf30ecc8b8c1c1fa32e4a8fa9e7f3e67286825e006807f714f15b039 guix-build-f541b92cf2bb/output/aarch64-linux-gnu/bitcoin-f541b92cf2bb-aarch64-linux-gnu.tar.gz
43b3f41f64a01f
...

🤔 vasild reviewed a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3488156994)
Approach ACK 9f6016c57440041945ace0639d2d97bc87ea6876

💬 vasild commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2546366108)
In the commit message of a80521bce064f9a3cf6a349db4b6ee9fd50a4e2f `optimization: Introduce PresaltedSipHasher for repeated hashing`: `build/src/bench/bench_bitcoin` should be changed to `build/bin/bench_bitcoin`.

The tests `SaltedOutpointHasherBench*` do not exist?

💬 vasild commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2546273279)
In `master`, the `shorttxidk0` and `shorttxidk1` members are default initialized to `0` when the object is created. Then they are assigned in `CBlockHeaderAndShortTxIDs::FillShortTxIDSelector()` and eventually used in `CBlockHeaderAndShortTxIDs::GetShortID()`.

This means that (in `master`) a call to `GetShortID()` without a preceding call to `FillShortTxIDSelector()` is well defined and will use `shorttxidk...` as `0`.

Now, with these changes such an usage would trigger this newly added `Asser
...

💬 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