💬 Sjors commented on pull request "depends: swap some cctools tools for LLVM tools":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2039574673)
Also tested the guix build (made on AMD Ubuntu) for 77eaee0ba46a68268be352e7d9c39488f9339099 on macOS 14.4.1.

```
4fd8566ccc8229c4784eac5cc21d077b179b4165b457911fe142ea2e3b0069e7 guix-build-77eaee0ba46a/output/arm64-apple-darwin/SHA256SUMS.part
6cca0ae159eddbf88dd1858eec51625816346487fc5eb9e5260037c686e8e312 guix-build-77eaee0ba46a/output/arm64-apple-darwin/bitcoin-77eaee0ba46a-arm64-apple-darwin-unsigned.tar.gz
c967afbfd76efb0a16e54524734396e56afe249273620f59904253a8608d1416 guix-build
...

💬 naiyoma commented on pull request "net: update comment for service bit support info for seed.bitcoin.sipa.be":
(https://github.com/bitcoin/bitcoin/pull/29809#issuecomment-2039613643)
@stratospher thanks for the review . I think it would be more ideal to simply reference the seeder repo. However, for the sake of uniformity, I believe it's better to continue listing them as is currently being done.

> Concept ACK. thanks for opening this! it's easy to forget to update support for service bit filtering here and the list just keeps getting longer.
>
> another possibility could be just mentioning that the seeder supports service bit filtering and linking to the seeder repo t
...

💬 Sjors commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2039631300)
On todays master 5a5ab1d5446693ee2655860c2f2920bcc3389c83:

```sh
git clean -dfx
cd depends
make NO_BOOST=1 NO_LIBEVENT=1 NO_QT=1 NO_SQLITE=1 NO_UPNP=1 NO_NATPMP=1 NO_USDT=1
```

Immediately fails:
```
Fetching db-4.8.30.NC.tar.gz from https://download.oracle.com/berkeley-db
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 21.7M 100 21.7M 0 0 19.2M 0 0:00:0
...

💬 maflcko commented on pull request "[WIP] ci: test secp256k1 MSAN asm annotations":
(https://github.com/bitcoin/bitcoin/pull/29742#discussion_r1553508634)
```suggestion
export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,memory --disable-hardening CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE'
```

nit: While touching, can remove the duplicate CFLAGS and CXXFLAGS. Same below.

💬 maflcko commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2039637101)
So can it be closed, or is there something that points to a problem in this repo that can be fixed here?

