💬 hodlinator commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2577338490)
3% speedup is less to write home about but still good. Having a less constrained ([Sjors](https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2575279833)) -dbcache setting (30GB) would lead to less XOR read/writes.
12.3% speedup ([l0rinc](https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2558426619)) could at least in part be explained by the 1GB setting, leading to more XOR-operations.
An optimization that provides a bigger win for constrained devices should be welcomed.
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2577338490)
3% speedup is less to write home about but still good. Having a less constrained ([Sjors](https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2575279833)) -dbcache setting (30GB) would lead to less XOR read/writes.
12.3% speedup ([l0rinc](https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2558426619)) could at least in part be explained by the 1GB setting, leading to more XOR-operations.
An optimization that provides a bigger win for constrained devices should be welcomed.
👍 Sjors approved a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2536792956)
ACK 7452638559e8ccbe00db5ef5a53cb21ffe27d337
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2536792956)
ACK 7452638559e8ccbe00db5ef5a53cb21ffe27d337
💬 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