💬 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

💬 paplorinc commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581828040)
same

💬 paplorinc commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581828932)
Some of these should probably be debugs, if they're spammy

💬 paplorinc commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581828336)
was the message changed on purpose to confirm to the rest of the logs?
Shouldn't we rather remove the prefixes now that the category is specified?

💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581830592)
i've mentioned this in my post: these are intentionally logged at Warning level as port mapping failures are not generally considered very serious problems, there's no reason to unnecessarily worry users about them