💬 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).
(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.
(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.
(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
(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
(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
(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
(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
(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.
(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.
(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.
(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?
(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
(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
...
(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.
(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
...
(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
...
💬 maflcko 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-2080852359)
If the transaction is eventually considered abandoned in all cases, then adding the `sync_mempools` (or I guess `syncwithvalidationinterface` would be enough) seems fine.
(https://github.com/bitcoin/bitcoin/issues/29806#issuecomment-2080852359)
If the transaction is eventually considered abandoned in all cases, then adding the `sync_mempools` (or I guess `syncwithvalidationinterface` would be enough) seems fine.
⚠️ luke-jr opened an issue: ""Migrate Wallet" is unclear to translators"
(https://github.com/bitcoin/bitcoin/issues/29979)
Going through translations, I'm seeing things that suggest "migration" is being misinterpreted in the sense of animals migrating (moving/relocating). We could clarify with translator comments (assuming translators actually see them), but that won't help if non-native English speakers are using Bitcoin Core in English, so maybe a better approach is to come up with another word for this?
(https://github.com/bitcoin/bitcoin/issues/29979)
Going through translations, I'm seeing things that suggest "migration" is being misinterpreted in the sense of animals migrating (moving/relocating). We could clarify with translator comments (assuming translators actually see them), but that won't help if non-native English speakers are using Bitcoin Core in English, so maybe a better approach is to come up with another word for this?
💬 m3dwards commented on pull request "depends: pass verbose through to cmake based makefiles":
(https://github.com/bitcoin/bitcoin/pull/29960#issuecomment-2081071753)
I have tried to look for an alternative variable that will enable verbose output regardless of build system but I couldn't find one. I thought this looked promising [https://cmake.org/cmake/help/latest/envvar/VERBOSE.html](https://cmake.org/cmake/help/latest/envvar/VERBOSE.html) but it didn't work for me.
(https://github.com/bitcoin/bitcoin/pull/29960#issuecomment-2081071753)
I have tried to look for an alternative variable that will enable verbose output regardless of build system but I couldn't find one. I thought this looked promising [https://cmake.org/cmake/help/latest/envvar/VERBOSE.html](https://cmake.org/cmake/help/latest/envvar/VERBOSE.html) but it didn't work for me.
💬 tdb3 commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2081107100)
> > both curl and bitcoin-cli returned an error:
>
> I suspect you're running with `-rpcdoccheck`, you shouldn't get any errors without it? I'm not sure if `-rpcdoccheck` is meant to be an internal-only testing flag (as per #24695), but it [doesn't seem to be documented as such](https://github.com/bitcoin/bitcoin/blob/2a07c4662d7266158d47f79fa2433ab22e22c907/src/init.cpp#L655), so I guess better to be safe - force pushed to make the docs contingent on the `-rpcdeprecated` too.
>
> Thanks f
...
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2081107100)
> > both curl and bitcoin-cli returned an error:
>
> I suspect you're running with `-rpcdoccheck`, you shouldn't get any errors without it? I'm not sure if `-rpcdoccheck` is meant to be an internal-only testing flag (as per #24695), but it [doesn't seem to be documented as such](https://github.com/bitcoin/bitcoin/blob/2a07c4662d7266158d47f79fa2433ab22e22c907/src/init.cpp#L655), so I guess better to be safe - force pushed to make the docs contingent on the `-rpcdeprecated` too.
>
> Thanks f
...