💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825925932)
Maybe worth a separate PR to ensure the test coverage.

💬 maflcko commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2452025672)
This pull: https://github.com/bitcoin/bitcoin/actions/runs/11630801646/job/32390526596?pr=31187#step:6:337

```
C++ compiler flags .................... -Wno-error=unused-member-function -O2 -g -std=c++20 -fPIC -fdebug-prefix-map=/home/runner/work/bitcoin/bitcoin=. -fmacro-prefix-map=/home/runner/work/bitcoin/bitcoin=. -Werror -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-
...

💬 maflcko commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2452026337)
lgtm ACK 54d07dd37d5919c4e3bc535ae498d623282741d1

💬 maflcko commented on issue "intermittent issue in wallet_upgradewallet.py AssertionError: [node 2] Node returned unexpected exit code (1) vs (0) when stopping":
(https://github.com/bitcoin/bitcoin/issues/30798#issuecomment-2452045667)
https://cirrus-ci.com/task/6288622511456256?logs=ci#L3303

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825939865)
That's a good point. It may be the case that the selected peer has a parent -> child dependency but was not picked as one of the `fanout_with_ancestors` peers, the parent and the child may have ended up in different sets.

We should check the return of `TryRemovingFromSet` before adding it to `vInv`

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825957496)
These collisions of this kind *could* happen in theory, but I don't think they can be really gamed:

short ids are peer specific, they are generated using `SipHashUint256` which are salted using ` m_k0` and `m_k1`. Therefore, while a collision of this kind is theoretically possible, an adversarial peer should not be able to guess collisions and hence exploit them

💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1825957996)
Re https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824164226: I think I agree with marco that a compile time check would be nicer and we should ideally upstream it so that our `FuzzedDataProvider` impl stays in sync.

👍 dergoegge approved a pull request: "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction"
(https://github.com/bitcoin/bitcoin/pull/31203#pullrequestreview-2410304438)
utACK 5a26cf7773efdc316ef8235c541ad39e230cd10a

💬 sr-gi commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2452082143)
Removed dummy commit

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1825965609)
Yes, that seems better. Making ProcessPCP a void is also fine with me.

💬 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