💬 ajtowns commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2371229366)
Perhaps:
```c++
constexpr auto TOO_RECENT_FOR_ASSUMEVALID = std::chrono::days{14})};
...
} else if (GBPET(...) <= count_seconds(TOO_RECENT_FOR_ASSUME_VALID)) {
```
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2371229366)
Perhaps:
```c++
constexpr auto TOO_RECENT_FOR_ASSUMEVALID = std::chrono::days{14})};
...
} else if (GBPET(...) <= count_seconds(TOO_RECENT_FOR_ASSUME_VALID)) {
```
💬 ajtowns commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2371247053)
Separate text with a blank line rather than indenting?
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2371247053)
Separate text with a blank line rather than indenting?
💬 ajtowns commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2371280673)
consider setting `const bool fScriptChecks = (script_check_reason != nullptr);` here, and leaving the following code unchanged?
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2371280673)
consider setting `const bool fScriptChecks = (script_check_reason != nullptr);` here, and leaving the following code unchanged?
💬 ajtowns commented on pull request "log: reduce excessive "rolling forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2371296279)
Perhaps also noting that rate limiting does not apply to LogDebug and LogTrace would help encourage people to use those macros for noisy logs, rather than sticking with INFO level but disabling rate limiting.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2371296279)
Perhaps also noting that rate limiting does not apply to LogDebug and LogTrace would help encourage people to use those macros for noisy logs, rather than sticking with INFO level but disabling rate limiting.
💬 Sjors commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3322771214)
As I try newer guix time machine commits, new cans of worms keep being opened. E.g. when testing the proposed fix for https://codeberg.org/guix/guix/issues/1257 I ran into yet more packages failing to build, probably related to this: https://codeberg.org/guix/guix/issues/928. Hopefully one day there'll be a master commit that just works(tm).
Until then, it's probably better than we recommend users to use one substitute. Thanks for the hint @trevarj.
Going back and forth with the time machi
...
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3322771214)
As I try newer guix time machine commits, new cans of worms keep being opened. E.g. when testing the proposed fix for https://codeberg.org/guix/guix/issues/1257 I ran into yet more packages failing to build, probably related to this: https://codeberg.org/guix/guix/issues/928. Hopefully one day there'll be a master commit that just works(tm).
Until then, it's probably better than we recommend users to use one substitute. Thanks for the hint @trevarj.
Going back and forth with the time machi
...
💬 maflcko commented on issue "cli:passing non-integers to datacarriersize":
(https://github.com/bitcoin/bitcoin/issues/33460#issuecomment-3322788080)
This is a known issue and should affect almost all args. See also https://github.com/bitcoin/bitcoin/issues/22978
(https://github.com/bitcoin/bitcoin/issues/33460#issuecomment-3322788080)
This is a known issue and should affect almost all args. See also https://github.com/bitcoin/bitcoin/issues/22978
💬 Kruwed commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322791530)
>This specific PR seeks clarity in the datacarrier options' future availability, in favor of removing the deprecation, as the deprecation designation definition is unclear, potentially incorrect (if the definition is will be removed), and causes uncertainty for Bitcoin Core users around timing.
Why shouldn't the datacarrier options be removed? My earlier question remains unaddressed:
>>Why should Bitcoin Core support a config option to perform targeted censorship of large OP_RETURNs?
@S
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322791530)
>This specific PR seeks clarity in the datacarrier options' future availability, in favor of removing the deprecation, as the deprecation designation definition is unclear, potentially incorrect (if the definition is will be removed), and causes uncertainty for Bitcoin Core users around timing.
Why shouldn't the datacarrier options be removed? My earlier question remains unaddressed:
>>Why should Bitcoin Core support a config option to perform targeted censorship of large OP_RETURNs?
@S
...
💬 hebasto commented on pull request "qt: Fix dark mode detection on Linux":
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3322865645)
> Enable GTK3 platform theme integration on Linux to properly detect
> system dark/light mode settings. Without this, Qt applications on
> Linux don't automatically adapt to the system theme, unlike macOS
> where it works out of the box.
Could you please clarify which build this change is intended to affect, statically linked or shared?
If the latter, which system was this tested on (OS, desktop environment, Qt version)?
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3322865645)
> Enable GTK3 platform theme integration on Linux to properly detect
> system dark/light mode settings. Without this, Qt applications on
> Linux don't automatically adapt to the system theme, unlike macOS
> where it works out of the box.
Could you please clarify which build this change is intended to affect, statically linked or shared?
If the latter, which system was this tested on (OS, desktop environment, Qt version)?
💬 Sjors commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322927882)
Concept meh, I think removing the deprecation warning sets the wrong expectation.
Those who fully want to embrace filtering should be looking for alternative software, rather than demand that Bitcoin Core implements it for them.
The current code is deprecated in the sense of being unmaintained, even if it's not going to be removed anytime soon. Pull requests that try to expand or "improve" their functionality have consistently been closed.
So perhaps the following can be softened, from:
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322927882)
Concept meh, I think removing the deprecation warning sets the wrong expectation.
Those who fully want to embrace filtering should be looking for alternative software, rather than demand that Bitcoin Core implements it for them.
The current code is deprecated in the sense of being unmaintained, even if it's not going to be removed anytime soon. Pull requests that try to expand or "improve" their functionality have consistently been closed.
So perhaps the following can be softened, from:
...
💬 maflcko commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3322949088)
Will the be useful overall? I think it is clear that no developer is using this platform right now (maybe not even a user), so the benefit of the CI config is mostly a sanity check. The issues it finds seems to happen less than once per year:
* https://github.com/bitcoin/bitcoin/issues/26820
* https://github.com/bitcoin/bitcoin/issues/30587
* Possibly https://github.com/bitcoin/bitcoin/pull/27529, albeit the CI task was only later adjusted to find it
All the issues were trivial to fixup
...
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3322949088)
Will the be useful overall? I think it is clear that no developer is using this platform right now (maybe not even a user), so the benefit of the CI config is mostly a sanity check. The issues it finds seems to happen less than once per year:
* https://github.com/bitcoin/bitcoin/issues/26820
* https://github.com/bitcoin/bitcoin/issues/30587
* Possibly https://github.com/bitcoin/bitcoin/pull/27529, albeit the CI task was only later adjusted to find it
All the issues were trivial to fixup
...
💬 Sjors commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322957188)
Additionally, maybe dropping the startup warning also makes sense. It's arguably premature when there is no concrete plan to remove the option.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322957188)
Additionally, maybe dropping the startup warning also makes sense. It's arguably premature when there is no concrete plan to remove the option.
💬 maflcko commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#discussion_r2371586635)
Will this cache anything?
According to https://github.com/bitcoin/bitcoin/actions/caches
I see:
> docker.io--tonistiigi--binfmt-latest-linux-x64 30 MB cached 4 days ago
refs/pull/33436/merge
Last used 4 days ago
Which seems a bit odd, but I am not familiar with GHA, nor the `docker/` actions.
(https://github.com/bitcoin/bitcoin/pull/33436#discussion_r2371586635)
Will this cache anything?
According to https://github.com/bitcoin/bitcoin/actions/caches
I see:
> docker.io--tonistiigi--binfmt-latest-linux-x64 30 MB cached 4 days ago
refs/pull/33436/merge
Last used 4 days ago
Which seems a bit odd, but I am not familiar with GHA, nor the `docker/` actions.
💬 ajtowns commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322973150)
> The current code is deprecated in the sense of being unmaintained
Why do you believe it is unmaintained? It is still tested, and there are still contributors who are willing to keep it operational.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3322973150)
> The current code is deprecated in the sense of being unmaintained
Why do you believe it is unmaintained? It is still tested, and there are still contributors who are willing to keep it operational.
🤔 Eunovo reviewed a pull request: "log: always print initial script verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3256881020)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/010cf81407c0df8de766fd2a116463d180f25f33
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3256881020)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/010cf81407c0df8de766fd2a116463d180f25f33
💬 maflcko commented on pull request "ci: add Valgrind fuzz":
(https://github.com/bitcoin/bitcoin/pull/33461#issuecomment-3323011990)
Seems fine to add, and the runtime seems fast. Though, I wonder how many issues this will find. The only issue I am aware of this year is https://github.com/bitcoin/bitcoin/issues/32121, but this was only found by adding newly generated fuzz inputs to the qa-assets repo, which already runs this CI task.
(Same feedback for https://github.com/bitcoin/bitcoin/pull/33425, which seems similar, with the difference of added library hardening?)
(https://github.com/bitcoin/bitcoin/pull/33461#issuecomment-3323011990)
Seems fine to add, and the runtime seems fast. Though, I wonder how many issues this will find. The only issue I am aware of this year is https://github.com/bitcoin/bitcoin/issues/32121, but this was only found by adding newly generated fuzz inputs to the qa-assets repo, which already runs this CI task.
(Same feedback for https://github.com/bitcoin/bitcoin/pull/33425, which seems similar, with the difference of added library hardening?)
💬 maflcko commented on pull request "ci: re-add Valgrind job to the CI":
(https://github.com/bitcoin/bitcoin/pull/33411#issuecomment-3323084726)
No objection, but are there any real issues that this task has found in the last years? The only ones I am aware of are false-positives around valgrind and questions around updating the task config itself for a newly added package. If those are the only reasons to add it, the reasoning seems circular and pull request authors having to deal with valgrind false positives directly in their pull requests may even be counter-productive? For example, https://github.com/bitcoin/bitcoin/pull/31282/files
...
(https://github.com/bitcoin/bitcoin/pull/33411#issuecomment-3323084726)
No objection, but are there any real issues that this task has found in the last years? The only ones I am aware of are false-positives around valgrind and questions around updating the task config itself for a newly added package. If those are the only reasons to add it, the reasoning seems circular and pull request authors having to deal with valgrind false positives directly in their pull requests may even be counter-productive? For example, https://github.com/bitcoin/bitcoin/pull/31282/files
...
💬 luke-jr commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3323114350)
The entire change (#32406) needs to be reverted. Un-deprecating it doesn't make any sense as long as it's being sabotaged with new bugs. The default change is the big problem.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3323114350)
The entire change (#32406) needs to be reverted. Un-deprecating it doesn't make any sense as long as it's being sabotaged with new bugs. The default change is the big problem.
💬 luke-jr commented on pull request "Fix dark mode detection on Linux":
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3323130893)
Seems like this could make things worse for native (Qt) environments?
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3323130893)
Seems like this could make things worse for native (Qt) environments?
💬 maflcko commented on pull request "ci: run native_fuzz_with_msan":
(https://github.com/bitcoin/bitcoin/pull/33425#issuecomment-3323149107)
Unrelatedly(?), I wonder if it is possible to adjust the existing macos fuzz task to have stdlib hardening enabled.
(https://github.com/bitcoin/bitcoin/pull/33425#issuecomment-3323149107)
Unrelatedly(?), I wonder if it is possible to adjust the existing macos fuzz task to have stdlib hardening enabled.
💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3323153850)
`504803f909...48c5939cf4`: fix tidy, note that also in `master` there is inconsistency: the prototype of `OpenNetworkConnection()` in `net.h` uses `strDest` whereas the implementation in `net.cpp` uses `pszDest`. Resolved by this PR.
> If you're updating the name anyways, maybe just get rid of the hungarian notation and call it `destination`?
Yes, I think this would be very nice to do. I avoided it because it would bloat the diff as a lot of lines inside the body of the functions will be
...
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3323153850)
`504803f909...48c5939cf4`: fix tidy, note that also in `master` there is inconsistency: the prototype of `OpenNetworkConnection()` in `net.h` uses `strDest` whereas the implementation in `net.cpp` uses `pszDest`. Resolved by this PR.
> If you're updating the name anyways, maybe just get rid of the hungarian notation and call it `destination`?
Yes, I think this would be very nice to do. I avoided it because it would bloat the diff as a lot of lines inside the body of the functions will be
...