💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907022627)
I switched to using `chainman.GetConsensus()` everywhere.
The lock on cs_main is needed to get `pblockindex` and `tip`, but it's not needed to get `chainman` and therefore also not needed to get `pow_limit`. I'll move some things around.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907022627)
I switched to using `chainman.GetConsensus()` everywhere.
The lock on cs_main is needed to get `pblockindex` and `tip`, but it's not needed to get `chainman` and therefore also not needed to get `pow_limit`. I'll move some things around.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907046174)
This results in `assigning to 'CBlockIndex *' from 'const CBlockIndex *' discards qualifier` at the line `next_index.pprev = &tip;`.
I'd rather not enter the rabbit hole of changing `CBlockIndex` to make `pprev` as `const CBlockIndex*`. So for now I just drop the `const` qualifier for `tip`. Unless you have another suggestion?
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907046174)
This results in `assigning to 'CBlockIndex *' from 'const CBlockIndex *' discards qualifier` at the line `next_index.pprev = &tip;`.
I'd rather not enter the rabbit hole of changing `CBlockIndex` to make `pprev` as `const CBlockIndex*`. So for now I just drop the `const` qualifier for `tip`. Unless you have another suggestion?
💬 maflcko commented on pull request "build, test: Build `db_tests.cpp` regardless of `USE_BDB`":
(https://github.com/bitcoin/bitcoin/pull/31617#issuecomment-2577461131)
review ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501 🔺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK fd2d96d9087b
...
(https://github.com/bitcoin/bitcoin/pull/31617#issuecomment-2577461131)
review ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501 🔺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK fd2d96d9087b
...
💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907053464)
> I'd rather not enter the rabbit hole of changing `CBlockIndex` to make `pprev` as `const CBlockIndex*`.
It's fine, my bad. It needs to be mutable, so I edited my nit suggestion.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907053464)
> I'd rather not enter the rabbit hole of changing `CBlockIndex` to make `pprev` as `const CBlockIndex*`.
It's fine, my bad. It needs to be mutable, so I edited my nit suggestion.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2577538503)
Addressed comments. Looking into replacing `testnet4.hex` with something much more compact.
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2577538503)
Addressed comments. Looking into replacing `testnet4.hex` with something much more compact.
💬 psgreco commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2577633853)
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
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2577633853)
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
💬 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
(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
(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`
(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.
(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
(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.
(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.
(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.
(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
...
(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.
(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)
...
(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.
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/31548#discussion_r1907326800)
Actually, `static` seems a better fit. Thx, fixed.