💬 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

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1581827125)
This is the reason why
```C++
if (!m_blockfile_cursors[chain_type]) {
// If a snapshot is loaded during runtime, we may not have initialized this cursor yet.
assert(chain_type == BlockfileType::ASSUMED);
const auto new_cursor = BlockfileCursor{this->MaxBlockfileNum() + 1};
m_blockfile_cursors[chain_type] = new_cursor;
LogPrint(BCLog::BLOCKSTORAGE, "[%s] initializing blockfile cursor to %s\n", chain_type, new_cursor);
}
```
is not applicable here, right?
Would it
...

🤔 scgbckbone reviewed a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2026646688)
re-ACK (I'm pro default function parameter)

💬 paplorinc commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581828073)
the condition error code indicates this should also be an error

🤔 paplorinc reviewed a pull request: "net: Modernize logging in UPnP and nat-pmp code"
(https://github.com/bitcoin/bitcoin/pull/29978#pullrequestreview-2026646982)
+1

💬 paplorinc commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581827964)
the message states it's an error, but the level is warning

💬 paplorinc commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581828183)
same, won't repeat it for the rest