💬 furszy commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r1628301329)
> It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the `preset_inputs` not also be reduced to their effective amount in line 1139?
Thats because `preset_inputs.total_amount` could either be the full amount or the effective one (see `PreSelectedInputs`). I didn't want to introduce more changes within this PR but we should split the amounts (probably replacing `PreSelectedinputs` usage for `CoinsResult`).
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r1628301329)
> It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the `preset_inputs` not also be reduced to their effective amount in line 1139?
Thats because `preset_inputs.total_amount` could either be the full amount or the effective one (see `PreSelectedInputs`). I didn't want to introduce more changes within this PR but we should split the amounts (probably replacing `PreSelectedinputs` usage for `CoinsResult`).
📝 theuni opened a pull request: "build: warn on self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30235)
Belt-and suspenders after #30234. Self-assignment should be safe _and_ discouraged.
We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore.
(https://github.com/bitcoin/bitcoin/pull/30235)
Belt-and suspenders after #30234. Self-assignment should be safe _and_ discouraged.
We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore.
💬 theuni commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2150803796)
@maflcko Hmm, how best to ignore the tidy false-positive in a leveldb header?
> ./leveldb/include/leveldb/status.h:106:24: error: operator=() does not handle self-assignment properly
It looks like we could add `ExcludeHeaderFilterRegex` to our tidy config, but that's not wired up until clang-tidy-19.
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2150803796)
@maflcko Hmm, how best to ignore the tidy false-positive in a leveldb header?
> ./leveldb/include/leveldb/status.h:106:24: error: operator=() does not handle self-assignment properly
It looks like we could add `ExcludeHeaderFilterRegex` to our tidy config, but that's not wired up until clang-tidy-19.
💬 fanquake commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150813579)
```bash
test/uint256_tests.cpp:273:7: error: explicitly assigning value of variable of type 'arith_uint256' to itself [-Werror,-Wself-assign-overloaded]
v /= v;
~ ^ ~
test/uint256_tests.cpp:277:7: error: explicitly assigning value of variable of type 'arith_uint256' to itself [-Werror,-Wself-assign-overloaded]
v -= v;
~ ^ ~
2 errors generated.
```
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150813579)
```bash
test/uint256_tests.cpp:273:7: error: explicitly assigning value of variable of type 'arith_uint256' to itself [-Werror,-Wself-assign-overloaded]
v /= v;
~ ^ ~
test/uint256_tests.cpp:277:7: error: explicitly assigning value of variable of type 'arith_uint256' to itself [-Werror,-Wself-assign-overloaded]
v -= v;
~ ^ ~
2 errors generated.
```
💬 theuni commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150869143)
Ran into this: https://github.com/llvm/llvm-project/issues/42469
Worked around it by disabling the warning for the specific tests rather than removing them.
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150869143)
Ran into this: https://github.com/llvm/llvm-project/issues/42469
Worked around it by disabling the warning for the specific tests rather than removing them.
💬 theuni commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150905605)
Turns out [clang fixed this in v17](https://github.com/llvm/llvm-project/commit/c5302325b2a62d77cf13dd16cd5c19141862fed0). Updated the comment and commit message to reflect that.
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150905605)
Turns out [clang fixed this in v17](https://github.com/llvm/llvm-project/commit/c5302325b2a62d77cf13dd16cd5c19141862fed0). Updated the comment and commit message to reflect that.
👍 TheCharlatan approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2100282452)
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2100282452)
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
📝 theuni opened a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236)
Noticed while looking at the `-wno-*` flags in #30235.
This was disabled in #18738 due to the combo of old gcc and qt. We no longer support the affected gcc, and the old qt should no longer be relevant to us anyway.
See old fixes in:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88136
and
https://bugreports.qt.io/browse/QTBUG-75210
and
https://codereview.qt-project.org/c/qt/qtbase/+/245434
(https://github.com/bitcoin/bitcoin/pull/30236)
Noticed while looking at the `-wno-*` flags in #30235.
This was disabled in #18738 due to the combo of old gcc and qt. We no longer support the affected gcc, and the old qt should no longer be relevant to us anyway.
See old fixes in:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88136
and
https://bugreports.qt.io/browse/QTBUG-75210
and
https://codereview.qt-project.org/c/qt/qtbase/+/245434
💬 mzumsande commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2150971292)
Not sure if it's helpful to do anything in between releases, but if it's still broken in August, I'd suggest to remove it for 28.0 (a new seeder will be added in #30007, so the total number wouldn't change).
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2150971292)
Not sure if it's helpful to do anything in between releases, but if it's still broken in August, I'd suggest to remove it for 28.0 (a new seeder will be added in #30007, so the total number wouldn't change).
🤔 ryanofsky reviewed a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2100288867)
Sorry for not seeing this earlier. I think I found a potential problem with this change, so I left a comment below and would be curious to find out more.
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2100288867)
Sorry for not seeing this earlier. I think I found a potential problem with this change, so I left a comment below and would be curious to find out more.
💬 ryanofsky commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1628436308)
In commit "p2p: For assumeutxo, download snapshot chain before background chain" (ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883)
It seems like the `pindexLastCommonBlock->nHeight < snapshot_height` check is not strict enough because the snapshot block might not necessarily be an ancestor the peer's best known block.
In this case, the `pindexLastCommonBlock` assignments on line 1406 and 1411 will reset it back to the fork point between the two nodes each time `FindNextBlocksToDownload` is called
...
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1628436308)
In commit "p2p: For assumeutxo, download snapshot chain before background chain" (ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883)
It seems like the `pindexLastCommonBlock->nHeight < snapshot_height` check is not strict enough because the snapshot block might not necessarily be an ancestor the peer's best known block.
In this case, the `pindexLastCommonBlock` assignments on line 1406 and 1411 will reset it back to the fork point between the two nodes each time `FindNextBlocksToDownload` is called
...
🤔 cbergqvist reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2089208116)
Code reviewed e11fa4524723aebc0d879ee083c2d6d46849d89a.
Overall concept: Not fully convinced about the benefit of breaking out `LegacyDataSPKM`, seems to add complexity. Is the goal to delete `LegacyScriptPubKeyMan` in the future when ripping out BDB and only keep `LegacyDataSPKM`?
Why perform the inlining of `IsMine()` in 31e918afc3920a2f8aa367ad323f590b4856aa26 before the function is removed? Seems risky in case of other PRs modifying it prior to removal etc.
`LegacyScriptPubKeyMan::N
...
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2089208116)
Code reviewed e11fa4524723aebc0d879ee083c2d6d46849d89a.
Overall concept: Not fully convinced about the benefit of breaking out `LegacyDataSPKM`, seems to add complexity. Is the goal to delete `LegacyScriptPubKeyMan` in the future when ripping out BDB and only keep `LegacyDataSPKM`?
Why perform the inlining of `IsMine()` in 31e918afc3920a2f8aa367ad323f590b4856aa26 before the function is removed? Seems risky in case of other PRs modifying it prior to removal etc.
`LegacyScriptPubKeyMan::N
...
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628300902)
This class and possibly `LegacyScriptPubKeyMan` below could do with comments summarizing their responsibilities since breaking them apart and naming one `LegacyDataSPKM` makes things less clear.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628300902)
This class and possibly `LegacyScriptPubKeyMan` below could do with comments summarizing their responsibilities since breaking them apart and naming one `LegacyDataSPKM` makes things less clear.
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621331017)
Why reset `require_format` here? Seems `options` is not going to be read by anything after this line and none of the database types is holding a reference to it.
Avoiding that would mean that you could introduce more immutability around line 4386:
```
const DatabaseOptions options{
.require_existing = true,
.require_format = DatabaseFormat::BERKELEY_RO,
};
```
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621331017)
Why reset `require_format` here? Seems `options` is not going to be read by anything after this line and none of the database types is holding a reference to it.
Avoiding that would mean that you could introduce more immutability around line 4386:
```
const DatabaseOptions options{
.require_existing = true,
.require_format = DatabaseFormat::BERKELEY_RO,
};
```
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628376667)
*`"Error reading wallet database: LegacyDataSPKM::LoadKey failed"`, same for `LoadCryptedKey` and `LoadCScript` below.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628376667)
*`"Error reading wallet database: LegacyDataSPKM::LoadKey failed"`, same for `LoadCryptedKey` and `LoadCScript` below.
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621407661)
```suggestion
// Only create base LegacyDataSPKM if using BERKELEY_RO
std::unique_ptr<ScriptPubKeyMan> spk_manager = m_database->Format() == "bdb_ro" ?
std::make_unique<LegacyDataSPKM>(*this) :
std::make_unique<LegacyScriptPubKeyMan>(*this, m_keypool_size);
```
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621407661)
```suggestion
// Only create base LegacyDataSPKM if using BERKELEY_RO
std::unique_ptr<ScriptPubKeyMan> spk_manager = m_database->Format() == "bdb_ro" ?
std::make_unique<LegacyDataSPKM>(*this) :
std::make_unique<LegacyScriptPubKeyMan>(*this, m_keypool_size);
```
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628371045)
This comment goes on to say `AddWatchOnly(const CScript&)` is an inherited virtual method which appears untrue. Maybe add an extra commit ahead of the others that tidies up that comment?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628371045)
This comment goes on to say `AddWatchOnly(const CScript&)` is an inherited virtual method which appears untrue. Maybe add an extra commit ahead of the others that tidies up that comment?
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628438776)
In commit 46520aacc43dd42a7968587ab9ece488bbe9503d: Should this line have been removed in the following commit (31e918afc3920a2f8aa367ad323f590b4856aa26)?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628438776)
In commit 46520aacc43dd42a7968587ab9ece488bbe9503d: Should this line have been removed in the following commit (31e918afc3920a2f8aa367ad323f590b4856aa26)?
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628492436)
Worth adding a comment about the consequences of setting `keypool_size=0`? I'm guessing there are still cases where we generate some pubkeys despite having a zero pool size, otherwise you would have removed some of the code and the for-loop below?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628492436)
Worth adding a comment about the consequences of setting `keypool_size=0`? I'm guessing there are still cases where we generate some pubkeys despite having a zero pool size, otherwise you would have removed some of the code and the for-loop below?
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628408816)
Could be marked `static`?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628408816)
Could be marked `static`?