💬 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

🚀 achow101 merged a pull request: "wallet: remove outdated `pszSkip` arg of database `Rewrite` func"
(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
...

🤔 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`?

...

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

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

✅ 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)

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

✅ 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)

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

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

💬 ajtowns commented on pull request "Enable `-natpmp` by default":
(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
...

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

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

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

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

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

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

🤔 yuvicc reviewed a pull request: "doc: remove dangling toc entries from `developer-notes.md`"
(https://github.com/bitcoin/bitcoin/pull/33040#pullrequestreview-3045563685)
I think we can also add

```
- [Interface Naming Style]([Internal interface
naming style](#internal-interface-naming-style))
```