💬 maflcko commented on issue "Clang 14 emits `-Wunreachable-code` warnings":
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1948192976)
Sure, but that is simply a revert of the commit mentioned in the description. Thus, the code would no longer be compiled unconditionally, and compile errors may be missed.
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1948192976)
Sure, but that is simply a revert of the commit mentioned in the description. Thus, the code would no longer be compiled unconditionally, and compile errors may be missed.
🤔 hebasto reviewed a pull request: "lint: Check for missing bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29408#pullrequestreview-1884987863)
Tested 48562917172accd911e4c86f1032e4282fd321f5 on Ubuntu 23.10.
Missing `#include <config/bitcoin-config.h>` are reported correctly.
What are we supposed to do to automatically avoid redundant / unneeded includes?
(https://github.com/bitcoin/bitcoin/pull/29408#pullrequestreview-1884987863)
Tested 48562917172accd911e4c86f1032e4282fd321f5 on Ubuntu 23.10.
Missing `#include <config/bitcoin-config.h>` are reported correctly.
What are we supposed to do to automatically avoid redundant / unneeded includes?
🚀 fanquake merged a pull request: "doc: Update translation process guide"
(https://github.com/bitcoin/bitcoin/pull/29414)
(https://github.com/bitcoin/bitcoin/pull/29414)
🤔 furszy reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1884993957)
ACK 316c7c8450
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1884993957)
ACK 316c7c8450
💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492391457)
I'm still unsure about the way this is written. Maybe better to write something like "`func` usually contains a loop that dispatches a single validation event to all subscribers sequentially. This will block validation if it is processed synchronously".
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492391457)
I'm still unsure about the way this is written. Maybe better to write something like "`func` usually contains a loop that dispatches a single validation event to all subscribers sequentially. This will block validation if it is processed synchronously".
🚀 fanquake merged a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037)
(https://github.com/bitcoin/bitcoin/pull/28037)
💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492401601)
Would be a nice follow-up to remove the non-deterministic scheduler from all tests, unless they are explicitly testing the scheduler.
cc @aureleoules , who was asking for a solution to this for the corecheck infra
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492401601)
Would be a nice follow-up to remove the non-deterministic scheduler from all tests, unless they are explicitly testing the scheduler.
cc @aureleoules , who was asking for a solution to this for the corecheck infra
💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492409498)
Are you sure this is correct? Would it be possible that it is called once for each validation interface event, and the inserted function would then iterate over each subscriber?
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492409498)
Are you sure this is correct? Would it be possible that it is called once for each validation interface event, and the inserted function would then iterate over each subscriber?
🤔 maflcko reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1885045429)
ACK 316c7c845036cbffa22b9e44f31dca8573ffb639 🔁
<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: ACK 316c7c845036cbffa22b9e44f3
...
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1885045429)
ACK 316c7c845036cbffa22b9e44f31dca8573ffb639 🔁
<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: ACK 316c7c845036cbffa22b9e44f3
...
💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492412912)
https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492391457
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492412912)
https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492391457
💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948330593)
> What are we supposed to do to automatically avoid redundant / unneeded includes?
Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948330593)
> What are we supposed to do to automatically avoid redundant / unneeded includes?
Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1948331732)
ACK https://github.com/bitcoin/bitcoin/pull/26008/commits/ac246f68299d0dc208ae513dfad1a8fc91b5e6d4
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1948331732)
ACK https://github.com/bitcoin/bitcoin/pull/26008/commits/ac246f68299d0dc208ae513dfad1a8fc91b5e6d4
💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948352540)
Done
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948352540)
Done
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1948355393)
Added `m_clients_mutex` similar to the one in `Connman`. Fixed a deadlock on `g_best_block_mutex` when we the block from a peer at the same time as we receive `SubmitSolution` from the pool / job declarator client.
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1948355393)
Added `m_clients_mutex` similar to the one in `Connman`. Fixed a deadlock on `g_best_block_mutex` when we the block from a peer at the same time as we receive `SubmitSolution` from the pool / job declarator client.
🤔 brunoerg reviewed a pull request: "fuzz: Generate with random libFuzzer settings"
(https://github.com/bitcoin/bitcoin/pull/28178#pullrequestreview-1885135549)
light ACK fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde
I did a light test of these values using differential fuzzing and mutation testing. I applied differential fuzzing between the original coin selection functions and their mutants. In general:
1. Running from seed corpus, `-runs=100000` was able to kill most mutants (>55%) but `-max_total_time=6000` is so much more effective (>90%), as expected.
2. Running without seed corpus, `-runs=100000` was useless, `-max_total_time=6000` was effectiv
...
(https://github.com/bitcoin/bitcoin/pull/28178#pullrequestreview-1885135549)
light ACK fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde
I did a light test of these values using differential fuzzing and mutation testing. I applied differential fuzzing between the original coin selection functions and their mutants. In general:
1. Running from seed corpus, `-runs=100000` was able to kill most mutants (>55%) but `-max_total_time=6000` is so much more effective (>90%), as expected.
2. Running without seed corpus, `-runs=100000` was useless, `-max_total_time=6000` was effectiv
...
🚀 fanquake merged a pull request: "[26.x] more backports"
(https://github.com/bitcoin/bitcoin/pull/29209)
(https://github.com/bitcoin/bitcoin/pull/29209)
💬 fanquake commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1948410187)
> Removal happens at some unknown time after deprecation,
No, removal is going to happen pretty immediately after the 27.x branch off.
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1948410187)
> Removal happens at some unknown time after deprecation,
No, removal is going to happen pretty immediately after the 27.x branch off.
💬 hebasto commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948433154)
> > What are we supposed to do to automatically avoid redundant / unneeded includes?
>
> Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
IWYU falsely suggests to remove `#include <config/bitcoin-config.h>` in any case where the corresponding macro is commented out in the `config/bit
...
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948433154)
> > What are we supposed to do to automatically avoid redundant / unneeded includes?
>
> Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
IWYU falsely suggests to remove `#include <config/bitcoin-config.h>` in any case where the corresponding macro is commented out in the `config/bit
...
💬 hebasto commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948460459)
> > What are we supposed to do to automatically avoid redundant / unneeded includes?
>
> Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
I assume that both macros `HAVE_TIMINGSAFE_BCMP` and `HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR` are commented out in `config/bitcoin-config.h` in
...
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948460459)
> > What are we supposed to do to automatically avoid redundant / unneeded includes?
>
> Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?
I assume that both macros `HAVE_TIMINGSAFE_BCMP` and `HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR` are commented out in `config/bitcoin-config.h` in
...
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492523347)
re: https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492409498
Yeah the sentence "When used in the ValidationSignals synchronous processing will block validation. It is called for each subscriber on each validation interface event." is a little misleading because only one task per event is inserted, regardless of number of subscribers. In the future we might want to change this so indexes/wallets don't block each other, but that is the current implementation.
I would fix this by
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492523347)
re: https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492409498
Yeah the sentence "When used in the ValidationSignals synchronous processing will block validation. It is called for each subscriber on each validation interface event." is a little misleading because only one task per event is inserted, regardless of number of subscribers. In the future we might want to change this so indexes/wallets don't block each other, but that is the current implementation.
I would fix this by
...