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

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907299844)
In commit "refactor,blocks: cache block serialized size for consecutive calls" (https://github.com/bitcoin/bitcoin/commit/6e22e55920da33dc8970793f9e6a854eb3faa3c4)

IMO in new code it would be better to avoid using `uint32_t` and `unsigned int` for sizes and just use `size_t` for consistency and simplicity. static_casts like this could then be avoided except when calling older functions, and could eventually be removed when older functions are updated.

However, if we do want to keep using n
...

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907341326)
In commit "refactor,blocks: cache block serialized size for consecutive calls" (https://github.com/bitcoin/bitcoin/commit/6e22e55920da33dc8970793f9e6a854eb3faa3c4)

I don't see a reason to delete the comments from this function. I think they are helpful and it is inconsistent to get rid of these because this commit is not deleting the corresponding comments in the WriteUndoDataForBlock function.

I particularly think the "Write index header" comments in both functions are helpful because the
...

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907288195)
In commit "refactor,blocks: cache block serialized size for consecutive calls" (6e22e55920da33dc8970793f9e6a854eb3faa3c4)

Seems like this could be a static assert

EDIT: but I guess it is removed in the upcoming commit, so doesn't matter too much

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907413267)
In commit "scripted-diff: rename block and undo functions for consistency" (2ff0ea366c61b2fcb80ad1f711480915c7a7aa2e)

Agree with hodlinator that Read/Write or Load/Save would now be better than Read/Save. The reason I suggested Save instead of Write in https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-250956282 is that previously we had a high level function for writing called Save and lower level functions called Write, but now after 9b59f8b624cc641bd7216ececffa3111041fd760, th
...