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

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2332233857)
@sdaftuar Added your benchmarks as a commit to this PR (I've reworked it to auto-detect the transaction count).

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745897934)
Taken, thanks.

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745898836)
I am certainly open to additional approaches in regards to the (potential fragility) of online source.

> So I think it might be useful to add a sanity check to confirm that cxx_supports_fortify_source is actually true in release builds:

I have a PR to try and do this in #27038.

👍 ryanofsky approved a pull request: "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type"
(https://github.com/bitcoin/bitcoin/pull/30824#pullrequestreview-2283657342)
Code review ACK 30803a35d54acda19ded88474c205f8954fea5e1