💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629143813)
`LegacyScriptPubKeyMan` is a `LegacyDataSPKM`, so the comment would be more correct, but it could be corrected later when removing `LegacyScriptPubKeyMan` - unless you plan to rename `LegacyDataSPKM` back into `LegacyScriptPubKeyMan` once the deletion of the latter is complete.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629143813)
`LegacyScriptPubKeyMan` is a `LegacyDataSPKM`, so the comment would be more correct, but it could be corrected later when removing `LegacyScriptPubKeyMan` - unless you plan to rename `LegacyDataSPKM` back into `LegacyScriptPubKeyMan` once the deletion of the latter is complete.
💬 hebasto commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2151853425)
> Hmm, how best to ignore the tidy false-positive in a leveldb header?
FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2151853425)
> Hmm, how best to ignore the tidy false-positive in a leveldb header?
FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.
💬 fanquake commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2151861139)
> FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.
Is that actually the behaviour we want for all subtrees? Sounds like it could just hide bugs, and no-one is running any checks in the upstream repos.
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2151861139)
> FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.
Is that actually the behaviour we want for all subtrees? Sounds like it could just hide bugs, and no-one is running any checks in the upstream repos.
💬 maflcko commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1629164166)
Can you explain why this change in behavior is correct? Previously, if the peer was missing, it would return `true`. Now, it would return `false`.
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1629164166)
Can you explain why this change in behavior is correct? Previously, if the peer was missing, it would return `true`. Now, it would return `false`.
👍 fanquake approved a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236#pullrequestreview-2101490603)
ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3 - this is in `-Wextra` for Clang and GCC.
(https://github.com/bitcoin/bitcoin/pull/30236#pullrequestreview-2101490603)
ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3 - this is in `-Wextra` for Clang and GCC.
🚀 fanquake merged a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236)
(https://github.com/bitcoin/bitcoin/pull/30236)
💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151869901)
An alternative would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151869901)
An alternative would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
💬 fanquake commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151878946)
> An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151878946)
> An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.
🚀 fanquake merged a pull request: "build: no-longer allow GCC-10 in C++20 check"
(https://github.com/bitcoin/bitcoin/pull/30228)
(https://github.com/bitcoin/bitcoin/pull/30228)
💬 hebasto commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2151924530)
CMake's https://github.com/hebasto/bitcoin/pull/191 is built on top of this PR change.
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2151924530)
CMake's https://github.com/hebasto/bitcoin/pull/191 is built on top of this PR change.
🤔 hebasto reviewed a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236#pullrequestreview-2101568286)
Post-merge ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3.
(https://github.com/bitcoin/bitcoin/pull/30236#pullrequestreview-2101568286)
Post-merge ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3.
💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151947313)
> > An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
>
> Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.
Ah sorry, looks like they didn't change anything about the deprecation in C++23 and it is kept as-is for now.
However, I found that this patch seems incomplete. For example, no warning is printed to catch https://eel.is/c++draft/depr.
...
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151947313)
> > An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
>
> Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.
Ah sorry, looks like they didn't change anything about the deprecation in C++23 and it is kept as-is for now.
However, I found that this patch seems incomplete. For example, no warning is printed to catch https://eel.is/c++draft/depr.
...
💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151951796)
For reference https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2139r2.html#3.8 said:
> Strong recommendation: take decisive action early in the C++23 development cycle, so early adopters can shake out the remaining cost of updating old code. Note that this would be both an API and ABI breaking change, and it would be good to set that precedent early if we wish to allow such breakage in C++23.
But that didn't happen.
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151951796)
For reference https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2139r2.html#3.8 said:
> Strong recommendation: take decisive action early in the C++23 development cycle, so early adopters can shake out the remaining cost of updating old code. Note that this would be both an API and ABI breaking change, and it would be good to set that precedent early if we wish to allow such breakage in C++23.
But that didn't happen.
💬 hebasto commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151952891)
CMake includes dependencies with `-isystem`, which is equivalent to `suppress_external_warnings = yes`. Therefore, the block removed in this PR has never been a part of the CMake staging branch.
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151952891)
CMake includes dependencies with `-isystem`, which is equivalent to `suppress_external_warnings = yes`. Therefore, the block removed in this PR has never been a part of the CMake staging branch.
💬 fanquake commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151955730)
> unless -Wdeprecated-copy-dtor is passed.
I think we can turn on additional warning flags, if you think that is valuable.
> CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.
We may still need a way to disable that functionality, so all warnings can be observed, so I think this block is still relevant.
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151955730)
> unless -Wdeprecated-copy-dtor is passed.
I think we can turn on additional warning flags, if you think that is valuable.
> CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.
We may still need a way to disable that functionality, so all warnings can be observed, so I think this block is still relevant.
💬 fanquake commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151958943)
See comments starting here: https://github.com/bitcoin/bitcoin/pull/30189#issuecomment-2137467259. cc @maflcko.
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151958943)
See comments starting here: https://github.com/bitcoin/bitcoin/pull/30189#issuecomment-2137467259. cc @maflcko.
💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151968101)
> > unless -Wdeprecated-copy-dtor is passed.
>
> I think we can turn on additional warning flags, if you think that is valuable.
Yeah, not sure how useful that'd be, given that the deprecation apparently won't result in a deletion (for now).
> > CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.
>
> We may still need a way to disable that functionality, so all warnings can be observed, so I think this block (or atleast the function
...
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151968101)
> > unless -Wdeprecated-copy-dtor is passed.
>
> I think we can turn on additional warning flags, if you think that is valuable.
Yeah, not sure how useful that'd be, given that the deprecation apparently won't result in a deletion (for now).
> > CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.
>
> We may still need a way to disable that functionality, so all warnings can be observed, so I think this block (or atleast the function
...
💬 fanquake commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2152016135)
> Yeah, could open a tracking issue in the cmake repo (hebasto/bitcoin)?
Opened one here so it isn't forgotten: https://github.com/hebasto/bitcoin/issues/223.
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2152016135)
> Yeah, could open a tracking issue in the cmake repo (hebasto/bitcoin)?
Opened one here so it isn't forgotten: https://github.com/hebasto/bitcoin/issues/223.
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629164462)
```suggestion
// Implements the full legacy wallet behavior
// Slated for deletion once Berkeley DB library dependency is removed.
class LegacyScriptPubKeyMan : public LegacyDataSPKM
```
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629164462)
```suggestion
// Implements the full legacy wallet behavior
// Slated for deletion once Berkeley DB library dependency is removed.
class LegacyScriptPubKeyMan : public LegacyDataSPKM
```
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629293058)
Haven't fully grokked the code yet, so I understand if you'd rather not answer my questions, but if you want:
> I don't think this needs to have any additional explanation. keypool_size only matters for ranged descriptors. Non-HD keys do not make ranged descriptors, so the value here is ignored.
We're in an HD chain `for`-loop, how is this not involving HD keys?
Surely legacy wallets could be HD wallets.
> The loop below is for an entirely different set of descriptors and is unrelated
...
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629293058)
Haven't fully grokked the code yet, so I understand if you'd rather not answer my questions, but if you want:
> I don't think this needs to have any additional explanation. keypool_size only matters for ranged descriptors. Non-HD keys do not make ranged descriptors, so the value here is ignored.
We're in an HD chain `for`-loop, how is this not involving HD keys?
Surely legacy wallets could be HD wallets.
> The loop below is for an entirely different set of descriptors and is unrelated
...