💬 fanquake commented on pull request "depends: swap some cctools tools for LLVM tools":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2039696450)
Guix Build (aarch64):
```bash
c39f93edbd44b97256834f1173f23c311a7f7fdf467ecebd4ec930e0691732ad guix-build-77eaee0ba46a/output/aarch64-linux-gnu/SHA256SUMS.part
4e88994692636055e105346229ad87fcb40569a984fddb278e92b78498a983ef guix-build-77eaee0ba46a/output/aarch64-linux-gnu/bitcoin-77eaee0ba46a-aarch64-linux-gnu-debug.tar.gz
f2d7c80d039ecfe9889eada5be4f701f413b22eb679511634a20545528b61a15 guix-build-77eaee0ba46a/output/aarch64-linux-gnu/bitcoin-77eaee0ba46a-aarch64-linux-gnu.tar.gz
df23e2
...

🤔 glozow reviewed a pull request: "clang-tidy: Enable misc-no-recursion"
(https://github.com/bitcoin/bitcoin/pull/29690#pullrequestreview-1983063955)
Concept ACK, as I think we agree that we want to avoid *unintentional* recursion. I think it's useful to have all of our recursive functions enumerated.

💬 glozow commented on pull request "clang-tidy: Enable misc-no-recursion":
(https://github.com/bitcoin/bitcoin/pull/29690#discussion_r1553559809)
Not sure if the point is to discourage recursion, or to tell developers that it has to be marked explicitly? I think we can consider it implied that people should use recursion only when appropriate, just like any other practices that can lead to bugs. And disagreements about whether or not [fill in the blank] is appropriate can be resolved in review on a case-by-case basis.
```suggestion
- Recursion is checked by clang-tidy and thus must be made explicit. Use
```

💬 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#discussion_r1553606825)
In 1ffdf3f0ad10879cc4aab6a513af8d1c20c63151: Why? What is the point to put a single function consisting of a single `if` in a new file and module? I fail to see how more boilerplate than the actual logic is beneficial.

If your point is that large files shouldn't exist, then I tend to disagree. For example, `doc/developer-notes.md` is large and exists. Same for `./src/rpc/blockchain.cpp`.

My preference would be to not change code, unless there is a reason.

💬 Sjors commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2039881846)
I'm still running the test suite to see if it's not broken. The machine is quite slow.

At minimum we should add gnu patch to the list of requirements / recommendations, at least for macOS < 14.

🚀 fanquake merged a pull request: "depends: build libqrencode with CMake"
(https://github.com/bitcoin/bitcoin/pull/29725)

💬 m3dwards commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2039911554)
I can partially replicate this regression using pre-built binaries on Windows 11. For me, `27.0rc1` is **15% slower** than `26.0`. I can't quite replicate the higher variability of `27.0rc1` as it had a slightly higher range but a lower standard deviation.

**26.0**

Mean: 35.21 seconds
Range: 9 seconds
Standard Deviation: 2.25


**27.0rc1**

Mean: 40.77 second
...

👍 ryanofsky approved a pull request: "Logging cleanup"
(https://github.com/bitcoin/bitcoin/pull/29798#pullrequestreview-1983292855)
Code review ACK d98ca056a82264884bc1aa2da3af540a73ee8f26

Nice cleanups! Main feedback I have is to more clearly indicate which commits are refactoring, and which have behavior changes. Would suggest putting "logging, refactor:" in refactoring commit titles and "logging:" in commits that change behavior. Also I would consider dropping any behavior changes that aren't obvious improvments and doing them in separate PRs so they have more visibility and are not lost in "logging cleanups."

💬 ryanofsky commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1553726631)
In commit "rpc: remove misleading text from logging help" (e78866a10973c64a59e8a71eed737328b6bc0ca3)

Right now this returns NONE/true when the empty string "" is passed. Should return false in that case too and never return NONE?

Also, this is a nice change, but I found description confusing because the whole thing sounds it this like a help text change, but then the last sentence mentions it also adds a new error. Would suggest a description more like:

```
rpc: make logging method rej
...

💬 ryanofsky commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1553719054)
In commit "logging: minor encapsulation improvement and use BCLog::NONE instead of 0" (1fae74149507e5914a7bb7986e4906053b8dbbbb)

Would be good to add "refactor:" prefix to commit message so it is obvious this is not supposed to change behavior.

💬 ryanofsky commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1553715178)
In commit "logging: move special cases to GetLogCategory()" (9fb321ad2b0176955718317b796715250eac01f4)

Would be good if commit title had a "refactor:" prefix so it is clear this is not intended to change behavior.

Also, I think this commit should remove ALL and NONE cases from the map, so special cases are handled completely within the GetLogCategory() and LogCategoryToStr() functions, and there are no more special cases in the map and LogCategoriesList(). Would suggest:

```diff
--- a/
...

💬 ryanofsky commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1553729155)
re: https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1549650210
In commit "logging: remove unused code" (d98ca056a82264884bc1aa2da3af540a73ee8f26)

> Would you prefer an assert

I'd suggest just dropping this commit entirely. If you adopt my suggestion from the first commit the `{"", BCLog::NONE}` map entry already disappears so this does not provide an additional simplification to the map, and I don't think there's really another reason to change GetLogPrefix behavior here, at le
...

💬 dergoegge commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2039930366)
> One example on line 282 of transaction_tests.cpp:

I believe this is unrelated to this PR, see https://github.com/bitcoin/bitcoin/issues/29051. `make clean` and rebuilding should fix it.

👋 fanquake's pull request is ready for review: "depends: build miniupnpc with CMake"
(https://github.com/bitcoin/bitcoin/pull/29707)

💬 glozow commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1553735891)
"or similarly small" doesn't really make sense to me. AFAICT this makes just enough transactions to go past the 5MB mempool.