💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1906977462)
You lost a line here:
```md
- This PR also introduces a new startup option, `-maxcoinbaseweight` (default: `8,000` weight units).
```
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1906977462)
You lost a line here:
```md
- This PR also introduces a new startup option, `-maxcoinbaseweight` (default: `8,000` weight units).
```
💬 maflcko commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#issuecomment-2577351760)
I've created https://github.com/bitcoin/bitcoin/pull/31620 to reduce the long, redundant and confusing error message observed originally a bit.
But the two changes are unrelated otherwise.
lgtm ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
(https://github.com/bitcoin/bitcoin/pull/31462#issuecomment-2577351760)
I've created https://github.com/bitcoin/bitcoin/pull/31620 to reduce the long, redundant and confusing error message observed originally a bit.
But the two changes are unrelated otherwise.
lgtm ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
💬 maflcko commented on pull request "test: Remove --noshutdown flag, Tidy startup failures":
(https://github.com/bitcoin/bitcoin/pull/31620#issuecomment-2577352102)
For testing the third commit different ways are possible. For example:
```
$ rm -rf ./releases
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
```
Then, observing a shorter error message.
(https://github.com/bitcoin/bitcoin/pull/31620#issuecomment-2577352102)
For testing the third commit different ways are possible. For example:
```
$ rm -rf ./releases
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
```
Then, observing a shorter error message.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906980852)
For the web developer in me that's obvious :-)
But I'll add `.json` to make it more clear this doesn't apply to the other format.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1906980852)
For the web developer in me that's obvious :-)
But I'll add `.json` to make it more clear this doesn't apply to the other format.
💬 maflcko commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#discussion_r1906984997)
I think anything is fine that documents that this function is local to this module.
(https://github.com/bitcoin/bitcoin/pull/31548#discussion_r1906984997)
I think anything is fine that documents that this function is local to this module.
💬 maflcko commented on pull request "fuzz: Abort when global PRNG is used before SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31548#issuecomment-2577366143)
> Could extend this to also catch too early uses of randomness when they occur in `GetRandBytes`, but `g_rng_temp_path_init` makes that a bit cumbersome.
That is what this pull is trying to do. Did I miss something?
(https://github.com/bitcoin/bitcoin/pull/31548#issuecomment-2577366143)
> Could extend this to also catch too early uses of randomness when they occur in `GetRandBytes`, but `g_rng_temp_path_init` makes that a bit cumbersome.
That is what this pull is trying to do. Did I miss something?
💬 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`