💬 luke-jr commented on pull request "gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs":
(https://github.com/bitcoin-core/gui/pull/850#issuecomment-2577726807)
>This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.

Two bytes, isn't it? For the PSBT_IN_SIGHASH keytype

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907228559)
Fixed in latest push https://github.com/bitcoin/bitcoin/compare/7452638559e8ccbe00db5ef5a53cb21ffe27d337..2fb833e69627a8f9b1c4b14b60d1df9903adf210

💬 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_r1907237490)
> The problem is that the goal of this PR was to speed up the RPC.

I've updated the description and the title to limit the usage to just `blockToJSON`

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1907239188)
Fixed now.
Thanks for the review.

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1907239504)
fixed

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