💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223866061)
```suggestion
- `unloadwallet` - Return RPC_INVALID_PARAMETER when both the RPC wallet endpoint
and wallet_name parameters are unspecified. Previously the RPC failed with a JSON
parsing error.
```

💬 ismaelsadeeq commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2223903289)

Makes sense, I think we can make the bounds not just an arbitrary number but tied it to the cores of the machine.

That will prevent footgoon whereby user will spawn more threads than the machine can handle leading to degraded performance due to lots of context switching.

✅ achow101 closed a pull request: "test: functional test for incomplete PSBT with additional signatures required"
(https://github.com/bitcoin/bitcoin/pull/33035)

💬 achow101 commented on pull request "test: functional test for incomplete PSBT with additional signatures required":
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3104989559)
What is the purpose of this test? There are already multiple tests that do exactly what you have added, there is nothing useful being added here.

Please do not use a LLM to write your code.

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