💬 fanquake commented on issue "cmake: incorrect assumption that `Debug` build type will not use optimisations":
(https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2329444845)
> I think we should first prioritize coordinating the optimization and debugging flags between the depends subsystem and the main build system:

I'm not sure? This hardening feature just be turned on if optimisations are being used, and hardening isn't disabled. Whether or not depends flags override CMake defaults, or if depends defaults match CMake defaults seems tangential.

👍 l0rinc approved a pull request: "lint: Check for release note snippets in the wrong folder"
(https://github.com/bitcoin/bitcoin/pull/30812#pullrequestreview-2280625657)
Approach ACK

💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744042377)
a different linebreak would make this slightly more readable:
```suggestion
/// A possible error returned by any of the linters.
/// The error string should explain the failure type and list all violations.
```

💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744041608)
Checked the code with multiple invalid files, they were all detected correctly:
```
doc/release-notes/release-notes-27064.md
doc/release-notes/release-notes-27064_b.md
^^^
Release note snippets must be put into the doc/ folder directly.
```

💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744055191)
I think we can make this more idiomatic Rust by:
* using regex to match the name instead of string manipulations, repeating parts of the path and the file's structure (extension + dot);
* match a slice of the files to return a `Result`;
* extract intermediary results for clarity;
* we could like use [fs::read_dir("doc/release-notes")](https://doc.rust-lang.org/std/fs/fn.read_dir.html) to iterate the folder, but I see that delegating to `git` is common here:
```Rust
fn lint_doc_release_note
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744056376)
I'm definitely happy to put the existing tests back in if it makes review easier, but I want to make sure I understand your concern, so let me try to summarize it:

Prior to https://github.com/bitcoin/bitcoin/commit/facf629ce8ff1d1f6d254dde4e89b5018f8df60e, to construct a `arith_uint256` from a hex string, one would use the `arith_uint256` string constructor, and as such we had tests covering that constructor. Since https://github.com/bitcoin/bitcoin/commit/facf629ce8ff1d1f6d254dde4e89b5018f8d
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2329458177)
Force-pushed to increase `uint256`/`arith_uint256` conversion tests (hopefully) addressing @maflcko's [concern](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743729122), (or at the very least just improving test coverage). To avoid changing the same lines multiple times, I've squashed the `conversion` move commit into 6cfa7f4a0361d9c396d1c5bd71849295baf6290d.

Also adopted two style nits on [happy-path](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743693696) and [lim
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744062744)
Done.

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744063063)
Adopted both suggestions, thanks!

👍 tdb3 approved a pull request: "build: Fix / improve coverage scripts"
(https://github.com/bitcoin/bitcoin/pull/30772#pullrequestreview-2280663264)
cr re ACK d9fcbfc3727e06e6f57d4ab09861f3212d558426

💬 maflcko commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744076957)
> using regex to match the name instead of string manipulations, repeating parts of the path and the file's structure (extension + dot);

Seems overkill to import a dependency to check whether a dot is present in a string view or not.



> match a slice of the files to return a `Result`; extract intermediary results for clarity;

Sure, I'll try that tomorrow.


> we could like use [fs::read_dir("doc/release-notes")](https://doc.rust-lang.org/std/fs/fn.read_dir.html) to iterate the fol
...

⚠️ fanquake opened an issue: "build: reproducibility issue with macOS Guix builds"
(https://github.com/bitcoin/bitcoin/issues/30815)
There seems to be a recurring (permissions related?) issue in the macOS guix builds, that is causing non-determinsm. It seems to have started since the CMake switchover. Copied in the related discussion / diff of comparing two outputs from #30743:

> EDIT: Probably something with the zipping again?

> Yea, looks like a permissions issue:

#### Comparing `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.fanquake` & `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.thecharlatan`


...

👍 l0rinc approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280694965)
ACK b60013ed71b697bab98884aa475fb64ae7736c2e

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744081326)
so these are basically roundtrip tests now, right?
These are very valuable (could even be a fuzz test now), but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning), i.e. something like:
```C++
BOOST_CHECK_EQUAL(UintToArith256(ZeroL), 0);
BOOST_CHECK_EQUAL(UintToArith256(OneL), 1);
BOOST_CHECK_EQUAL(ArithToUint256(0), ZeroL);
BOOST_CHECK_EQUAL(ArithToUint256(1), OneL);

BOOST_CHECK_EQUAL(ArithT
...

🤔 ismaelsadeeq reviewed a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2280706719)


Tested ACK e9ad9e01409eaa2009f75deb1cf834983c5a6137

I was able to recreate the issue locally when running the `p2p_permissions` test while `bitcoind` was running on `regtest`.

<details>
<summary>Before</summary>

```terminal
abubakarismail@Abubakars-MacBook-Pro bitcoin % build/test/functional/p2p_permissions.py
2024-09-01T12:48:15.964000Z TestFramework (INFO): PRNG seed is: 6689536799961214212
2024-09-01T12:48:15.964000Z TestFramework (INFO): Initializing test directory /var/fold
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744104279)
> so these are basically roundtrip tests now, right?

I think `BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith);` is the only roundtrip?

> but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning):

Isn't that covered here? https://github.com/bitcoin/bitcoin/pull/30773/files#diff-d06d0fcd312512f190cc0d05450dd55973c60aeb8d57ed2d705e7beaa383f386R565-R568

💬 ryanofsky commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1744107968)
Followup points:

- I misunderstood "the build system may assign" to mean "cmake may assign" but actually it is referring to our own [ProcessConfigurations.cmake](https://github.com/bitcoin/bitcoin/blob/b8d2f58e06439e9523c02ca1729ff20cd0ba53ca/cmake/module/ProcessConfigurations.cmake#L145) file. So I think a more accurate comment than what I suggested would be "Set CMAKE_{C,CXX}\_FLAGS_DEBUG to empty so the default options normally used for debug builds do not override MSAN options in CMAKE_{C
...

💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744124233)
Thanks for looking into it @marcofleon! If there are no operational concerns adding the test cases, I'm happy with it.

I guess if we keep this, the `uint256::FromHex()` asserts have become superfluous and could be removed. Shouldn't make a meaningful performance impact (since they're called much less often than the `FromUserHex()` asserts, so it's more of a thing to keep the test code lean.

<details>
<summary>git diff on 6fa4c92e4c</summary>

```diff
diff --git a/src/test/fuzz/hex.cpp
...

💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744129399)
No I don't feel strongly about it, I just think this change is not nearly useful enough to warrant an extra 33LoC diff. Anyway, not worth arguing over, like I said it's not blocking.

👍 ryanofsky approved a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2280796157)
Code review ACK 192d71e312d0eab6cc1ce061edb34d9d338f7fec.

This seems like a reasonable approach, but I wonder if a more consistent way of doing this would just treat all the libraries the same as secp, installing them to the lib/ directory and listing them in libbitcoinkernel.pc. That way the secp library would not be an outlier, the TARGET_OBJECTS calls would not be needed, and build would be lighter, with each .o object file added to one library file .a instead of two.