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

💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064565125)
Why restrict to 22.0 and above?

💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064544864)
Please put these with their respective versions above.

If we're going to do Windows testing as well, then we should be downloading all of the versions above, not just this small subset. This subset doesn't even make sense, there's no single test that uses just these versions. `wallet_migration.py` uses only 28.0, the rest require the older versions too.

💬 achow101 commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2064578902)
What is the purpose of this host triple? We don't make Windows binaries with any other host triple.