💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906989778)
I thought about generating a fake testnet4 with empty blocks, but that wouldn't be much smaller. However if testnet5 turns out to have large initial blocks, we can always do that.
Headers are not enough, because I need the tip to update. Unless you want to revive #9483 :-)
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906989778)
I thought about generating a fake testnet4 with empty blocks, but that wouldn't be much smaller. However if testnet5 turns out to have large initial blocks, we can always do that.
Headers are not enough, because I need the tip to update. Unless you want to revive #9483 :-)
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906991364)
That said, committing only 2016 nonces would make it a lot smaller, so maybe it's a good idea regardless.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906991364)
That said, committing only 2016 nonces would make it a lot smaller, so maybe it's a good idea regardless.
💬 hodlinator commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#issuecomment-2577377697)
> That is what this pull is trying to do. Did I miss something?
Was thinking something like:
```diff
void GetRandBytes(Span<unsigned char> bytes) noexcept
{
g_used_g_prng = true;
+ if constexpr (G_FUZZING) {
+ Assert(g_seeded_g_prng_zero); // Need to seed randomness first
+ }
ProcRand(bytes.data(), bytes.size(), RNGLevel::FAST, /*always_use_real_rng=*/false);
}
```
So one gets that assert as quickly as possible.
But as I said, `g_rng_temp_path_init` would make
...
(https://github.com/bitcoin/bitcoin/pull/31548#issuecomment-2577377697)
> That is what this pull is trying to do. Did I miss something?
Was thinking something like:
```diff
void GetRandBytes(Span<unsigned char> bytes) noexcept
{
g_used_g_prng = true;
+ if constexpr (G_FUZZING) {
+ Assert(g_seeded_g_prng_zero); // Need to seed randomness first
+ }
ProcRand(bytes.data(), bytes.size(), RNGLevel::FAST, /*always_use_real_rng=*/false);
}
```
So one gets that assert as quickly as possible.
But as I said, `g_rng_temp_path_init` would make
...
💬 maflcko commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#issuecomment-2577389848)
I think as long as the assert hits at any time, it should be good enough and more than sufficient for now, as long as random use isn't missed fully. Debugging should be trivial in any case for a developer, I'd presume.
(https://github.com/bitcoin/bitcoin/pull/31548#issuecomment-2577389848)
I think as long as the assert hits at any time, it should be good enough and more than sufficient for now, as long as random use isn't missed fully. Debugging should be trivial in any case for a developer, I'd presume.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907010936)
I had to move `CHECK_NONFATAL` outside the `WITH_LOCK` call to avoid `returning reference to local temporary object [-Wreturn-stack-address]`
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1907010936)
I had to move `CHECK_NONFATAL` outside the `WITH_LOCK` call to avoid `returning reference to local temporary object [-Wreturn-stack-address]`
💬 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
...