💬 wincss commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2593329870)
> > Prevent startup when user provided a -blockreservedweight lower than 2000 WU
>
> Maybe @wangchun from F2Pool can confirm if this would work for F2Pool and how many weigh units they currently reserve. And if they do it with a custom patch or `-maxblockweight`.
We (F2Pool) reserved 2000 WU for the coinbase transaction. This was achieved by patching the source code, ensuring that the check in `src/init.cpp` does not affect us.
In fact, we do not face the "duplicate reservation" issue bec
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2593329870)
> > Prevent startup when user provided a -blockreservedweight lower than 2000 WU
>
> Maybe @wangchun from F2Pool can confirm if this would work for F2Pool and how many weigh units they currently reserve. And if they do it with a custom patch or `-maxblockweight`.
We (F2Pool) reserved 2000 WU for the coinbase transaction. This was achieved by patching the source code, ensuring that the check in `src/init.cpp` does not affect us.
In fact, we do not face the "duplicate reservation" issue bec
...
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593335197)
High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I'd rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and _also_ we would be ok with that failure?
If not, we could just skip the complexity and not modify the policy at all.
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593335197)
High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I'd rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and _also_ we would be ok with that failure?
If not, we could just skip the complexity and not modify the policy at all.
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593345160)
> High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I'd rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and _also_ we would be ok with that failure?
>
> If not, we could just skip the complexity and not modify the policy at all.
From a quick look at the new uses of `bitcoin_check_cxx_source_compiles` here, only `check_
...
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593345160)
> High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I'd rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and _also_ we would be ok with that failure?
>
> If not, we could just skip the complexity and not modify the policy at all.
From a quick look at the new uses of `bitcoin_check_cxx_source_compiles` here, only `check_
...
💬 hebasto commented on pull request "cmake: Add `CheckLinkerSupportsPIE` module":
(https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2593347683)
> I forgot that we set `CMAKE_TRY_COMPILE_TARGET_TYPE` globally. And even worse, it's buried in a module. If that upsets CMake internal tests, I think we should undo that.
Undone in https://github.com/bitcoin/bitcoin/pull/31662.
> What is the status of this?
Updated and ready for review.
(https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2593347683)
> I forgot that we set `CMAKE_TRY_COMPILE_TARGET_TYPE` globally. And even worse, it's buried in a module. If that upsets CMake internal tests, I think we should undo that.
Undone in https://github.com/bitcoin/bitcoin/pull/31662.
> What is the status of this?
Updated and ready for review.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2593392797)
@wincss thanks for the clarification. You should be able to use `-blockreservedweight=2000 -maxblockweight` after this change. Is there anything else you need a patch for? Fee free to open a Github issue.
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2593392797)
@wincss thanks for the clarification. You should be able to use `-blockreservedweight=2000 -maxblockweight` after this change. Is there anything else you need a patch for? Fee free to open a Github issue.
💬 luke-jr commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2593426552)
eg https://github.com/luke-jr/bitcoin/commit/56ee485533820d8c7f0e4e27acd712aa8b411286
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2593426552)
eg https://github.com/luke-jr/bitcoin/commit/56ee485533820d8c7f0e4e27acd712aa8b411286
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593426987)
I tried making this change (simply removing the global `CMAKE_TRY_COMPILE_TARGET_TYPE` assignment and ended up with an identical `bitcoin-build-config.h`.
It was .4 sec slower, but that's not enough to care about :)
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593426987)
I tried making this change (simply removing the global `CMAKE_TRY_COMPILE_TARGET_TYPE` assignment and ended up with an identical `bitcoin-build-config.h`.
It was .4 sec slower, but that's not enough to care about :)
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593463805)
> High-level question: Do we actually need the old autotools behavior?
That was a strict requirement for the staging branch, and mapping theAutotools' `AC_PREPROC_IFELSE`, `AC_COMPILE_IFELSE` and `AC_LINK_IFELSE` macros to CMake functions was essential.
> Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and _also_ we would be ok with that failure?
I don't think so.
> Is there an obvious reason I'm missing to not just use CMake's d
...
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593463805)
> High-level question: Do we actually need the old autotools behavior?
That was a strict requirement for the staging branch, and mapping theAutotools' `AC_PREPROC_IFELSE`, `AC_COMPILE_IFELSE` and `AC_LINK_IFELSE` macros to CMake functions was essential.
> Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and _also_ we would be ok with that failure?
I don't think so.
> Is there an obvious reason I'm missing to not just use CMake's d
...
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593511535)
> > High-level question: Do we actually need the old autotools behavior?
>
> That was a strict requirement for the staging branch, and mapping the Autotools' `AC_PREPROC_IFELSE`, `AC_COMPILE_IFELSE` and `AC_LINK_IFELSE` macros to CMake functions was essential.
>
Sure, I didn't mean to imply that we were doing the wrong thing here. Only that we've learned that it causes problems and the 1:1 matching with autotools is problematic.
> > Is there an obvious reason I'm missing to not just u
...
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593511535)
> > High-level question: Do we actually need the old autotools behavior?
>
> That was a strict requirement for the staging branch, and mapping the Autotools' `AC_PREPROC_IFELSE`, `AC_COMPILE_IFELSE` and `AC_LINK_IFELSE` macros to CMake functions was essential.
>
Sure, I didn't mean to imply that we were doing the wrong thing here. Only that we've learned that it causes problems and the 1:1 matching with autotools is problematic.
> > Is there an obvious reason I'm missing to not just u
...
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593602568)
> Proposed alternative to this PR: https://github.com/theuni/bitcoin/commits/cmake-trycompile-exe/
> What do you think?
Thank you. It Looks great!
I also pushed an updated branch, which has a cleaner resulting code.
However, I'm happy to switch to your branch, if you prefer it.
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593602568)
> Proposed alternative to this PR: https://github.com/theuni/bitcoin/commits/cmake-trycompile-exe/
> What do you think?
Thank you. It Looks great!
I also pushed an updated branch, which has a cleaner resulting code.
However, I'm happy to switch to your branch, if you prefer it.
📝 hebasto converted_to_draft a pull request: "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally"
(https://github.com/bitcoin/bitcoin/pull/31662)
This was requested in https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092.
From https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2511246212:
> (Almost?) every CMake check internally uses the [`try_compile()`](https://cmake.org/cmake/help/latest/command/try_compile.html) command, whose behaviour, in turn, depends on the [`CMAKE_TRY_COMPILE_TARGET_TYPE`](https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_TARGET_TYPE.html) variable:
>
> 1. The defau
...
(https://github.com/bitcoin/bitcoin/pull/31662)
This was requested in https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092.
From https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2511246212:
> (Almost?) every CMake check internally uses the [`try_compile()`](https://cmake.org/cmake/help/latest/command/try_compile.html) command, whose behaviour, in turn, depends on the [`CMAKE_TRY_COMPILE_TARGET_TYPE`](https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_TARGET_TYPE.html) variable:
>
> 1. The defau
...
👋 hebasto's pull request is ready for review: "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally"
(https://github.com/bitcoin/bitcoin/pull/31662)
(https://github.com/bitcoin/bitcoin/pull/31662)
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593680026)
A typo has been fixed (
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2593680026)
A typo has been fixed (
⚠️ darosior opened an issue: "Enable PCP by default?"
(https://github.com/bitcoin/bitcoin/issues/31663)
Centralize discussion around turning the `natpmp` setting to on by default.
UPnP used to be turned on by default. It was turned off by default following numerous vulnerabilities found in miniupnpc. We recently got rid of the miniupnpc dependency by dropping the UPnP feature (https://github.com/bitcoin/bitcoin/pull/31130). In addition we implemented PCP with NAT-PMP fallback in house (https://github.com/bitcoin/bitcoin/pull/30043), safer protocols enabling the same NAT traversal feature to end u
...
(https://github.com/bitcoin/bitcoin/issues/31663)
Centralize discussion around turning the `natpmp` setting to on by default.
UPnP used to be turned on by default. It was turned off by default following numerous vulnerabilities found in miniupnpc. We recently got rid of the miniupnpc dependency by dropping the UPnP feature (https://github.com/bitcoin/bitcoin/pull/31130). In addition we implemented PCP with NAT-PMP fallback in house (https://github.com/bitcoin/bitcoin/pull/30043), safer protocols enabling the same NAT traversal feature to end u
...
💬 fjahr commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2593757745)
Post-merge tACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2593757745)
Post-merge tACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
💬 darosior commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2593761021)
To be clear i'm not suggesting we turn this option on by default in 29, this merely opens the discussion more formally than yesterday's chat on IRC. And i started writing this mainly to centralize results of tests against various routers.
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2593761021)
To be clear i'm not suggesting we turn this option on by default in 29, this merely opens the discussion more formally than yesterday's chat on IRC. And i started writing this mainly to centralize results of tests against various routers.
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917235927)
Could you add a `LINK_LIBRARIES` option to `check_cxx_source_compiles_with_flags` and use that here instead?
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917235927)
Could you add a `LINK_LIBRARIES` option to `check_cxx_source_compiles_with_flags` and use that here instead?
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917234974)
Could you add an LDFLAGS param to the function as `LINK_OPTIONS` and use that here instead?
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917234974)
Could you add an LDFLAGS param to the function as `LINK_OPTIONS` and use that here instead?
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917262536)
`LDFLAGS` might make more sense because, for example, in the OSS-Fuzz environment, the `SANITIZER_LDFLAGS` variable can be set to `-fsanitize=fuzzer` or `/usr/lib/libFuzzingEngine.a`.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917262536)
`LDFLAGS` might make more sense because, for example, in the OSS-Fuzz environment, the `SANITIZER_LDFLAGS` variable can be set to `-fsanitize=fuzzer` or `/usr/lib/libFuzzingEngine.a`.
💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker build`.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1917272201)
> Does this env var work for podman as well?
The "podman" cli frontend doesn't use this environment variable[^1], but I tested locally (on Fedora 40) that this PR works for the build phase when using docker cli to interact with podman container host:
```console
$ sudo dnf install podman-machine
$ podman machine init
$ podman machine start
Starting machine "podman-machine-default"
API forwarding listening on: /run/user/1000/podman/podman-machine-default-api.sock
$ env -i HOME="$HOME"
...
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1917272201)
> Does this env var work for podman as well?
The "podman" cli frontend doesn't use this environment variable[^1], but I tested locally (on Fedora 40) that this PR works for the build phase when using docker cli to interact with podman container host:
```console
$ sudo dnf install podman-machine
$ podman machine init
$ podman machine start
Starting machine "podman-machine-default"
API forwarding listening on: /run/user/1000/podman/podman-machine-default-api.sock
$ env -i HOME="$HOME"
...