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

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3536862669)
> although I would also consider peeling off bits->bytes refactor and span-usage to its own PR

It's a good idea, I will take it especially because I have been working on improved documentation for the asmap internals and this would go along nicely with this part of the original PR, I think. Since I started this restructuring yesterday already I will push this soon without your latest review here addressed: https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895 (thank you f
...

📝 fjahr opened a pull request: "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation"
(https://github.com/bitcoin/bitcoin/pull/33878)
Depends on #33026, the prior part in this series. Please review this one first.

This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).

The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum i
...

⚠️ fjahr opened an issue: "Embedded ASMap data Tracking Issue (Part 2)"
(https://github.com/bitcoin/bitcoin/issues/33879)
The predecessor tracking issue for this topic can be found here: #28794 This was closed since there seemed to be just one main PR left but I am reviving a topic issue for asmap because recently a few PRs have spun out of the main PR and related discussions. Aside from the somewhat tangential changes the main embedding PR is split into a series of three as of now.

- Embedding implementation series
- [ ] #33026
- [ ] #33857
- [ ] #28792
- [ ] https://github.com/bitcoin/bitcoin/issues/3338
...