💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1948024853)
Updated 75931055b84d1f0e9f44d3d3efc28785fd23e66c -> 316c7c845036cbffa22b9e44f31dca8573ffb639 ([noGlobalSignals_17](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_17) -> [noGlobalSignals_18](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_18), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_17..noGlobalSignals_18))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491488517), correcting docstring.
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1948024853)
Updated 75931055b84d1f0e9f44d3d3efc28785fd23e66c -> 316c7c845036cbffa22b9e44f31dca8573ffb639 ([noGlobalSignals_17](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_17) -> [noGlobalSignals_18](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_18), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_17..noGlobalSignals_18))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491488517), correcting docstring.
🤔 naumenkogs requested changes to a pull request: "net: attempts to connect to all resolved addresses when connecting to a node"
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-1884670140)
Mostly looks good, let me know what you think about the suggestions.
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-1884670140)
Mostly looks good, let me know what you think about the suggestions.
💬 naumenkogs commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492195357)
nit: would be nice to say `XYZ (pszDest) is provided` instead of `pszDest is provided`
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492195357)
nit: would be nice to say `XYZ (pszDest) is provided` instead of `pszDest is provided`
💬 naumenkogs commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492202202)
nit:
I'd suggest to clarify the commit message saying that "a valid one is found" refers to what exactly (getting up to here and `connected = true`).
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492202202)
nit:
I'd suggest to clarify the commit message saying that "a valid one is found" refers to what exactly (getting up to here and `connected = true`).
💬 naumenkogs commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492192396)
nit: do we still need `Shuffle(resolved.begin(), resolved.end(), FastRandomContext());`? It certainly doesn't achieve the same goal as before... I'd suggest either dropping it altogether, or document this new purpose (either inline or in the commit message).
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492192396)
nit: do we still need `Shuffle(resolved.begin(), resolved.end(), FastRandomContext());`? It certainly doesn't achieve the same goal as before... I'd suggest either dropping it altogether, or document this new purpose (either inline or in the commit message).
💬 naumenkogs commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492205690)
nit: can you add a comment to justify early termination here (instead of trying going through other addrs)
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492205690)
nit: can you add a comment to justify early termination here (instead of trying going through other addrs)
💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1948087608)
Tested with HWI 2.4.0
When a Ledger Nano X is in screen saver mode, I now get the following error:
```
'...hwi' error: Could not open client or get fingerprint information: ('0x5515', 'Error in <DefaultInsType.GET_VERSION: 1> command', '')
```
That's what HWI returns, so can't make it much better...
When the Bitcoin app is not opened on the device the error is more clear:
```
'...hwi' error: Could not open client or get fingerprint information: Ledger is not in either the Bitco
...
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1948087608)
Tested with HWI 2.4.0
When a Ledger Nano X is in screen saver mode, I now get the following error:
```
'...hwi' error: Could not open client or get fingerprint information: ('0x5515', 'Error in <DefaultInsType.GET_VERSION: 1> command', '')
```
That's what HWI returns, so can't make it much better...
When the Bitcoin app is not opened on the device the error is more clear:
```
'...hwi' error: Could not open client or get fingerprint information: Ledger is not in either the Bitco
...
💬 Sjors commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1948108652)
> Also please note that I find the announcement period super short.
Removal happens at some unknown time after deprecation, but from what I can tell, not before the 28.0 release (~November 2024). The v27.* releases will be maintained for a while longer too: https://bitcoincore.org/en/lifecycle/
> the child transaction must be verified before signing their parents
What exactly do you mean by "verified"? Are you checking proof-of-work from the headers? The signature?
I'm not familiar e
...
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1948108652)
> Also please note that I find the announcement period super short.
Removal happens at some unknown time after deprecation, but from what I can tell, not before the 28.0 release (~November 2024). The v27.* releases will be maintained for a while longer too: https://bitcoincore.org/en/lifecycle/
> the child transaction must be verified before signing their parents
What exactly do you mean by "verified"? Are you checking proof-of-work from the headers? The signature?
I'm not familiar e
...
💬 maflcko commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1948169568)
cc @murchandamus You may be interested in the rss_limit_mb (last commit). Not sure if you set it in your fuzz script, or if you ever ran into OOM.
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1948169568)
cc @murchandamus You may be interested in the rss_limit_mb (last commit). Not sure if you set it in your fuzz script, or if you ever ran into OOM.
💬 vasild commented on issue "Clang 14 emits `-Wunreachable-code` warnings":
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1948185285)
The code in question can be improved:
```cpp
#ifdef USE_BDB
if constexpr (true) {
return MakeBerkeleyDatabase(path, options, status, error);
} else
#endif
{
error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
status = DatabaseStatus::FAILED_BAD_FORMAT;
return nullptr;
}
```
If `USE_BDB` is defined then this becomes:
```cpp
if c
...
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1948185285)
The code in question can be improved:
```cpp
#ifdef USE_BDB
if constexpr (true) {
return MakeBerkeleyDatabase(path, options, status, error);
} else
#endif
{
error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
status = DatabaseStatus::FAILED_BAD_FORMAT;
return nullptr;
}
```
If `USE_BDB` is defined then this becomes:
```cpp
if c
...
💬 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