💬 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

💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452535019)
> @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`.

Magical. Thank you 🙏

💬 TheCharlatan commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2452537469)
> But, I don't see #include <concepts>.

If you check the IWYU logs in the "tidy" CI job you will see that it is indeed missing:
```
[19:28:01.280] /ci_container_base/src/policy/feerate.h should add these lines:
[19:28:01.280] #include <concepts> // for integral
```
This report is generated on each CI run, but the missing includes are not enforced. This can be confusing, so best to always check the CI output.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1826266792)
Not too sure about the `return removed == 1`

The assume is there to make sure that if the code happens to be wrong, it is catched on testing. In the current approach, if the code moving data from `delayed_set` to `m_local_set` was broken somehow, this method will return that data was actually removed. If were to return `removed == 1` in the aforementioned case, it would return false, and the caller's logic may break.

I don't think any of these cases is likely to happen given the assume wil
...