💬 davidgumberg commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2080239525)
I think I will leave it in here since the motivation for https://github.com/bitcoin/bitcoin/commit/54b2115bb848e1aad8a86aae858de63a851235ce is mostly the increased number of lint checks only accessible through the lint test runner. But I will split it out if anyone has an objection, or if this PR stalls.

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2080372645)
I replaced the `requests` with `urllib`, tested the script for downloading .tar.gz file and works fine for me.

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#issuecomment-2080373067)
Hello @fjahr , @alfonsoromanz , @kevkevinpal ,
Could you please confirm if it is ready for merging, or does it require further reviews?

💬 Karimazam commented on issue "Prune mode is incompatible with -txindex.":
(https://github.com/bitcoin/bitcoin/issues/28684#issuecomment-2080384383)
i installed ltc core wallet when i run it for the very fast time it shows same issue litecoin core wallet prune mode is incompatible with blockfilterindex does anybody know why i am facing this f**king issue/?

👍 fanquake approved a pull request: "build, msvc: Drop duplicated `common\url.cpp` source file"
(https://github.com/bitcoin/bitcoin/pull/29976#pullrequestreview-2026447201)
ACK 97a4ad5713853f51c729cced73f133fafa735ba2

🚀 fanquake merged a pull request: "build, msvc: Drop duplicated `common\url.cpp` source file"
(https://github.com/bitcoin/bitcoin/pull/29976)

📝 fanquake converted_to_draft a pull request: "depends: swap some cctools tools for LLVM tools"
(https://github.com/bitcoin/bitcoin/pull/29739)
These tools are used in GUI packaging on macOS, and also somewhat of a blocker for #21778. The main issue is that some distros don't really ship these tools in a standard ways, i.e Ubuntu only ships `llvm-otool` with a version suffix, i.e `llvm-otool-17`, which makes it hard to find and use. Rather than trying to deal with that mess, switch to using the equivalent LLVM tools.

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581749184)
Is it required to check for 404 in a separate, additional request?

💬 laanwj commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581750310)
That's how the current code works too - it first does a HEAD request to check that the file is there, then goes to downloading. i don't know the original reasoning behind this, though.

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581750345)
No, I could write more pythonically and with one request, but the reason I wrote it this way was to commit to the previous code. If you agree with one request, I can change it.
Should this change be in a new commit or should I force it on the previous commit?

💬 maflcko commented on issue "Require thread_local":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2080410522)
> We only use `thread_local` for storing the thread name in `std::string`. Can use dumb `char` array for this:

If this works, then string_view may also work? Yet another alternative may be to just completely nuke thread_local with a map from id to $obj (See https://github.com/bitcoin/bitcoin/issues/18678#issuecomment-621717592)

💬 laanwj commented on issue "Rethink thread_local (take 2)":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2080412606)
i think the main problem with the map approach is that it doesn't clean up data when threads disappear, this is something that TLS handles automatically, and also why it's so hard for platforms to get right

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581758233)
Same question as https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566378431

I like that this doesn't add extra burden to `m_recent_rejects` (which is probably the busier of the 2 filters even though they have the same size). It makes more sense if you think of each tx within the package as reconsiderable, even though the package isn't?

💬 laanwj commented on issue "qa: Support git worktrees when running the linters locally via Docker":
(https://github.com/bitcoin/bitcoin/issues/29972#issuecomment-2080415116)
i think the root problem here is that docker only provides access to the source directory to the container, and a worktree isn't self-contained, it contains a path to the actual repository the worktree comes from, which won't be available there. The linters need access to git metadata to do their checks (for example, to make sure that they only check files in git, not generated ones).

💬 maflcko commented on issue "qa: Support git worktrees when running the linters locally via Docker":
(https://github.com/bitcoin/bitcoin/issues/29972#issuecomment-2080420581)
> Yes, this happens if you try to run any of the CI tasks locally in a worktree.

At least for the "real" CI tasks, we should consider making a fallback, as I don't think much from git is needed? So far I could only find `git log -1` in `ci/test/`, but that should work in a worktree, no?

⚠️ laanwj opened an issue: "depends: Freetype and xcbproto version in depends are too new"
(https://github.com/bitcoin/bitcoin/issues/29977)
While investigating interface versions for [qtsowrap](https://github.com/laanwj/qtsowrap/blob/main/scripts/collector.py), i noticed that currently, in depends, the version of freetype and xproto are too new. As of 8cd9475321737a69454f3b54a588b7bfe9f32847.

At least if we're considering Ubuntu 20.04LTS as the reference. xcb-proto is even too new for 22.04LTS:

| library | 20.04LTS | 22.04LTS | depends |
|-------|--------|---------|---------|
| libfontconfig | 2.13.1 | 2.13.1 |
...

💬 laanwj commented on issue "qa: Support git worktrees when running the linters locally via Docker":
(https://github.com/bitcoin/bitcoin/issues/29972#issuecomment-2080422077)
> but that should work in a worktree, no?

All git commands work in a worktree, but not when it is seperated from its main repository by containers.

💬 laanwj commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2080456729)
> Rebased to re-run CI.

Better to ask someone in the IRC channel to re-run the CI, as that won't invalidate ACKs 😄

re-ACK 469e4834bad295f6d9fbf8048a8db23aa758bac9

💬 fjahr commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#issuecomment-2080516946)
@Umiiii I am not sure why you are asking me because I am not familiar with this change yet. I'm sure a maintainer will take a look at this change soon. You need to have a bit of patience though, there are a lot of open PRs and Issues and this being open for 3 days is not very long. And in general, more review is always better, 3 ACKs is the rule of thumb.

And please remove the @ mentions in your commit message. The commit message is merged with the change and that leads to a lot of unnecessar
...

📝 laanwj opened a pull request: "net: Modernize logging in UPnP and nat-pmp code"
(https://github.com/bitcoin/bitcoin/pull/29978)
i’m looking at this code anyway for #17012, so thought i’d might just as well modernize the logging:

- Use log level and log category `NET` (comes closest imo-because the mapping is for P2P)
- Port mapping errors are logged to Warning category instead of Error, because they’re not fatal, and not generally considered very serious problems.
- Prefix UPnP and natpmp messages consistently.