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

💬 theStack commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2080579036)
Thanks for review and the feedback. I added commits to remove unused Popen methods (`poll` and `kill`; note that `pid` and `wait` are used internally) and another one that removes obsolete `check_output` comments and also gets rid of the Popen API numbering.

👋 maflcko's pull request is ready for review: "build: Bump clang minimum supported version to 15"
(https://github.com/bitcoin/bitcoin/pull/29165)

🤔 paplorinc reviewed a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2026623423)
Recreated the change to understand it better, commented on what I've noticed.

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581784723)
seems to me the related comments needs to be updated in this file (e.g line 184 and 199)

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581824896)
Does this reassignment still make sense?

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581825870)
we might as well use `blockPos` here
```C++
m_blockman.AddToBlockFileInfo(block, pindex->nHeight, blockPos);
```

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581825021)
should we keep the original comment here?

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581825604)
Does the comment on line 215 need any update after the change?

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581822649)
nit: is the naming style deliberate here? When is it camel and when snake?

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581826711)
we could move this closer to the usage