💬 darosior commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2593052385)
An alternative to touching consensus code to test this is to fuzz `EvalScript` directly. We already have a target for it, `eval_script`. It could be adapted to also be ran under a Tapscript context by filling dummy `ScriptExecutionData`, and the soft fork check could be added to this target. The downside of this approach is that it would not cover the `VerifyScript` logic, but in the context of making sure proposed opcodes are indeed soft forks what we really care about it `EvalScript`.
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2593052385)
An alternative to touching consensus code to test this is to fuzz `EvalScript` directly. We already have a target for it, `eval_script`. It could be adapted to also be ran under a Tapscript context by filling dummy `ScriptExecutionData`, and the soft fork check could be added to this target. The downside of this approach is that it would not cover the `VerifyScript` logic, but in the context of making sure proposed opcodes are indeed soft forks what we really care about it `EvalScript`.
💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1916790039)
Sounds good, I kept the check and adjusted the comment in a separate commit. Let me know if it makes sense or can be improved in any way.
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1916790039)
Sounds good, I kept the check and adjusted the comment in a separate commit. Let me know if it makes sense or can be improved in any way.
👍 ryanofsky approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2553008614)
Code review ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant `inline` keyword
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2553008614)
Code review ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant `inline` keyword
💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2593155279)
> Why are `bitcoin-chainstate` and `bitcoin-util` being put into libexec? `bitcoin-util` is meant to be called by users directly. Not sure why `bitcoin-chainstate` would be put there either?
Agreed. The point of libexec is (implicitly) to make it HARDER to run binaries from there, because they don't live in the user's `$PATH`. They're meant to be found by other binaries that _do_ live in `$PATH`. So long as we're recommending/relying on users running a binary by hand, it shouldn't live in lib
...
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2593155279)
> Why are `bitcoin-chainstate` and `bitcoin-util` being put into libexec? `bitcoin-util` is meant to be called by users directly. Not sure why `bitcoin-chainstate` would be put there either?
Agreed. The point of libexec is (implicitly) to make it HARDER to run binaries from there, because they don't live in the user's `$PATH`. They're meant to be found by other binaries that _do_ live in `$PATH`. So long as we're recommending/relying on users running a binary by hand, it shouldn't live in lib
...
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2593227168)
Updated 430d5feee9b8be3a85e85696c449753783301c2b -> 2a92702bafca5c78b270a9502a22cb9deac02cfc ([kernel_cache_sizes_16](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_16) -> [kernel_cache_sizes_17](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_17), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_16..kernel_cache_sizes_17))
* Took @stickies-v's [suggestion](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916527403) for si
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2593227168)
Updated 430d5feee9b8be3a85e85696c449753783301c2b -> 2a92702bafca5c78b270a9502a22cb9deac02cfc ([kernel_cache_sizes_16](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_16) -> [kernel_cache_sizes_17](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_17), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_16..kernel_cache_sizes_17))
* Took @stickies-v's [suggestion](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916527403) for si
...
💬 maflcko commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2593247012)
> MSVC
Has someone reported the request to them? If not, it seems less likely they'll fix it.
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2593247012)
> MSVC
Has someone reported the request to them? If not, it seems less likely they'll fix it.
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2593266357)
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc
No changes except for outstanding review comments to simplify `CheckedLeftShift()`'s implementation wrt range checks, making the `_MiB` test platform-agnostic, making `IndexCacheSizes` members `size_t` and doc/typo improvements.
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2593266357)
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc
No changes except for outstanding review comments to simplify `CheckedLeftShift()`'s implementation wrt range checks, making the `_MiB` test platform-agnostic, making `IndexCacheSizes` members `size_t` and doc/typo improvements.
📝 hebasto opened 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
...
✅ fanquake closed an issue: "Nuke format linter"
(https://github.com/bitcoin/bitcoin/issues/30530)
(https://github.com/bitcoin/bitcoin/issues/30530)
🚀 fanquake merged a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061)
(https://github.com/bitcoin/bitcoin/pull/31061)
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2593311499)
@fanquake @theuni
Thank you for your feedback!
> So I propose dropping the `libexec` stuff entirely for this PR.
Dropped.
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2593311499)
@fanquake @theuni
Thank you for your feedback!
> So I propose dropping the `libexec` stuff entirely for this PR.
Dropped.
💬 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
...