💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2569566280)
Thank you for the review and code suggestions @hodlinator,

Updated 7517126d9fd2c3e50dc95ce831bac6895b950e24 -> ad80e410b3bf8814cb5b997715ee8cb28f2a2a66 ([kernel_cache_sizes_4](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_4) -> [kernel_cache_sizes_5](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_4..kernel_cache_sizes_5))

* Add handling for dbcache sizes exceeding `size_t` si
...

💬 remcoros commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901998957)
👍 my bad, github didn't show it as outdated and was looking at the convo tab.

💬 edilmedeiros commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2569575625)
Concept ACK.

Already lost many hours trying to debug this and my conclusion so far is that the FAT driver on macOS is flawed.
Also, it's quite difficult to reproduce this reliably, IBD will fail in a different point every time. Maybe we can detect a broken block in persistent memory and restart IBD from there?

💬 edilmedeiros commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1902010252)
If my memory is not failing me, I have seem only macOS related issues reporting problems with exFAT external drivers. I tried exFAT on the internal drive of an old 2016 Intel Macbook Pro (original 1GB SSD, APFS on a primary partition and exFAT on a secondary partition for Bitcoin data) and IBD failed on v26. It was about a year ago, I was trying to understand if it was a problem with the external drive I was using at the time or a software problem. Unfortunately, I can't try that again with that
...

💬 mzumsande commented on issue "Assertion pindexPrev && pindexPrev == chainstate.m_chain.Tip() when running regtest":
(https://github.com/bitcoin/bitcoin/issues/31562#issuecomment-2569582744)
> Thx alot! so is this expected to reach the next release cycle? which version will it be included in?

Looks like it will already be fixed in 28.1 (backport PR #31594) which should be released soon.

💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1902019482)
> It is a vector of package fee frac's, which can be used to build a histogram.

Building a histogram seems to be one of the use cases of this field. I don't feel it should be called a histogram only because of this. Is there any other reason as well?
I hope I'm not missing any context here.

🤔 mzumsande reviewed a pull request: "qa: Ensure consistent use of decimals instead of floats"
(https://github.com/bitcoin/bitcoin/pull/31595#pullrequestreview-2529511916)
See https://github.com/python/cpython/issues/128005 (long discussion, skip to [this post](https://github.com/python/cpython/issues/128005#issuecomment-2547582533) to save time) for more background on this.
This is NetBSD-specific, and apparently caused by a pkgsrc patch which caused `sys.float_repr_style` to be "legacy". So the question is if this qualifies as a bug in NetBSD or not and whether we need to accomodate it by moving to Decimals.

💬 achow101 commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#issuecomment-2569631398)
Pulled in @theStack's test

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1902048035)
I think you are, I'm not introducing this term in this PR
You can see https://github.com/bitcoin/bitcoin/pull/21422 for context

📝 l0rinc opened a pull request: "Modernize recent `ByteType` usages and simplify read/write functions"
(https://github.com/bitcoin/bitcoin/pull/31601)
Follow-up of https://github.com/bitcoin/bitcoin/pull/31524, applying a few of the suggestions to the codebase, while enabling more trivial reads/writes to use the new `std::byte` interface.

💬 l0rinc commented on pull request "refactor: Allow std::byte in (Read/Write)(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1902054044)
The more I looked at it in this case, the more I actually like it - I pushed a change to https://github.com/bitcoin/bitcoin/pull/31601, let's discuss it there

💬 achow101 commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2569659777)
ACK faf7eac364fb7f421a649b483286ac8681d92b31

🤔 glozow reviewed a pull request: "[28.x] 28.1 backports (final?)"
(https://github.com/bitcoin/bitcoin/pull/31594#pullrequestreview-2529550653)
ACK 1451c56cce3e3df784e1dac969ffb0165df313c7 except for release note

💬 glozow commented on pull request "[28.x] 28.1 backports (final?)":
(https://github.com/bitcoin/bitcoin/pull/31594#discussion_r1902065226)
Should also add hebasto to the credits

👍 vasild approved a pull request: "test: have miner_tests use Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31581#pullrequestreview-2529550479)
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab

Not in this PR, just an observation: given that we have two concepts: "block" and "block template", I find the below pattern a bit confusing:

```
block_template = mining->createNewBlock()
block = block_template->getBlock()
```
because I would expect `createNewBlock()` to create a new block, but apparently it creates a new block template.

💬 vasild commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1902065107)
Feel free to ignore: could use `block.vtx[0]` here instead of `MakeTransactionRef(txCoinbase)`:

```suggestion
BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, block.vtx[0]));
```
and reduce the scope of `txCoinbase` to only inside the above `{` `}` block.

💬 fanquake commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2569669491)
@mzumsande thanks for providing the actual context. Given the above, this doesn't seem like a great change.

🚀 achow101 merged a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542)

🚀 achow101 merged a pull request: "build: Use character literals for generated json headers to avoid narrowing"
(https://github.com/bitcoin/bitcoin/pull/31547)

💬 hebasto commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1902082301)
> Is it important this is a Decimal?

I confirm that your patch works.

> ... floating point values weren't defined to be this precise anyway

See @mzumsande's [comment](https://github.com/bitcoin/bitcoin/pull/31595#pullrequestreview-2529511916).