💬 achow101 commented on pull request "Enable `-natpmp` by default":
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3105012258)
Concept ACK

💬 achow101 commented on pull request "test: delete commented-out tests and add a test case in wallet_signer":
(https://github.com/bitcoin/bitcoin/pull/33020#issuecomment-3105022393)
ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e

💬 achow101 commented on pull request "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures":
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3105042497)
The `CHECK_NONFATAL` is correct. `complete == true` means that the PSBT is complete and can be finalized. The invariant here is that `complete == true` means `FinalizeAndExtract` must succeed.

BIP 174 states that a finalizer must only construct valid scriptSigs and witnesses. We encounter this error because we assume that if a PSBT is finalized, the scriptSigs and witnesses are valid so we are not performing the extra step of verifying the script. However, in this situation, the finalizer has
...

🚀 achow101 merged a pull request: "test: delete commented-out tests and add a test case in wallet_signer"
(https://github.com/bitcoin/bitcoin/pull/33020)

💬 sipa commented on pull request "Enable `-natpmp` by default":
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3105053907)
utACK

💬 achow101 commented on pull request "init: [gui] Avoid UB/crash in InitAndLoadChainstate":
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3105070165)
ACK fac90e5261b811739ada56e06ea793a12f9c2c3d

📝 l0rinc opened a pull request: "doc: remove dangling toc entries from `developer-notes.md`"
(https://github.com/bitcoin/bitcoin/pull/33040)
The table of content wasn't adjusted when the corresponding entries were removed in https://github.com/bitcoin/bitcoin/pull/32572, see:
* `Ignoring IDE/editor files`: https://github.com/bitcoin/bitcoin/commit/faf65f05312be7647f485f088ba00fef97f47bf4
* `Shebang`: https://github.com/bitcoin/bitcoin/commit/7777fb8bc749e18c178ef460b65219187e676128
* `Wallet`: https://github.com/bitcoin/bitcoin/commit/fa69c5b170f56d554fcb0d0887bd27f961fe3e74

💬 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

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

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