💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514815293)
nit: `NO` might work, but isn't `OFF` a clearer inversion of `ON`?
```suggestion
export BITCOIN_CONFIG="-DREDUCE_EXPORTS=ON -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_KERNEL_TEST=ON -DBUILD_SHARED_LIBS=ON -DWITH_EMBEDDED_ASMAP=OFF"
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514782960)
Why change this? Various tools expect an empty line before EOF (although I admit it's probably rarely relevant for these generated files).

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2515345175)
nit: Could replace `for`s with `copy_n()`:
```diff
--- a/src/netgroup.cpp
+++ b/src/netgroup.cpp
@@ -89,9 +89,8 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const
std::vector<std::byte> ip_bits(16);
if (address.HasLinkedIPv4()) {
// For lookup, treat as if it was just an IPv4 address (IPV4_IN_IPV6_PREFIX + IPv4 bits)
- for (int8_t byte_i = 0; byte_i < 12; ++byte_i) {
- ip_bits[byte_i] = std::byte(IPV4_IN_IPV6_PREFIX[byte_i]);
-
...

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2515564991)
nit: Align with style of prior line (`sep_pos`)?
```suggestion
const size_t ip_len{buffer.size() - sep_pos - 1};
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514789998)
A variant of `BitsToBytes` from src/merkleblock.cpp already exists, but it seems okay to make more modern divisionless version (albeit dividing by 8 probably gets optimized as 3 right shifts). I understand not wanting to touch & reuse consensus code for now.

nit: Clearer parameter name?
```suggestion
std::vector<std::byte> BitsToBytes(std::span<const uint8_t> bits) noexcept
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2522389508)
Side note: the documentation below indicates that a failure should be returned if no default ASN has been set, but the current code (also on master) returns zero:
https://github.com/bitcoin/bitcoin/blob/e00deb2cfa60e846ea3de48bed35c7d3992db4bd/contrib/asmap/asmap.py#L158-L165

I think it would be good to update the documentation or the code to match, but understand if others don't want to do it in the context of this PR.

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2515426420)
nit: Could replace `for` with:
```C++
std::ranges::copy(std::as_bytes(buffer.subspan(1, asmap_size)), std::back_inserter(asmap));
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2522547981)
remark in 35e6c5bdd467e8342ed606f338c3b4a363c2ef35 - "refactor: Operate on bytes instead of bits in Asmap code":
#### Endian-inversion on a bit-level

The old `vector<bool>` data on master converts to `"dfc0...."_hex` (`df` being the bit-endian-inversion of `fb`, and `c0` being the inversion of `03`).
But the old code would endian-invert each byte while loading from disk or from a byte-buffer:

https://github.com/bitcoin/bitcoin/blob/8f7e02a08626a3141b64df09dfeb38d7d9e2974c/src/util/asmap.
...

💬 purpleKarrot commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536621101)
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1

Thanks for working on this!

💬 purpleKarrot commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#issuecomment-3536624391)
ACK 0dd8d5c237e2fdb0168d9ca644e7f2f5a8b6ed72

💬 purpleKarrot commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2529975644)
> it returns the path to the “Release” plugin when building with `--config Debug`.

Interesting. I put that in my backlog. Low priority, because your approach is working. I just want to know why the `LOCATION` property is not set correctly.

💬 optout21 commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2529976285)
An off-topic nit: the `current_tip` value technically can be nullptr, and this is not checked here and not handled inside `GetFirstBlock`. However, this can happen only on an empty chain, and the behavior is not touched by this PR.

💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3536633344)
This occurred to me while [refactoring](https://github.com/stringintech/go-bitcoinkernel/pull/9) the context and chainstate manager options in the Go wrapper, so I opened this PR to get feedback.

cc @TheCharlatan @stickies-v

🤔 optout21 reviewed a pull request: "refactor: remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3468175235)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Review, local build, unit tests.
The change has two small code-health improvements; first concerns compiler lifetime hints and can be validated by code review. The other change -- change of return value from pointer to reference reduces chance of null pointer mishandling. The affected method is used in a few places, all are checked, so the impact is minimal.

🚀 hebasto merged a pull request: "cmake: Specify Windows plugin path in `test_bitcoin-qt` property"
(https://github.com/bitcoin/bitcoin/pull/33865)

💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2529998066)
Can a check on `dirty_count` be added after the `BatchWrite` below?

💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2530000189)
Misleading comment, no calculation takes place here, it's just an accessor.

💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2530010319)
I suggest moving this line one line down, such that `m_dirty_count` and `cachedCoinsUsage` updates are on adjacent lines, as they are logically related.

💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#issuecomment-3536685676)
A general observation: keeping a count like the dirty count is very efficient, but prone to going out of sync, e.g with future incorrect code changes. For eg., a missing decrement can result in the value creeping higher, a missing increment can result the value going lower, event to underflow -- `size_t` is unsigned. One mitigation is to create (inlinable) inc/dec methods, with sanity checks (in debug mode); pre-decrease the value cannot be 0, pre-increase it cannot be equal to the total entries
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3536696051)
What about capping it to min of 8 or `-par` value if set to something other than 0. If `-par` is 0, set to 8? I think extra threads will help here even on a single core system.