💬 fanquake commented on pull request "depends: swap some cctools tools for LLVM tools":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2039370588)
> I did run into a confusing, but unrelated (happens on https://github.com/bitcoin/bitcoin/commit/28f2ca675f89a764e1ec8eb5671b35357b8677f3 too, see https://github.com/bitcoin/bitcoin/issues/29792), failure with bdb,

Yes that is unrelated.

> CI seems unhappy for macOS cross:

Fixed up.

💬 maflcko commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2039443040)
What is your `patch --version`?

💬 Sjors commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2039482595)
patch 2.0-12u11-Apple

💬 fanquake commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2039500096)
Can you produce a more minified set of steps/environment to reproduce. i.e does running the same set of build steps outside of depends also fail?

👍 BrandonOdiwuor approved a pull request: "test: Bump timeouts in feature_index_prune and wallet_importdescriptors"
(https://github.com/bitcoin/bitcoin/pull/29791#pullrequestreview-1982820317)
crACK 49c0b8b2288e60ae22fcac5d03811cf36ecec058

💬 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.