💬 l0rinc commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#issuecomment-3105072573)
Please see the follow-up, removing the corresponding TOC entries: https://github.com/bitcoin/bitcoin/pull/33040
(https://github.com/bitcoin/bitcoin/pull/32572#issuecomment-3105072573)
Please see the follow-up, removing the corresponding TOC entries: https://github.com/bitcoin/bitcoin/pull/33040
📝 achow101 opened a pull request: "wallet: Set minversion to FEATURE_LATEST during migration"
(https://github.com/bitcoin/bitcoin/pull/33041)
Although the minversion is not used by descriptor wallets, we should still update it to the latest version for consistency.
No test as any test requires very old versions of Bitcoin Core.
(https://github.com/bitcoin/bitcoin/pull/33041)
Although the minversion is not used by descriptor wallets, we should still update it to the latest version for consistency.
No test as any test requires very old versions of Bitcoin Core.
💬 achow101 commented on pull request "wallet: remove outdated `pszSkip` arg of database `Rewrite` func":
(https://github.com/bitcoin/bitcoin/pull/32990#issuecomment-3105112040)
ACK 2dfeb6668cb2e98e3dccf946af084e8a08e1fab5
(https://github.com/bitcoin/bitcoin/pull/32990#issuecomment-3105112040)
ACK 2dfeb6668cb2e98e3dccf946af084e8a08e1fab5
🚀 achow101 merged a pull request: "wallet: remove outdated `pszSkip` arg of database `Rewrite` func"
(https://github.com/bitcoin/bitcoin/pull/32990)
(https://github.com/bitcoin/bitcoin/pull/32990)
💬 ajtowns commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3105269388)
> The behaviour already exists in a limited fashion. Even without this change, an attacker can trivially create such a transaction (as discussed in OP). Here is for instance a `p2p_invalid_tx.py` case which passes both with or without this PR:
Okay, but that just means the extra work isn't providing much value today either?
> I don't think this logic is actually meant to prevent an attacker from wasting resources anyways. I think it's rather to make sure we don't split the network when int
...
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3105269388)
> The behaviour already exists in a limited fashion. Even without this change, an attacker can trivially create such a transaction (as discussed in OP). Here is for instance a `p2p_invalid_tx.py` case which passes both with or without this PR:
Okay, but that just means the extra work isn't providing much value today either?
> I don't think this logic is actually meant to prevent an attacker from wasting resources anyways. I think it's rather to make sure we don't split the network when int
...
🤔 pablomartin4btc reviewed a pull request: "wallet: Set minversion to FEATURE_LATEST during migration"
(https://github.com/bitcoin/bitcoin/pull/33041#pullrequestreview-3045248689)
ACK 5aa758861cf1399842fd0bea3a42d5b0cafcb0f6
I've noticed [this](https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079415883) while reviewing the removal of `upgradewallet` RPC [PR](#32944).
Please consider also the _[version related functions removal PR](#32977)_, if you haven't already, shouldn't `SetMinVersion` [renamed](https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208482177)? And perhaps change the first arg to a `const` so we can remove the features `enum`?
...
(https://github.com/bitcoin/bitcoin/pull/33041#pullrequestreview-3045248689)
ACK 5aa758861cf1399842fd0bea3a42d5b0cafcb0f6
I've noticed [this](https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079415883) while reviewing the removal of `upgradewallet` RPC [PR](#32944).
Please consider also the _[version related functions removal PR](#32977)_, if you haven't already, shouldn't `SetMinVersion` [renamed](https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208482177)? And perhaps change the first arg to a `const` so we can remove the features `enum`?
...
💬 ajtowns commented on pull request "test: Move `script_assets_tests` into its own suite":
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-3105300251)
ACK c40dbbbf77078bb86a652ca151b472e7bef61ae0
You could consider rebasing it on top of #32945 in order to not move the code to a different file, but that still involves moving code about and only avoids duplicating the `ToScript` function across both files.
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-3105300251)
ACK c40dbbbf77078bb86a652ca151b472e7bef61ae0
You could consider rebasing it on top of #32945 in order to not move the code to a different file, but that still involves moving code about and only avoids duplicating the `ToScript` function across both files.
💬 pablomartin4btc commented on pull request "wallet: Set minversion to FEATURE_LATEST during migration":
(https://github.com/bitcoin/bitcoin/pull/33041#discussion_r2224121095)
How this is related with the `SetMinVersion(FEATURE_LATEST` call during the wallet creation in the migration process?
https://github.com/bitcoin/bitcoin/blob/900bb53905aaf0a7b45c1471c5248d7e340ba27b/src/wallet/wallet.cpp#L2861-L2870
Shouldn't that instance of the call be removed as the one you are adding here is more generic?
(https://github.com/bitcoin/bitcoin/pull/33041#discussion_r2224121095)
How this is related with the `SetMinVersion(FEATURE_LATEST` call during the wallet creation in the migration process?
https://github.com/bitcoin/bitcoin/blob/900bb53905aaf0a7b45c1471c5248d7e340ba27b/src/wallet/wallet.cpp#L2861-L2870
Shouldn't that instance of the call be removed as the one you are adding here is more generic?
✅ luisschwab closed a pull request: "wallet: make coinbase that will mature on the next block available for selection"
(https://github.com/bitcoin/bitcoin/pull/32123)
(https://github.com/bitcoin/bitcoin/pull/32123)
💬 luisschwab commented on pull request "wallet: make coinbase that will mature on the next block available for selection":
(https://github.com/bitcoin/bitcoin/pull/32123#issuecomment-3105367383)
Closing as it failed to reach consensus.
(https://github.com/bitcoin/bitcoin/pull/32123#issuecomment-3105367383)
Closing as it failed to reach consensus.
✅ luisschwab closed an issue: "Incorrect balance when dealing with coinbase UTXOs that will be mature on `h + 1`"
(https://github.com/bitcoin/bitcoin/issues/32098)
(https://github.com/bitcoin/bitcoin/issues/32098)
💬 luisschwab commented on issue "Incorrect balance when dealing with coinbase UTXOs that will be mature on `h + 1`":
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-3105368063)
Closing as it failed to reach consensus.
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-3105368063)
Closing as it failed to reach consensus.
💬 achow101 commented on pull request "wallet: Set minversion to FEATURE_LATEST during migration":
(https://github.com/bitcoin/bitcoin/pull/33041#discussion_r2224155676)
That is only called on actually newly created wallets. A migrated wallet is not newly created and we don't use `Create` for the migrated wallet. It's only used for the watchonly and solvables wallets.
(https://github.com/bitcoin/bitcoin/pull/33041#discussion_r2224155676)
That is only called on actually newly created wallets. A migrated wallet is not newly created and we don't use `Create` for the migrated wallet. It's only used for the watchonly and solvables wallets.
💬 ajtowns commented on pull request "Enable `-natpmp` by default":
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3105380291)
ACK b2d07f872c58af9cfdf9f9a4af0645376f9b98cb
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3105380291)
ACK b2d07f872c58af9cfdf9f9a4af0645376f9b98cb
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224162725)
Thanks for pointed that out. Forgot to remove `wallet::` in the `EnsureUniqueWalletName()` call. The reason I wrapped it in `namespace wallet` is that it helps to avoid verbose `wallet::` prefixes and reduces the chance of symbol collisions. It's also consistent with most of the other unit tests under `src/wallet/test`, which commonly wrap helper functions and test cases in the same namespace.
That said, I agree `TestWalletName()` should probably be marked `static` to indicate internal linkag
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224162725)
Thanks for pointed that out. Forgot to remove `wallet::` in the `EnsureUniqueWalletName()` call. The reason I wrapped it in `namespace wallet` is that it helps to avoid verbose `wallet::` prefixes and reduces the chance of symbol collisions. It's also consistent with most of the other unit tests under `src/wallet/test`, which commonly wrap helper functions and test cases in the same namespace.
That said, I agree `TestWalletName()` should probably be marked `static` to indicate internal linkag
...
📝 l0rinc opened a pull request: "refactor: inline constant return values from `dbwrapper` write methods"
(https://github.com/bitcoin/bitcoin/pull/33042)
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480
### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.
### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a ret
...
(https://github.com/bitcoin/bitcoin/pull/33042)
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480
### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.
### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a ret
...
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178196)
Done!
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178196)
Done!
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178345)
Done!
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178345)
Done!
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178469)
Done!
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178469)
Done!
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3105471281)
Addressed [feedback](https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3044915254) from @stickies-v.
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3105471281)
Addressed [feedback](https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3044915254) from @stickies-v.
💬 ajtowns commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2224314216)
`if (!mask) mask += nonzero;` would be simpler, no? Or would that bias the fuzzer in undesirable ways somehow? Could perhaps do `mask += (!mask & nonzero)` to avoid an explicit branch.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2224314216)
`if (!mask) mask += nonzero;` would be simpler, no? Or would that bias the fuzzer in undesirable ways somehow? Could perhaps do `mask += (!mask & nonzero)` to avoid an explicit branch.