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

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

Same comments here about inappropriate use of uint32_t. IMO, we should prefer `size_t` if trying to modernize code or `unsigned int` if trying to be backwards compatible. If there is really a reason for introducing a third size type, it should be stated and clarified somewhere.

💬 maflcko commented on pull request "init,log: Unify block index log line":
(https://github.com/bitcoin/bitcoin/pull/31616#issuecomment-2578081538)
Is there any value in the duplicate timestamp? Seems better to remove it, as per https://github.com/bitcoin/bitcoin/pull/31616#discussion_r1905705466

💬 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#issuecomment-2578083931)
> 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?

I think it is useful to have 1749aef52af8ea5f436e5b26dc7281fce73d1436
We can use it in places like bf5c569898d0297de010102a623bf52009607ed8 and maybe more like @l0rinc mentioned https://github.com/bitcoin/bitcoin/pull/31179#pullrequestreview-2526455183.


> Edit: I see you had an end-to-end benchmark in the initial description, bu
...

💬 l0rinc 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_r1907458970)
My concern isn't with the description (I think reserving space can be a valuable change), but that it doesn't really help the user calling this RPC, right?

💬 mzumsande commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2578115905)
> From my tests, it's a wallet issue (inconsistent state), that only the GUI triggers, The wallet has an invalid state, but the "happy path" in the CLI version doesn't notice it

I agree, my understanding is this: There is a temporary inconsistent state in the wallet when during a rescan the node receives additional blocks (could be via RPC or p2p), that are prematurely processed without updating `m_last_block_processed`. This will self-correct when the `blockConnected` signals for the added b
...