💬 maflcko commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331996771)
review ACK a7a4e11db8722c85175bcc4d9f73a713e9e61513

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2332011275)
Rebased

👍 ryanofsky approved a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2283417302)
Code review ACK 766881e33d13ca2887f7ed179cdcc6bc007a6325. I like this approach, seems more generic and lightweight.

💬 ryanofsky commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1745761110)
In commit "build: Produce a usable static kernel library" (7739fa69462f070c62345f2f14fcf817a512d59a)

Seems ilke in theory this could install the same libraries more than once because it is just recursively adding dependencies without deduplicating them. For example if kernel library depends on util and crypto, and util library depends on crypto, crypto library could be listed twice. May be worth iterating over a unique version of the list so the install step is cleaner if it does contain dupl
...

📝 fanquake opened a pull request: "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type"
(https://github.com/bitcoin/bitcoin/pull/30824)
`FORTIFY_SOURCE` should be used if `ENABLE_HARDENING=ON` and optimisations are being used. This should not be coupled to any particular build type, because even if the build type is `Debug`, optimisations might still be in use.

Fixes: #30800.
Also somewhat of a followup to https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742257436.

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#issuecomment-2332049650)
Not sure about the inline source formatting. cc @hebasto.

💬 fanquake commented on issue "cmake: incorrect assumption that `Debug` build type will not use optimisations":
(https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2332050710)
> Not sure if this makes sense, but if the problem is that specifying _FORTIFY_SOURCE without optimizations triggers a compiler warning, maybe there should just be a check that drops _FORTIFY_SOURCE when it triggers the warning. That might avoid problems if there is a discrepancy between the way our cmake code detects optimizations and the way _FORTIFY_SOURCE implementation does.

That could be a good approach, but there's also the possiblity that FORTIFY will warn if the selected level will b
...

💬 TheCharlatan commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1745778510)
I was doing this, but removed it again from the patch. I feel like if there are duplicates, it is a bug in our library topology. But it could still make sense to guard against them.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332066227)
With the new changes, I'm considering disallowing entry into the mempool if *either* the base fee or the modified fee is non-0. If miners really want to mine things generally with 0 fee that's one thing, but offering an interface to avoid the dust cleanup seems suboptimal.

@glozow

> Can you remind me of the use cases of a keyed dust output again?

What I'm calling "keyed" anchors would be used anytime you don't want a third party to be able to run off with the utxo. As a motivating exam
...

💬 hebasto commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745781839)
Could you provide a few links that confirm this code checks the source fortification feature?

💬 ryanofsky commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1745782482)
Yeah I would leave alone for now to keep things simple. I don't think it is a bug to have multiple libraries a target depends on depend on some of the same libraries themselves though.

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745785332)
This just checks that optimisations are in use, which is when we want to use it, using the same approach as glibc: https://github.com/bminor/glibc/blob/4945ffc88a8ad49280bae64165683ddfd12b2390/include/features.h#L421.

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745788614)
i.e The new code should be performing the same tests as master, except that, rather than outsourcing the check for optimisations to a test of whether or not the build mode is `Debug`, it tests if optimisations are being used directly.

👍 hebasto approved a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2283464346)
ACK 3ae35b427fe59bc9ab24d07c1adb46faa702de20, I have reviewed the code and it looks OK. Also I've tested it locally.

👍 TheCharlatan approved a pull request: "interpreter: use int32_t instead of int type for risczero compile"
(https://github.com/bitcoin/bitcoin/pull/30794#pullrequestreview-2283493435)
ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2332108655)
Rebased to make CI green. This was based on a pre-CMake commit

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745827757)
I think one of them may have been my initial version, but I changed it because I didn't like the `else` and assigning the bool from itself (inverted).

So I'll leave this as-is for now.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745847260)
> build failure

Hmm. Pushed something to work around the msvc build failure.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745847386)
Actually, done here, because it is just one line of code :sweat_smile:

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745847825)
For symmetry, but removed in the last push.