💬 achow101 commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1843222722)
ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1417608844)
Feels like we are talking past each other a bit here. My point is that for developers, we already have tracepoints as a tool for debugging and observability into low-level details of how the software is working. In many cases, a developer will try to reproduce an issue reported by a user on their own node, at which point debuggers and tracepoints are going to be more useful than logs.

For node runners, if someone provided us with this log I don't see how having this would have helped any of u
...

💬 achow101 commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1417614132)
The logs are useful for figuring out what happened when a user asks about why their wallet chose a crazy number of UTXOs when one would have been sufficient. As we add more algos, being able to see in the logs what algo was used, and what the resulting waste score was, is helpful for investigating such questions.

🚀 achow101 merged a pull request: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581)

👍 pablomartin4btc approved a pull request: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581#pullrequestreview-1768047102)
ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3

👍 brunoerg approved a pull request: "fuzz: Avoid timeout in bitdeque"
(https://github.com/bitcoin/bitcoin/pull/29012#pullrequestreview-1768071879)
crACK fad1903b8a85506378101c1f857ba47b4a058fb4

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1417638654)
> wallet chose a crazy number of UTXOs when one would have been sufficient

I don't see how this example is relevant to this PR, which is why I think the logging discussion would be better moved to a logging PR.

This also presumes the user already had the logging enabled before the issue happened. I'm not sure the costs outweigh the benefits of having this log always on to be able to answer questions like your example.

💬 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