💬 ryanofsky commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2452089137)
> I don't think it is a supported use-case to take libraries from one build (with different build options) and drop them into another build

Agree it should not be a generally supported use case. And I do think it's nice to have a BUILD_FOR_FUZZING option that enables the best options for builds specifically doing fuzzing.

But for normal builds, I don't think the whole codebase should need to be recompiled with different options just to get a useful fuzz binary. And I don't think it is good
...

🚀 fanquake merged a pull request: "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction"
(https://github.com/bitcoin/bitcoin/pull/31203)

👍 ralyodio approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2410378334)
LGtm

💬 nguyensydo commented on something "":
(https://github.com/bitcoin/bitcoin/commit/e001dc3dc6e0c260519f5b538e262d9d771bca8a#commitcomment-148634289)
a

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1826029501)
Looking into whether I can avoid exposing this function even before cluster mempool lands; will consider it for a followup PR.

✅ fanquake closed an issue: "`Wunused-member-function` in test each commit"
(https://github.com/bitcoin/bitcoin/issues/31180)

🚀 fanquake merged a pull request: "ci: Do not error on unused-member-function in test each commit"
(https://github.com/bitcoin/bitcoin/pull/31187)

💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2452206987)
> And I don't think it is good for BUILD_FOR_FUZZING option to create strange libraries that seem functional but skip proof of work checks.

I don't think this conceptual issue is addressed by your alternative `SLOWCHECK` suggestion. If this was a supported use-case, someone could link a fuzzing-compiled `SLOWCHECK` library that seems functional, but is possibly slow, or may even crash in production, when normally it should not. So your use-case would also be a reason against `SLOWCHECK`. I do
...

💬 0xB10C commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2452328622)
> I was just trying to point out that there might be a bug somewhere - I don't know if so and where exactly and haven't analyzed this in detail but it seemed to me that pre-segwit txns are often considered unrequested, so these might be false positives.

@mzumsande and I had a look at my logs and the ["rate of a few [unsolicited] per minute"](https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2401651398) I was seeing were mostly non-segwit transactions - not unsolicited transactions. T
...

👍 rkrux approved a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2410623270)
reACK d7fd766feb2f579bdba0e778bacdeb13103e8282

🤔 mzumsande reviewed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2410592950)
Concept ACK

I wouldn't oppose deleting some benchmarks, but I also like the idea of using priorities. That, combined with the doc should be sufficient to deter micro-optimiziations in non-critical code.

💬 mzumsande commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1826118034)
typo: perforamnce

💬 mzumsande commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1826129957)
Not sure I follow this point: Under "denial of service" I would understand attacks that overwhelm the target with repeated requests (which are often but not necessarily identical) and as result may just slow it down / make it unresponsive instead of outright crashing it - while I agree that benchmarks don't really find these, I wouldn't see fuzz tests designed to find crashes as the best way to test these issues (instead of integration tests with some sort of stress)?

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826245538)
Resolving since we don't have a cache anymore

💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452512182)
So C++ noob question. Not sure if this is an appropriate place to ask.

I'm noticing that you're calling `CFeeRate` with an `int`:
https://github.com/bitcoin/bitcoin/blob/280e65d3a7993ea38ba49ae00133a7ba709c193e/src/wallet/test/coinselection_tests.cpp#L20-L38

Okay that's cool, clearly it compiles and runs...but how?

src/policy/feerate.h has the `class` definition. I don't see how it's able to only take an `int`

This is my best bet of how it works

https://github.com/bitcoin/bitcoi
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826249448)
Agreed the docs for `TxReconciliationTracker::AddToSet` could be improved. I'll update it with a version of your suggestion

💬 sipa commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452516136)
@jasonribble `template<std::integral I>` is the same as `template<typename I>`, with the added restriction that `I` must be a type that satisfies the `std::integral` concept. Since `int` does satisfy that concept, `I` can be instantiated as `int`.

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1826251662)
Done thanks

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1826251925)
Good idea, done

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-2452518924)
> > Does bitcoind consider this to be a breaking change? The new error message may be "more accurate", but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.

I've added a release notes for this