💬 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

💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581830803)
I have no reason to believe this message is spammy. Mind that it was logged as an unconditional message, and no one ever complained (this one is also reasonably important).

💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#discussion_r1581830894)
Yes, this was on purpose. i've kept the prefix because nat-pmp and UPnP messages should be distinguishable.

👍 hebasto approved a pull request: "build: Bump clang minimum supported version to 15"
(https://github.com/bitcoin/bitcoin/pull/29165#pullrequestreview-2026650269)
ACK fa1964c5b80ee28b0e06abdbd9a26e8e8c6f5acd.

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581824840)
nit: I'd prefer `window_tx_count.value()`, same below

🤔 fjahr reviewed a pull request: "rpc: Avoid getchaintxstats invalid results"
(https://github.com/bitcoin/bitcoin/pull/29720#pullrequestreview-2026643314)
tACK fa28613a2cc75e5668900a11337e441546f86358

I have written some additional coverage to test the behavior and build my understanding, it could be added here or I can make a follow-up: https://github.com/fjahr/bitcoin/commit/03a0f31dde51c094ad8e3df9f5d72abfe216c981

💬 fjahr commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581824200)
nit: we usually write `assumeutxo` without a dash

🤔 furszy reviewed a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2026654546)
Concept ACK

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2080752984)
Re-ACK 9552619961049d7673f84c40917b0385be70b782 , just addressing nits, no other changes

💬 furszy commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1581838289)
sure, will change it if I have to retouch the code.

💬 furszy commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1581838453)
sure, will change it if I have to retouch this code.