🚀 fanquake merged a pull request: "ci: use Clang 16 for Valgrind"
(https://github.com/bitcoin/bitcoin/pull/29848)

💬 laanwj commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2056868984)
Concept ACK, issues with misspelled or otherwise missing defines can be really sneaky (and have caused some hard to find bugs in the past), this ia good check to have.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2056882691)
ready for review

💬 theStack commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565820341)
Good idea, done.

💬 theStack commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565820757)
> Nice catch! I wonder if a more wholistic way to avoid unintentionally creating tx packages is to gather all the (confirmed only) UTXOs at the beginning? e.g. [this](https://github.com/glozow/bitcoin/commits/2024-04-fill-mempool-29827/), but feel free to ignore

That seems like a good approach to avoid similar problems popping up in the future 👌 I've cherry-picked your commit (with small conflict resolutions to have it introduced before the new test commit).

Btw, other approaches I've
...

💬 theStack commented on pull request "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)":
(https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565822381)
Thanks, done.

💬 maflcko commented on pull request "build: Fix false positive `CHECK_ATOMIC` test":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2056902062)
> On the master branch @ [0de63b8](https://github.com/bitcoin/bitcoin/commit/0de63b8b46eff5cda85b4950062703324ba65a80), a building `bitcoind` with clang-15 for `i686-pc-linux-gnu` fails to link:

Can you add exact steps to reproduce, ideally starting from a fresh install of the operating system?

💬 fjahr commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056917422)
Concept NACK

As I have said before, I think there needs to be a discussion about using more Rust just for the sake of using more Rust in the project. The only motivation given so far is that @maflcko said that it could be done. There is no benefit to doing this otherwise apparently and the biggest effect I see is that there will be fewer people able to contribute to that part of the code base. And this change alone will also take up a lot of review effort so it appears to be a clear negative,
...

💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2056921899)
> Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?

without focusing too much on the LN use-case, anytime you wanted to use something larger, you'd have to make sure you do so, f.e. when sweeping a max size commitment tx with penalty path. Basically you'll have to check if your tx is larger than 25k and switch if so.

💬 fanquake commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2056925244)
> Want to upstream the crc32 patch to match the others we have sitting there?

Sure. Opened a PR in our crc32c subtree fork: https://github.com/bitcoin-core/crc32c-subtree/pull/7, and one in Google upstream: https://github.com/google/crc32c/pull/64.

🤔 glozow reviewed a pull request: "test: check disconnection when sending sendaddrv2 after verack"
(https://github.com/bitcoin/bitcoin/pull/29699#pullrequestreview-2001213888)
LGTM

💬 glozow commented on pull request "test: check disconnection when sending sendaddrv2 after verack":
(https://github.com/bitcoin/bitcoin/pull/29699#discussion_r1565855209)
(I think this is a good example of why we shouldn't use `assert_debug_log` when we're able to check the logic otherwise)

🚀 glozow merged a pull request: "test: check disconnection when sending sendaddrv2 after verack"
(https://github.com/bitcoin/bitcoin/pull/29699)

🚀 glozow merged a pull request: "rpc, bugfix: Enforce maximum value for setmocktime"
(https://github.com/bitcoin/bitcoin/pull/29869)

💬 maflcko commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056954530)
> As I have said before, I think there needs to be a discussion about using more Rust just for the sake of using more Rust in the project. The only motivation given so far is that @maflcko said that it could be done.

Not sure where you get that from, but I never said that using more Rust for the sake of using more Rust can be done. I said something else:

* `I *don't* think moving all Python code to Rust makes sense.` https://www.github.com/bitcoin/bitcoin/pull/29744#issuecomment-2022526794
...

💬 fjahr commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056968267)
> Not sure where you get that from

"an alternative might be to rewrite the 3 lint checks to rust" https://github.com/bitcoin/bitcoin/pull/29479#pullrequestreview-1957109397 That is what @davidgumberg has given as the motivation in his description. It does not matter what you have said in other places because I have not been talking about that. I only talked about your comment that the author took as the motivation for this PR.

💬 theStack commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2056969396)
> > remove unused cpp-subprocess options
>
> Are all remaining options used?

From the 12 available cpp-subprocess options we currently only use 4 (namely `executable`, `input`, `output` and `error`):
https://github.com/bitcoin/bitcoin/blob/22c86140f8fe4f13b7b5415ff62922e497fd4948/src/common/run_command.cpp#L29
I.e. the other 8 are unused. This PR currently removes 3 of the 8 unused ones -- the remaining 5 (`cwd`, `bufsize`, `environment`, `close_fds`, `session_leader`) seemed to be commo
...

💬 naiyoma commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1565878704)
For this function, I wasn't sure which rounding type to use. That's why I opted to keep the implementation and just change to Decimal

🤔 pablomartin4btc reviewed a pull request: "Bugfix: GUI: Help messages already have a trailing newline, so don't add an extra one"
(https://github.com/bitcoin/bitcoin/pull/29658#pullrequestreview-2001282587)
I think this should be moved to the QT gui [repo](https://github.com/bitcoin-core/gui).

👍 pablomartin4btc approved a pull request: "contrib: list other binaries in manpage output"
(https://github.com/bitcoin/bitcoin/pull/29585#pullrequestreview-2001289630)
utACK 7c3ac598dd9a1f1a506c4931249ff6c9f1c949ba