💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064466039)
Probably, but I'm going to leave that for a followup, this diff is large enough already.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064475635)
Descriptor wallets can't use non-HD keys in the "keypool", so it doesn't make sense to keep this for such wallets.

All migrated wallets get new descriptors anyways for new addresses. The keypool is entirely dumped.

💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064478495)
Added comment.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064483504)
`flush()` is inherited from `ChainClient`, which defines it as pure virtual, so it needs to have an implementation here even if it does nothing.

💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064485063)
I've applied that patch as-is.

📝 Retropex opened a pull request: "Catch nocive arbitrary data. (missing code for #32359)"
(https://github.com/bitcoin/bitcoin/pull/32368)
Some people in the discussion of the PR #32359 are suggesting that `OP_RETURN` should be used instead of `OP_IF`, `OP_FALSE` data injections.

IMO, PR #32359 should never be merged, as it would encourage the storage of arbitrary data on Bitcoin. But if the PR is still merged, then this code should be added, because [large data insertions have no incentive](https://bitcoin.stackexchange.com/a/122322) to move to `OP_RETURN`.

![image](https://github.com/user-attachments/assets/32005c35-9c38-42
...

💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064490005)
So we actually test the OP_RETURN followed by non-push in `src/test/transaction_tests.cpp`. But it wouldn't hurt to test it separately in terms of mempool-acceptance.

💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064504791)
I included this patch and @darosior's patch verbatim.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064505718)
Indeed, removed.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064508912)
Apparently since this PR was opened, descriptor specific tests were added to those files. I've removed the legacy ones from them and added back to the `CMakeLists.txt`.

💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2064510978)
Fixed.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064511069)
Removed

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064514419)
Removed from the help text.

I've cleaned up some of the legacy wallet mentions that are no longer relevant. Some are still relevant, and others are less obviously legacy wallet only things so I will leave those for a followup.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064514946)
Done

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2064515535)
Removed

💬 petertodd commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2836467665)
@Sjors @darosior I think I made all the code corrections you both suggested. Let me know if I missed anything.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2836470205)
> And a few details:

Removed these

> Also it might be good to rename the `importprivkey` test helper to e.g. `importkey`, so it's not confused with the disappeared RPC method.

I'm going to leave that for a followup.

> You can use `--patience`. Maybe add this to the commit message?

Done, also mentioned `--histogram`.

💬 BullishNode commented on issue "Allow sending untrusted utxos in the sendtoaddress api":
(https://github.com/bitcoin/bitcoin/issues/32034#issuecomment-2836503702)
Yes. Unconfirmed change is considered trusted.

💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064558105)
Why is this restricted to 22.0 and above? The naming scheme for the Windows zips hasn't changed.

💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064566628)
imports go at the top of the file.