💬 sr-gi commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1417705899)
I think this is related to this? https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1329013797

💬 instagibbs commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1417711662)
ok so we agree, which makes the test a bit odd imo. The MEMPOOL message being sent or not has no relation on whether we respond to that particular `getdata` from the peer.

💬 sr-gi commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1417711877)
So I don't think this is inherently wrong. The irrelevant transaction is requestable (independently of the mempool message) because it is in the mempool and would have been announced to the peer if it happened to be interested in it (which is not the case, based on the filter).

Sending the mempool message and checking is done to make sure only the relevant transaction is annonced. However, we are still able to request the irrelevant.

💬 instagibbs commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1417717020)
```suggestion
self.log.info("We should get it anyway because it was in the mempool on connection to peer")
```

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1843362896)
> Building 17.0.5 failed for me on riscv64:

Nice. Glad we've now got Guix on RISC-V. I guess this is either a bug in LLVM (i.e a missing an `-latomic`), or in the combination of GCC 11 & LLVM 17. Will take a look.

💬 maflcko commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1843367906)
I'll also try to reproduce outside of my riscv64 metal, because it is currently a bit slow and faster CPUs will only hit the market next year (or so).

💬 luke-jr commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1417764868)
nit: blank line under this; ideally docs above

💬 maflcko commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1843435820)
Jup, same error with cross-compilation:

```
$ guix time-machine --commit=3778f778c37110460dd78088200cbd05eb0c49e4 -- build --target=riscv64-linux-gnu llvm
...
[ 89%] Linking CXX executable ../../bin/dsymutil
cd /tmp/guix-build-llvm-17.0.5.drv-0/source/build/tools/dsymutil && /gnu/store/ag7sflxmy9fpqxdrgwk6rlynvdxy5695-cmake-minimal-cross-3.24.2/bin/cmake -E cmake_link_script CMakeFiles/dsymutil.dir/link.txt --verbose=1
[ 89%] Building CXX object tools/llvm-isel-fuzzer/CMakeFiles/llvm-is
...

💬 instagibbs commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#issuecomment-1843441930)
ACK https://github.com/bitcoin/bitcoin/pull/28485/commits/97c0dfa8942c7fd62c920deee03b4d0c59aba641

tests line up with my understanding of the code

💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1843470806)
> Hmm, playing around with this code on godbolt, it hardly ever compiles down to bswaps. I'm not sure what changed from when I was initially investigating. Will have a look.

Ok, interesting, the problem is pretty easy to see here: https://godbolt.org/z/d5EGs8Ybs

GCC and MSVC both do the same thing: the optimization is done on the small static function, but it doesn't carry through when inlined. clang's output looks as expected. That explains why my test cases look good, but real world perf
...

💬 Sjors commented on pull request "test: Fix test by checking the actual exception instance":
(https://github.com/bitcoin/bitcoin/pull/28989#issuecomment-1843472589)
Looks like a reasonable fix, thanks!

💬 brunoerg commented on pull request "fuzz: p2p: Detect peer deadlocks":
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1843473953)
Concept ACK

💬 maflcko commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1843481759)
> clang can obviously do the right thing.

Are you sure? When I tried yesterday, it did not (https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445629)

🤔 ryanofsky reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1767990381)
Updated 8a02ce59ffa16c73611aeda6caf8b54d85c6208f -> 4a3a2651420ca1808cb25aed8a33d57dfcd627f5 ([`pr/noshut.19`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.19) -> [`pr/noshut.20`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.20), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noshut.19..pr/noshut.20)) with Qt windows bugfix

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1417585924)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1417160346

> Calling `app.node()` here before `app.createNode` results in a crash on windows when running `bitcoin-qt.exe`.

Great catch! This should be fixed now by passing a shutdown callback and delaying the call to app.node(). I didn't want to move the createNode call, because the interfaces::Node object is intentionally created after the splash screen, so the splash screen can show up without a delay. The delay wouldn't be s
...

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1417833003)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1417160672

> Nit: Could this be private?

Sure, switched to a private member in the latest push

💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1843504462)
> > clang can obviously do the right thing.
>
> Are you sure?

Absolutely not! I'm making this up as I go.

> When I tried yesterday, it did not ([#28674 (comment)](https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445629))

Thanks, I guess that's the test I just asked for.

Ok, I'll keep poking. But it seems like this approach likely won't work :(

💬 maflcko commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1843519987)
> Ok, I'll keep poking. But it seems like this approach likely won't work :(

What about hiding the function inside a compilation unit, where they are turned into `bswap`, and then rely on LTO to replace the `call internal_bswap_64` with `bswap`? (Haven't tried this)

💬 jonatack commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1843570199)
Post-merge ACK

💬 hebasto commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1843624931)
Rebased on top of the merged #28989.