💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1907240854)
Thanks this is fixed, I'll use `snake_case` henceforth.

🤔 stickies-v reviewed a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2536926530)
I like the increased checks for overflows on 32-bit (or other platforms), but I don't think we need the new `MiBToBytes()` helper function for that and some of the new logic seems buggy to me. More detail on both in the comments.

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907214794)
This check seems pretty broken to me. Left shifting a negative value is UB, and if db_cache is indeed too large it would already overflow in `static_cast<uint64_t>(db_cache << 20)`.

On my machine, it seems this whole block is compiled away entirely, I'm not getting any warnings for either negative or too large values.

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907068544)
I think a `std::optional<size_t>` return type is more appropriate for this function, the assertions make this function hard to test. Below diff updates the return type, adds a positiveness check (as per my other comment), and adds a test case:

<details>
<summary>git diff on 578654c686</summary>

```diff
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 46ca35ea90..623aec5aa0 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -123,7 +12
...

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907055901)
I think the docstring is misleading: we're casting to uint64_t, so the sign bit will always be unset. Relatedly, this function does not deal with negative input well, so I think we first assert that `mib` is positive and update the docstring to remove the signedness reference.

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907267762)
I'm not sure we need `MiBToBytes()` at all. All but one callsite can be eliminated by just storing the constants as `size_t` byte amounts, allowing compile-time guarantees that we're not overflowing the system's `size_t` size. It adds a few more right shift operations, but those aren't prone to overflowing, and are only required for viewing purposes.

The only place where we're dealing with run-time values (in `CalculateCacheSizes()`, we already partially (and buggily, as per my other comment)
...

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907297290)
This is only relevant on machines where size_t is not a uint64_t, so I'm not surprised this is optimized away. I tested this by compiling for `arm-linux-gnueabihf` and running with:
```
qemu-arm -L /usr/arm-linux-gnueabihf build_arm_32/src/bitcoind -nowallet -dbcache=10000 -signet
```
+1 for clamping the value range to zero before doing that check though.

💬 ismaelsadeeq commented on pull request "test: raise an error in `_bulk_tx_` when `target_vsize` is too low":
(https://github.com/bitcoin/bitcoin/pull/30322#issuecomment-2577858108)
> but would prefer to keep the vsize check in the MiniWallet functional framework test (first commit). It's true that this is currently unreachable, but a potential bug where e.g. the logic would change in a way that _bulk_tx isn't even called would then remain undetected by the test.

Reverted. thanks for review.

💬 maflcko commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#discussion_r1907326800)
Actually, `static` seems a better fit. Thx, fixed.

💬 luke-jr commented on pull request "run_command: Close non-std fds when execing slave processes":
(https://github.com/bitcoin/bitcoin/pull/30756#issuecomment-2577910594)
Refactored to build a `std::vector<int>` just in case `fs::directory_iterator` uses a fd itself during iteration.

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907373571)
> allowing compile-time guarantees that we're not overflowing the system's size_t size.

I'm not sure that is actually true. Does this actually guard against any possible overflow?

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907385011)
> This is only relevant on machines where size_t is not a uint64_t, so I'm not surprised this is optimized away.

It seems to me like this check is necessary on all platforms? E.g. on this branch, when I run with `-dbcache=10000000000000000` I get

```
../../src/kernel/caches.h:25 MiBToBytes: Assertion `std::countl_zero(static_cast<uint64_t>(mib)) >= 21' failed.
```

In `static_cast<uint64_t>(db_cache << 20)` we're left-shifting `db_cache` without checking that it'll fit the `uint64_t`.
...

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907407674)
I'm not sure what the guarantees are, but at least on my machine, with `static constexpr size_t MAX_FILTER_INDEX_CACHE{18446744073709551616};` (i.e. max + 1 on 64 bit) compilation fails with:

```
../../src/node/caches.cpp:22:48: error: integer literal is too large to be represented in any integer type
static constexpr size_t MAX_FILTER_INDEX_CACHE{18446744073709551616};
^
1 error generated.
```

Similarly, for `static constexpr uint8_t TES
...

🤔 maflcko reviewed a pull request: "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function"
(https://github.com/bitcoin/bitcoin/pull/31179#pullrequestreview-2537532645)
Is this worth it, when the real end-to-end speedup is less than a percent, according to https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900164051?

💬 maflcko 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_r1907407539)
This creates a copy of the witness stack. Previously it did not, so it seems like a pessimisation.

💬 theuni commented on pull request "ci: change the build to be verbose by default":
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2578043932)
> How is this different from just looking at the `C++ compiler flags ....................` output?

Not weighing in on the approach here, but the flags output is fuzzy at best (see the disclaimer below them). Some flags only apply to certain types of objects (simd for ex), certain libs (leveldb), certain build types (shared/static), etc.

👍 theuni approved a pull request: "build, test: Build `db_tests.cpp` regardless of `USE_BDB`"
(https://github.com/bitcoin/bitcoin/pull/31617#pullrequestreview-2537567612)
utACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501

👍 theuni approved a pull request: "init,log: Unify block index log line"
(https://github.com/bitcoin/bitcoin/pull/31616#pullrequestreview-2537578194)
utACK 60b7bbd3709519c6f2f8023a23ee0a194a5f0eb4

💬 maflcko commented on pull request "ci: change the build to be verbose by default":
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2578078036)
Yes, it is a bit fuzzy, but printing the flags to the CI log doesn't solve the problems listed in the motivation either, because to confirm that a flag is used for *all* compilations in a successful compilation, one will have to go through *all* lines. Without a script, I don't see anyone doing that manually. In fact, this change makes the log so bloated that the scrollback will normally truncate it, so it is more likely that the change is going to hide issues instead of surface them.

👍 ryanofsky approved a pull request: "refactor: cache block[undo] serialized size for consecutive calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2537328922)
Code review ACK 2ff0ea366c61b2fcb80ad1f711480915c7a7aa2e. New version of this PR is different and I think easier to follow than the previous version. All the changes now seem like obvious code cleanups.

Might consider renaming PR from "cache block[undo] serialized size" because I'm not sure it's even accurate to call not pointlessly recomputing the same values twice in a row "caching".