💬 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.

💬 ryanofsky commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228)
Not important, but it would probably make sense for both `disable_network` and `temporary_rollback` variables to be `std::optional` instead of `std::unique_ptr` since they aren't passed around it would simplify the way they are initialized.

🚀 achow101 merged a pull request: "net: Favor peers from addrman over fetching seednodes"
(https://github.com/bitcoin/bitcoin/pull/29605)

💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744155996)
> Seems overkill to import a dependency to check whether a dot is present in a string view or not

Maybe, in that case we can use glob patterns as well with `ls-files`:

```Rust
fn lint_doc_release_note_snippets() -> LintResult {
let result = check_output(git().args(["ls-files", "--", "doc/release-notes/release-notes-*.md", ":!doc/release-notes/*.*.md"]))?;
let snippet_notes = result.lines().collect::<Vec<_>>();
match snippet_notes.as_slice() {
[] => Ok(()),

...