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

👍 stickies-v approved a pull request: " bench: Remove redundant logging benchmarks "
(https://github.com/bitcoin/bitcoin/pull/30790#pullrequestreview-2283574260)
ACK fadbcd51fc77a3f4e877851463f3c7425fb751d2

Logging conditionality is orthogonal to the performance effect of logging threadnames, so just keeping the unconditional one makes sense. Removing the now unused `LogPrintfCategory` is a nice cleanup.

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745868679)
> the "string constructor" tests to be re-added

That'd be my preference for now. Even if the tests are redundant, it seems cleaner to keep them for now to make review trivial (and maybe for symmetry with the uint256 tests).

There are many more obvious redundant and useless tests, like `BOOST_CHECK(1 == 0+1);` in this test case, so a lot more could be changed, but maybe removing tests can be done in a follow-up, or not at all.

But none of this is a blocker, so fully up to you. Feel free
...

👍 pablomartin4btc approved a pull request: "test: Add coverage for dumptxoutset failure robustness"
(https://github.com/bitcoin/bitcoin/pull/30817#pullrequestreview-2283624689)
cr & tACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

💬 ryanofsky commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745887633)
The check above reminds me of the part of the XZ backdoor where the attacker inserted a single . character to disable a linux security feature in a similar cmake check:

https://git.tukaani.org/?p=xz.git;a=blobdiff;f=CMakeLists.txt;h=d2b1af7ab0ab759b6805ced3dff2555e2a4b3f8e;hp=76700591059711e3a4da5b45cf58474dac4e12a7;hb=328c52da8a2bbb81307644efdb58db2c422d9ba7;hpb=eb8ad59e9bab32a8d655796afd39597ea6dcc64d

So I think it might be useful to add a sanity check to confirm that `cxx_supports_forti
...