💬 fanquake commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329365908)
I think this is a duplicate of #29796?

💬 ryanofsky commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329370412)
Created a separate issue for this because I have some thoughts about it and don't want to hijack the discussion in #30800 which is more about _FORTIFY_SOURCE and detecting which optimization flags are set (as opposed to setting them). Working on an idea which I'll post below.

💬 achow101 commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-2329379225)
ACK a3108a7c5692d137b70b8442b4741936277e89be

📝 TheCharlatan opened a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814)
Since the move to cmake, the kernel static library that is installed after a cmake --install build is unusable. It lacks symbols for the internal libraries, besides those defined in the kernel library target.

Fix this by explicitly adding the required object files of its dependencies to the library instead of relying on additional linker
steps.

A generator expression guard is added to ensure that libraries that are not compatible with the target architecture are not compiled. It is not cl
...

💬 maflcko commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2329386280)
re-ACK 5b59cfc944f9eac73e1e69bcc660280b6809724a 🌞

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 5b59cfc944f9eac73e1e
...

💬 fanquake commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329403447)
> I saw https://github.com/bitcoin/bitcoin/pull/29796 too, but it seems to be a discussion of which optimization flags are set, not how the flags are set.

Sure. If the discussion is in general, about how options should be passed from depends to CMake, then I think (ideally) the answer is just, a toolchain file, and our CMake build should take (pretty much verbatim) what is set in that file (and not try and detect if it's a "depends" build, and behave differently if that is the case). That see
...

🚀 achow101 merged a pull request: "rpc: dumptxoutset height parameter follow-ups (29553)"
(https://github.com/bitcoin/bitcoin/pull/30808)

💬 achow101 commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2329407048)
ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358

💬 ryanofsky commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329412411)
I think the way things would work ideally is that depends system would set default build flags, and main build system would pick them up reliably, but the defaults could be freely overridden and edited by users running `ccmake` or passing `-D` values to `cmake`. Specifically this would mean:

1. Depends settings in the toolchain file would reliably take precedence over cmake defaults and over defaults normally set by the main build system.
2. User-defined `-D` and `ccmake` settings would reli
...

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