💬 luke-jr commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1843276390)
Hypothetical future issues is not an excuse to stagnate harmfully. You're making the perfect the enemy of the good.

💬 achow101 commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1417656406)
This log line is on by default.

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1417662670)
Again, not convinced having this log on all the time is worth it, and don't see how this log is relevant to this PR.

💬 ismaelsadeeq commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1843292726)
Concept ACK
Did a light review this looks good to me

💬 maflcko commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1843313196)
Building 17.0.5 failed for me on riscv64: `/var/log/guix/drvs/9k/xxqy3shpf5b3ajmw1ar2zrbm2a8pyp-llvm-17.0.5.drv.gz`:

```
...
[ 93%] Linking CXX executable ../../bin/dsymutil
cd /tmp/guix-build-llvm-17.0.5.drv-0/source/build/tools/dsymutil && /gnu/store/v9d5kgjpkcgfwc5k1jsq8a6i2qxmp9fw-cmake-minimal-3.24.2/bin/cmake -E cmake_link_script CMakeFiles/dsymutil.dir/link.txt --verbose=1
/gnu/store/dcrdgy1rjkh0kibpkw6v2mia7f921b27-gcc-11.3.0/bin/c++ -fPIC -fno-semantic-interposition -fvisibility
...

🚀 fanquake merged a pull request: "fuzz: Avoid timeout in bitdeque"
(https://github.com/bitcoin/bitcoin/pull/29012)

💬 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