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

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2080804374)
Rebased and taken out of draft. I've opened two pull requests against oss-fuzz to bump the compiler there (one globally and one for bitcoin-core), but they are waiting for merge. If they are not merged, I think we can consider oss-fuzz as unsupported and drop it temporarily. Oss-fuzz is nice, but it seems odd to consider it a blocker here.

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2080828548)
> 03a0f31

I think `assert_equal` can generally not be used on floating point values, no?

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581842446)
Thanks, done

💬 Randy808 commented on issue "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError":
(https://github.com/bitcoin/bitcoin/issues/29806#issuecomment-2080840395)
The [relevant part of the test](https://github.com/bitcoin/bitcoin/blob/7fee0ca014b1513e836d2550d1b7512739d5a79a/test/functional/wallet_backwards_compatibility.py#L173-L176) in `wallet_backwards_compatibility.py` creates tx3, calls bump fee to replace it in the wallet (creating tx4), and calls `abandontransaction` on tx3. Later on there's an assertion to check if tx3 is marked as abandoned, but this is the call that sporadically fails.

The failure seems to occur because the `TransactionAddedT
...

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581842910)
I think both should behave the same in this context.

💬 stickies-v commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2080843513)
> The problem is that https://github.com/bitcoin/bitcoin/pull/29700 updates some validation functions to return util::Result instead of bool.

I see, that was not apparent in the [link](https://github.com/ryanofsky/bitcoin/blob/55d7de92bbbe035c1833c89f885af14e5b243932/src/node/chainstate.cpp#L38-L179) you shared earlier. Would it make sense to do that refactoring in #29700 then, instead of in this PR? Because in this PR, I don't think the change makes a lot of sense on its own.

> I think a
...