💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907547564)
I'm more concerned how the shift will be cast. E.g. a potential future bump to 3000 MiB would error:
```
error: constant expression evaluates to -1149239296 which cannot be narrowed to type 'size_t' (aka 'unsigned long') [-Wc++11-narrowing]
16 | static constexpr size_t DEFAULT_KERNEL_CACHE{3000 << 20};
```
so to make this future proof, I think a function interpreting the values properly, or more ugly casts are required.

💬 theuni commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2578232458)
Also, it seems these were masked by the fact that we use `-Wno-error=implicit-function-declaration -Wno-error=implicit-int` for depends builds.

This is because `exit(0)` which is called a bunch during configure (and there are probably a bunch of others too, but exit is the first that causes problems) is implicitly defined because `stdlib.h` isn't included. Blah.

If this were an important library I'd say we should patch up their configure and remove these warning-off-switches. But since we'
...

👍 ryanofsky approved a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2537795237)
Code review ACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7. Changes since last review are rebasing due to a test conflict, giving the new field a better name and description, resolving the test conflict, and renaming a lot of test variables. The actual code change is still one-line change.

👍 theuni approved a pull request: "doc: Clarify min macOS and Xcode version"
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2537798676)
ACK fa029a78780fd00e1d3ce1bebb81a95857bfbb94

💬 theuni commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2578280793)
> Not sure about the first commit. There is no issue or risk with the existing code that is fixed by the style fixups, so to me it seems better to not change it, unless there is a compelling reason. Generally when it comes to style modernizations, my recommendation would be to include them in changes that touch the line anyway, or leave them as-is, or do them wholesale for the whole project (only if there is a good reason for them). But going out and doing them one-by one for specific subsection
...

💬 theuni commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2578290759)
Not opposed to this, but it seems like a good use-case for a kernel util :)

@thecharlatan @stickies-v @josibake

🤔 glozow reviewed a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2537832865)
reACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7

🚀 glozow merged a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391)

💬 psgreco commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2578308486)
@mzumsande if you end up using my version, please look at the final version here https://github.com/ElementsProject/elements/pull/1376/files, which addresses the locking too

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1907609405)
Ahh I see,
Initially I was under the impression that it did, but I was wrong hence I updated the title and description

🤔 furszy reviewed a pull request: "wallet: Cleanup accidental encryption keys in watchonly wallets"
(https://github.com/bitcoin/bitcoin/pull/28724#pullrequestreview-2537883870)
Code review ACK 69e95c2b4f99eb4

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1907617007)
Fixed in https://github.com/bitcoin/bitcoin/compare/bf5c569898d0297de010102a623bf52009607ed8..ea62aaed3b1851605c9ff33a9d9f9edcb53828a5

💬 pablomartin4btc commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1907622464)
> +1. Something like this would be helpful. I've received a few messages sharing these logs without knowing what to do next.

I was of them :raising_hand:, I stepped upon this issue while testing #31451 and I found this [answer](https://bitcoin.stackexchange.com/questions/121085/how-can-i-run-the-skipped-bitcoin-functional-tests) from @achow101 in stack-overflow before checking [/test/README.md](https://github.com/bitcoin/bitcoin/blob/master/test/README.md).

I'd put a reference to the `READ
...

🤔 pablomartin4btc reviewed a pull request: "test: raise explicit error if any of the needed release binaries is missing"
(https://github.com/bitcoin/bitcoin/pull/31462#pullrequestreview-2537897406)
Concept ACK.

Left a few [suggestions](https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1907622464) but other than that I'm happy with this step forward.

🤔 ryanofsky reviewed a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2537837742)
Code review 578654c686e394d08bac7c3bcddbfd90b46eaa62. Looks pretty good, but there does appear to be a possible overflow bug https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907214794 found by stickies that should be fixed

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907588881)
In commit "init: Simplify coinsdb cache calculation" (19316eccaf93776c49b1870f8aa9a5fe57ec5f33)

Didn't notice this first time I reviewed this, but it would be good if commit subject included "refactor" and make it obvious this is not changing behavior. This simplification is only changing the way the calculation is performed, not changing the result of the calculation.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907647674)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907214794

Agree with stickies overflow should be fixed. I think an easy way would just be to check if `db_cache > std::numeric_limits<size_t>::max() >> 20` though.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907618229)
In commit "kernel: Move kernel-specific cache size options to kernel" (41bc7164880ffd0782ff5882211c4fcadd656022)

This seems like it is overthinking things. I think this should just be:

```c++
Assert(mib >= 0);
Assert(mib <= std::numeric_limits<size_t>::max() >> 20);
return mib << 20;
```

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907604163)
To be pedantic, old calculation could only be useful if nMaxCoinsDBCache was *higher* not lower than 8 MiB. As long as it is 8MiB or lower this commit does not change behavior.

In #6102 the coins_db cache size was set to min(50% total cache size, 8MiB + 25% total cache size). But then in #8273 it was capped to nMaxCoinsDBCache, so as long as nMaxCoinsDBCache <= 8 MiB, this is just equivalent to min(50% total cache size, nMaxCoinsDBCache)

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907639861)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907068544

> and adds a test case

I don't think I see a new test case added in the diff. Is it missing?

Another way to make this testable without adding interface complexity would just be to throw std::overflow_error