👋 hebasto's pull request is ready for review: "build: Rename `PACKAGE_*` variables to `CLIENT_*`"
(https://github.com/bitcoin/bitcoin/pull/31042)
(https://github.com/bitcoin/bitcoin/pull/31042)
💬 vasild commented on pull request "fuzz: fuzz connman with non-empty addrman + ASMap":
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2407171509)
Concept ACK
The conflict with https://github.com/bitcoin/bitcoin/pull/28584 is only mechanical.
This PR changes "the contents" of the `CConnman` and does not add fuzzing to more connman methods (except `connman.ASMapHealthCheck()`). The main point of this PR is that methods that are fuzzed even before this PR would behave differently if addrman is not empty. Nice.
https://github.com/bitcoin/bitcoin/pull/28584 adds fuzzing to methods not fuzzed before: `OpenNetworkConnection()`, `Create
...
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2407171509)
Concept ACK
The conflict with https://github.com/bitcoin/bitcoin/pull/28584 is only mechanical.
This PR changes "the contents" of the `CConnman` and does not add fuzzing to more connman methods (except `connman.ASMapHealthCheck()`). The main point of this PR is that methods that are fuzzed even before this PR would behave differently if addrman is not empty. Nice.
https://github.com/bitcoin/bitcoin/pull/28584 adds fuzzing to methods not fuzzed before: `OpenNetworkConnection()`, `Create
...
💬 fanquake commented on pull request "build: Add missing USDT header dependency to kernel":
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2407187265)
> Currently depends builds pick up usdt and boost from the same path, and because boost always exists, the usdt path is implicitly included. So without boost, USDT isn't found.
To clarify for myself, the build currently picks up all depends dependencies from the same path (i.e something like `-isystem /root/ci_scratch/depends/x86_64-pc-linux-gnu/include`, (which is what is introduced with the change here)), so using any dependency will implictly include all others. It's just the case that the
...
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2407187265)
> Currently depends builds pick up usdt and boost from the same path, and because boost always exists, the usdt path is implicitly included. So without boost, USDT isn't found.
To clarify for myself, the build currently picks up all depends dependencies from the same path (i.e something like `-isystem /root/ci_scratch/depends/x86_64-pc-linux-gnu/include`, (which is what is introduced with the change here)), so using any dependency will implictly include all others. It's just the case that the
...
💬 maflcko commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407190445)
If this really was an issue, adding back the build patches would probably be easier than backporting all bugfixes for years: https://github.com/bitcoin/bitcoin/commit/0c57a798b50bf03a0b69f632c691932d608254ef#diff-42e88f6b2dc3f513ccde184c74d1853a01b2bfc3ee64debf98c52efe316b04c1L456
However, I still don't understand why this is an issue.
Maybe I am missing something obvious, but at least locally I can call `./configure` on the `28.x` branch just fine on Alma8:
```
dnf install gcc-toolset
...
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407190445)
If this really was an issue, adding back the build patches would probably be easier than backporting all bugfixes for years: https://github.com/bitcoin/bitcoin/commit/0c57a798b50bf03a0b69f632c691932d608254ef#diff-42e88f6b2dc3f513ccde184c74d1853a01b2bfc3ee64debf98c52efe316b04c1L456
However, I still don't understand why this is an issue.
Maybe I am missing something obvious, but at least locally I can call `./configure` on the `28.x` branch just fine on Alma8:
```
dnf install gcc-toolset
...
📝 ryanofsky opened a pull request: "scripted-diff: Replace strprintf(Untranslated(...)) with Untranslated(strprintf(...))"
(https://github.com/bitcoin/bitcoin/pull/31072)
This PR is just a scripted-diff replacing strprintf calls like:
```c++
strprintf(Untranslated("Error parsing command line arguments: %s"), error))
```
with:
```c++
Untranslated(strprintf("Error parsing command line arguments: %s", error))
```
Currently code is inconsistent and uses a mix of both styles, but second style is better because:
1. It scans more cleanly. It is easier to read the strprintf call, and easier to think about because the whole string, not just one part, is
...
(https://github.com/bitcoin/bitcoin/pull/31072)
This PR is just a scripted-diff replacing strprintf calls like:
```c++
strprintf(Untranslated("Error parsing command line arguments: %s"), error))
```
with:
```c++
Untranslated(strprintf("Error parsing command line arguments: %s", error))
```
Currently code is inconsistent and uses a mix of both styles, but second style is better because:
1. It scans more cleanly. It is easier to read the strprintf call, and easier to think about because the whole string, not just one part, is
...
💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1796886224)
Thanks! Reworked per feedback.
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1796886224)
Thanks! Reworked per feedback.
💬 hebasto commented on pull request "build: Add missing USDT header dependency to kernel":
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2407322051)
> Are you saying you think we shouldn't be able to include system headers when building using depends?
Yes, for headers-only packages built in depends, such as Boost and USDT. t's for the same reason, why we got rid of the `ALLOW_HOST_PACKAGES` variable.
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2407322051)
> Are you saying you think we shouldn't be able to include system headers when building using depends?
Yes, for headers-only packages built in depends, such as Boost and USDT. t's for the same reason, why we got rid of the `ALLOW_HOST_PACKAGES` variable.
💬 fanquake commented on pull request "build: Add missing USDT header dependency to kernel":
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2407334296)
> Yes, for headers-only packages built in depends, such as Boost and USDT.
Headers-only shouldn't make any difference here, and this should already be the case for Boost. If it's not, that should be fixed. See my point above re USDT, I don't understand how you propose to do this, given that the USDT headers exist amongst the libc headers.
(https://github.com/bitcoin/bitcoin/pull/30970#issuecomment-2407334296)
> Yes, for headers-only packages built in depends, such as Boost and USDT.
Headers-only shouldn't make any difference here, and this should already be the case for Boost. If it's not, that should be fixed. See my point above re USDT, I don't understand how you propose to do this, given that the USDT headers exist amongst the libc headers.
👍 fanquake approved a pull request: "build: Add missing USDT header dependency to kernel"
(https://github.com/bitcoin/bitcoin/pull/30970#pullrequestreview-2362721686)
ACK ccd10fdb97f9b8268a5cd60d7461967cfe536f16
(https://github.com/bitcoin/bitcoin/pull/30970#pullrequestreview-2362721686)
ACK ccd10fdb97f9b8268a5cd60d7461967cfe536f16
💬 ArmchairCryptologist commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407340807)
> Do you have any further information on how you conclude that more than half of Bitcoin Core installations run on these OSes (and their derivatives)? This number doesn't feel right to me, even as a (high) estimate.
> This report: https://repology.org/project/glibc/badges shows that glibc >=2.31 is supported by:
It is mostly a hunch based on my own experience with the server hosting industry. Most of the distros mentioned here are not generally considered stable for server usage, and are onl
...
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407340807)
> Do you have any further information on how you conclude that more than half of Bitcoin Core installations run on these OSes (and their derivatives)? This number doesn't feel right to me, even as a (high) estimate.
> This report: https://repology.org/project/glibc/badges shows that glibc >=2.31 is supported by:
It is mostly a hunch based on my own experience with the server hosting industry. Most of the distros mentioned here are not generally considered stable for server usage, and are onl
...
🚀 fanquake merged a pull request: "build: Add missing USDT header dependency to kernel"
(https://github.com/bitcoin/bitcoin/pull/30970)
(https://github.com/bitcoin/bitcoin/pull/30970)
💬 sipa commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407347013)
*If* there is significant demand for running Bitcoin Core on RH8, I think it would make sense for us to consider adding the glibc workarounds necessary so that guix-built binaries support glibc 2.28. I suspect that is easier than backporting fixes to 27.x for longer, and doesn't preclude RH8 users from getting the features added in 28.x and later.
I don't think the possibility of self-compiling on RH8 is a full replacement for project binaries that just work. Presumably people who use RH8 want
...
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407347013)
*If* there is significant demand for running Bitcoin Core on RH8, I think it would make sense for us to consider adding the glibc workarounds necessary so that guix-built binaries support glibc 2.28. I suspect that is easier than backporting fixes to 27.x for longer, and doesn't preclude RH8 users from getting the features added in 28.x and later.
I don't think the possibility of self-compiling on RH8 is a full replacement for project binaries that just work. Presumably people who use RH8 want
...
💬 hebasto commented on pull request "guix: use GCC 13 to build releases":
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2407347771)
Concept ACK. May be useful for switching to [Qt 6.8 or newer.](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346)
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2407347771)
Concept ACK. May be useful for switching to [Qt 6.8 or newer.](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346)
💬 fanquake commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407347872)
> if it still trivially compiles against it.
Having to maintain support for compiling well EOL glibcs is not trivial, and continues to get harder as the compilers and toolchains used by the project modernise. It's already the case that on the next update to our release (Guix) build environment, our current glibc (2.31), will stop compiling without further patching, because the newer binutils (2.41), can't compile it when targeting riscv.
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407347872)
> if it still trivially compiles against it.
Having to maintain support for compiling well EOL glibcs is not trivial, and continues to get harder as the compilers and toolchains used by the project modernise. It's already the case that on the next update to our release (Guix) build environment, our current glibc (2.31), will stop compiling without further patching, because the newer binutils (2.41), can't compile it when targeting riscv.
💬 sipa commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407368227)
@ArmchairCryptologist There is a difference between (1) our source code being able to be compiled against an old glibc, and then working on that old glibc and (2) the release process being able to run on modern platforms (and making use of the features that offers, including targetting modern hardware) and the resulting binaries working on old systems.
Of course, one possibility would be to have multiple release builds, for example a separate one targetting old-glibc x86_64 systems only. So far
...
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407368227)
@ArmchairCryptologist There is a difference between (1) our source code being able to be compiled against an old glibc, and then working on that old glibc and (2) the release process being able to run on modern platforms (and making use of the features that offers, including targetting modern hardware) and the resulting binaries working on old systems.
Of course, one possibility would be to have multiple release builds, for example a separate one targetting old-glibc x86_64 systems only. So far
...
💬 maflcko commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407391721)
> for example a separate one targetting old-glibc x86_64
Looking at the patches, it may be possible to do a x86_64 guix build (maybe even aarch64) with glibc1.28, without adding any patches back, as they relate to other architectures. However, I haven't tried this and don't know how involved it would be.
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407391721)
> for example a separate one targetting old-glibc x86_64
Looking at the patches, it may be possible to do a x86_64 guix build (maybe even aarch64) with glibc1.28, without adding any patches back, as they relate to other architectures. However, I haven't tried this and don't know how involved it would be.
💬 jonatack commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2407408687)
Thanks!
> I think we should just unify `settxfee` and your `setfeerate` into one RPC?
I'm not sure. Point 3 and the rationales in https://github.com/bitcoin/bitcoin/pull/20484#issuecomment-734786305 suggest a possibly safer way (perhaps you have a workaround in mind I'm overlooking).
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2407408687)
Thanks!
> I think we should just unify `settxfee` and your `setfeerate` into one RPC?
I'm not sure. Point 3 and the rationales in https://github.com/bitcoin/bitcoin/pull/20484#issuecomment-734786305 suggest a possibly safer way (perhaps you have a workaround in mind I'm overlooking).
💬 sipa commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407411552)
@maflcko I assume you mean glibc 2.28 and 2.27? What would be the downside of only having glibc 2.27 release binaries for x86_64 (and/or aarch64)?
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407411552)
@maflcko I assume you mean glibc 2.28 and 2.27? What would be the downside of only having glibc 2.27 release binaries for x86_64 (and/or aarch64)?
💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2407431612)
> from C++ one has greater control and observability - the test can read any global or member variable and can call arbitrary functions
> Also, when used from within C++ one does not have to re-implement the transport code in Python. From C++ we can use the existent code to parse or craft the socket data.
For these reasons, along with the speed of running the tests and the simplicity and robustness of using the same language for tests and code, I prefer to write test code in C++ when I can
...
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2407431612)
> from C++ one has greater control and observability - the test can read any global or member variable and can call arbitrary functions
> Also, when used from within C++ one does not have to re-implement the transport code in Python. From C++ we can use the existent code to parse or craft the socket data.
For these reasons, along with the speed of running the tests and the simplicity and robustness of using the same language for tests and code, I prefer to write test code in C++ when I can
...
💬 jonatack commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2407438275)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2407438275)
Concept ACK