💬 maflcko commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2688094249)
Yeah, I'd say a simple dev-only test-only `-DALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR=ON` should address any concern where someone really does want to do it. The option could be mentioned in the error message that rejects an unclean build.
However, for most other developers (or non-developers), the option would be turned `OFF` by default and then hopefully reduce the wasted time and bug reports. (Another report from today: https://github.com/bitcoin/bitcoin/issues/31959#issuecomment-2687926573 ...)
...
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2688094249)
Yeah, I'd say a simple dev-only test-only `-DALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR=ON` should address any concern where someone really does want to do it. The option could be mentioned in the error message that rejects an unclean build.
However, for most other developers (or non-developers), the option would be turned `OFF` by default and then hopefully reduce the wasted time and bug reports. (Another report from today: https://github.com/bitcoin/bitcoin/issues/31959#issuecomment-2687926573 ...)
...
💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2688109088)
> * Managed to generate coverage for a functional tests, not sure if we should include it, includes copy-pasting temporary paths for the root test directory.
I think adding the coverage for functional tests and fuzz tests (which I believe everybody does using llvm toolchain only ?) can be added in another PR .
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2688109088)
> * Managed to generate coverage for a functional tests, not sure if we should include it, includes copy-pasting temporary paths for the root test directory.
I think adding the coverage for functional tests and fuzz tests (which I believe everybody does using llvm toolchain only ?) can be added in another PR .
💬 murchandamus commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2688114845)
That’s more than what I intended to test here, and especially doesn’t mention that some combinations would exceed the maximal weight. I would suggest:
```
// 3) Test that the lowest-weight solution is found when some combinations would exceed the allowed weight
```
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2688114845)
That’s more than what I intended to test here, and especially doesn’t mention that some combinations would exceed the maximal weight. I would suggest:
```
// 3) Test that the lowest-weight solution is found when some combinations would exceed the allowed weight
```
💬 janb84 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2688118800)
re ACK [05605da](https://github.com/bitcoin/bitcoin/commit/05605dae2983c11f0ba662dbdc495f84de12724b)
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2688118800)
re ACK [05605da](https://github.com/bitcoin/bitcoin/commit/05605dae2983c11f0ba662dbdc495f84de12724b)
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2688185365)
ACK fab51b26d68b37f5914314ad29eeb930538ea7ae
<details>
<summary>Diff</summary>
Compared to previously acked fa77685ba5c7185781acca04f57399cdcd19e9f7, `net.cpp` and `pcp_tests.cpp` had header & Span -> std::span changes only.
</details>
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2688185365)
ACK fab51b26d68b37f5914314ad29eeb930538ea7ae
<details>
<summary>Diff</summary>
Compared to previously acked fa77685ba5c7185781acca04f57399cdcd19e9f7, `net.cpp` and `pcp_tests.cpp` had header & Span -> std::span changes only.
</details>
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1841260812)
I wanted to use representative values, but also avoid using values whose multiples overlap. So, I used the vsize of P2WPKH inputs and outputs.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1841260812)
I wanted to use representative values, but also avoid using values whose multiples overlap. So, I used the vsize of P2WPKH inputs and outputs.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1841254171)
I just felt that it wasn’t necessary to have four UTXOs to replicate the same coverage. Rather I wanted to be explicit about the cases that are being tested and show that they all work, but I felt that three UTXOs were sufficient to achieve that.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1841254171)
I just felt that it wasn’t necessary to have four UTXOs to replicate the same coverage. Rather I wanted to be explicit about the cases that are being tested and show that they all work, but I felt that three UTXOs were sufficient to achieve that.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1841263796)
As mentioned in the [opening comment](https://github.com/bitcoin/bitcoin/pull/29532#issue-2164298794), I’m looking to completely change the approach. Instead of using absolute values and feerates of zero, transactions in the new tests use real feerates, and spend UTXOs whose effective values are round amounts. Since there are a ton of tests, I expected that it would be too much to get reviewed in a single PR, and migrating from the old test suite to the new test suite would make it obvious which
...
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1841263796)
As mentioned in the [opening comment](https://github.com/bitcoin/bitcoin/pull/29532#issue-2164298794), I’m looking to completely change the approach. Instead of using absolute values and feerates of zero, transactions in the new tests use real feerates, and spend UTXOs whose effective values are round amounts. Since there are a ton of tests, I expected that it would be too much to get reviewed in a single PR, and migrating from the old test suite to the new test suite would make it obvious which
...
💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r1973771126)
Hi @kevkevinpal !
I took your suggestion and added the test in 6109bc5
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r1973771126)
Hi @kevkevinpal !
I took your suggestion and added the test in 6109bc5
💬 ryanofsky commented on issue "RFC: multiprocess binaries in 29.0 or 30.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2688230690)
At this point `bitcoin-node` and `bitcoin-gui` binaries supporting an `-ipcbind` option will not make the 29.0 release. But it would still be helpful to know what specific concerns remain so we can figure out how to address them before the 30.0 release. In the issue description I listed 5 possible concerns: (1) and (2) are technical, (3) is about communication and (4) and (5) have to do with internal process and precedent. It would be very helpful to know from anyone with an opinion which of the
...
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2688230690)
At this point `bitcoin-node` and `bitcoin-gui` binaries supporting an `-ipcbind` option will not make the 29.0 release. But it would still be helpful to know what specific concerns remain so we can figure out how to address them before the 30.0 release. In the issue description I listed 5 possible concerns: (1) and (2) are technical, (3) is about communication and (4) and (5) have to do with internal process and precedent. It would be very helpful to know from anyone with an opinion which of the
...
💬 ryanofsky commented on issue "cmake: unable to build binaries from my fork repo but can do so from bitcoin repo":
(https://github.com/bitcoin/bitcoin/issues/31959#issuecomment-2688263542)
@maflcko do you have ideas on how this failure could have happened? I'm not sure how/whether the change discussed #31942 would address this, possibly because I don't understand exactly what change #31942 is proposing. From the error it seems like the SDK installation got updated between builds, but probably I am misinterpreting it.
(https://github.com/bitcoin/bitcoin/issues/31959#issuecomment-2688263542)
@maflcko do you have ideas on how this failure could have happened? I'm not sure how/whether the change discussed #31942 would address this, possibly because I don't understand exactly what change #31942 is proposing. From the error it seems like the SDK installation got updated between builds, but probably I am misinterpreting it.
👍 pablomartin4btc approved a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2648149962)
tACK 05605dae2983c11f0ba662dbdc495f84de12724b
<details>
<summary>Tested in both Ubuntu 22.04 (with clang 16) and macOS 14.4</summary>
</details>
Maybe we could advice the user about the warnings, left a comment there.
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2648149962)
tACK 05605dae2983c11f0ba662dbdc495f84de12724b
<details>
<summary>Tested in both Ubuntu 22.04 (with clang 16) and macOS 14.4</summary>
</details>
Maybe we could advice the user about the warnings, left a comment there.
💬 pablomartin4btc commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1973803086)
> Its harmless and can be ignored.
I got it in both Ubuntu and macOS. Perhaps we can document it as well to avoid confusion?
```
$ find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata
build/src/univalue/default_44199.profraw: main: function basic block count change detected (counter mismatch)
Make sure that all profile data to be merged is generated from the same binary.
$ ll build/coverage.profdata
... 15926688 feb 27 09:44 build/coverag
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1973803086)
> Its harmless and can be ignored.
I got it in both Ubuntu and macOS. Perhaps we can document it as well to avoid confusion?
```
$ find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata
build/src/univalue/default_44199.profraw: main: function basic block count change detected (counter mismatch)
Make sure that all profile data to be merged is generated from the same binary.
$ ll build/coverage.profdata
... 15926688 feb 27 09:44 build/coverag
...
💬 maflcko commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2688377140)
@ryanofsky The diff I am proposing would be:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 94464907de..79ea015b78 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -126,6 +126,16 @@ cmake_dependent_option(BUILD_WALLET_TOOL "Build bitcoin-wallet tool." ${BUILD_TE
option(ENABLE_HARDENING "Attempt to harden the resulting executables." ON)
option(REDUCE_EXPORTS "Attempt to reduce exported symbols in the resulting executables." OFF)
+
+option(ALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR "
...
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2688377140)
@ryanofsky The diff I am proposing would be:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 94464907de..79ea015b78 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -126,6 +126,16 @@ cmake_dependent_option(BUILD_WALLET_TOOL "Build bitcoin-wallet tool." ${BUILD_TE
option(ENABLE_HARDENING "Attempt to harden the resulting executables." ON)
option(REDUCE_EXPORTS "Attempt to reduce exported symbols in the resulting executables." OFF)
+
+option(ALLOW_CONFIGURE_ON_UNCLEAN_BUILD_DIR "
...
💬 purpleKarrot commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r1973892213)
Data members should not be `const`. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-constref
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r1973892213)
Data members should not be `const`. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-constref
🤔 janb84 reviewed a pull request: "contrib: Add deterministic-unittest-coverage"
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2648277252)
Concept ACK [fa99c3b](https://github.com/bitcoin/bitcoin/commit/fa99c3b544b631cfe34d52fb5e71636aedb1b423)
- Cloned & Build
- Runned the tooling, as instructed per Readme
- Reviewed the code
As a side note:
I have run into the same non-deterministic issues with util_string_tests or util_tests as [brunoerg](https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684942423) although not important for this PR
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2648277252)
Concept ACK [fa99c3b](https://github.com/bitcoin/bitcoin/commit/fa99c3b544b631cfe34d52fb5e71636aedb1b423)
- Cloned & Build
- Runned the tooling, as instructed per Readme
- Reviewed the code
As a side note:
I have run into the same non-deterministic issues with util_string_tests or util_tests as [brunoerg](https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684942423) although not important for this PR
💬 janb84 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1973882490)
Also add it here:
```suggestion
In addition to git, both llvm-profdata and llvm-cov must be installed.
```
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1973882490)
Also add it here:
```suggestion
In addition to git, both llvm-profdata and llvm-cov must be installed.
```
💬 janb84 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1973878899)
Because git is now a hard dependency, I would suggest to change the readme to:
```suggestion
In addition to git, both llvm-profdata and llvm-cov must be installed. Also, the qa-assets
```
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1973878899)
Because git is now a hard dependency, I would suggest to change the readme to:
```suggestion
In addition to git, both llvm-profdata and llvm-cov must be installed. Also, the qa-assets
```
💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1973903540)
> I got it in both Ubuntu and macOS. Perhaps we can document it as well to avoid confusion?
>
> ```
> $ find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata
> build/src/univalue/default_44199.profraw: main: function basic block count change detected (counter mismatch)
> Make sure that all profile data to be merged is generated from the same binary.
> ```
Have you tried building with clang 19 ? I dont get this on MacOs
```
╰─ find build -
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1973903540)
> I got it in both Ubuntu and macOS. Perhaps we can document it as well to avoid confusion?
>
> ```
> $ find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata
> build/src/univalue/default_44199.profraw: main: function basic block count change detected (counter mismatch)
> Make sure that all profile data to be merged is generated from the same binary.
> ```
Have you tried building with clang 19 ? I dont get this on MacOs
```
╰─ find build -
...
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1973909534)
Not sure. If someone has the source code, they likely got it via git. If someone has compiled with clang, they likely have llvm tools. So it would be better to just remove the sentence.
In any case, I'll leave this nit as-is for now, to not invalidate the three acks.
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1973909534)
Not sure. If someone has the source code, they likely got it via git. If someone has compiled with clang, they likely have llvm tools. So it would be better to just remove the sentence.
In any case, I'll leave this nit as-is for now, to not invalidate the three acks.