🤔 furszy reviewed a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2864553336)
Should also change [HasLegacyRecords()](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/walletdb.cpp#L544) as it would log the same error twice with this change.

✅ pinheadmz closed an issue: "getchaintips doesn't mark headers-only chain as invalid"
(https://github.com/bitcoin/bitcoin/issues/8050)

💬 pinheadmz commented on issue "getchaintips doesn't mark headers-only chain as invalid":
(https://github.com/bitcoin/bitcoin/issues/8050#issuecomment-2904551865)
Confirmed by running a test [I wrote for a draft PR](https://github.com/bitcoin/bitcoin/pull/27434/files#diff-8bd914231171fe351d1ea5bcb566576fad26da2d3d4c4c918b352871fd84809c)

before #30666:
```
[{'height': 20, 'hash': '2182ec6e445c3869f7d17bcf2bcae03c286256cf7359b3c7c834cee3ad029a3f', 'branchlen': 10, 'status': 'headers-only'}, {'height': 10, 'hash': '2e13e49c2f95b6b17c8eece52ce971ce5610a9c8cd824b7be139fcbadae6d0b2', 'branchlen': 0, 'status': 'active'}]
[{'height': 20, 'hash': '2182ec6e445c386
...

🤔 rkrux reviewed a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2864642456)
ACK 62ad4d80883191073ea39f8b8d5ccef0748414a1

> Should also change [HasLegacyRecords()](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/walletdb.cpp#L544) as it would log the same error twice with this change.

Can be removed as I checked that the only other usage of `HasLegacyRecords` is [directly inside](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/wallet.cpp#L4315) the `MigrateLegacyToDescriptor` fu
...

💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104750615)
> A better option would be to use the test's temp dir as scratch space

That doesn't work with the currently used `std::string` overload of `subprocess::Popen()` because of to the space in the full path. Is using `fs::temp_directory_path()` acceptable?

👍 theStack approved a pull request: "test: fix and augment block tests of invalid_txs"
(https://github.com/bitcoin/bitcoin/pull/32591#pullrequestreview-2864682937)
ACK 8fcd6845052354fad80ae7e5feda3f6a2e441e12


Diff of `feature_block.py` test runs between merge-base and PR (patched out the timestamps from the [logging formatter](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/test/functional/test_framework/test_framework.py#L833) in order to compare):
```
> TestFramework (INFO): Reject block with invalid tx: InvalidOPIFConstruction
14a16,30
> TestFramework (INFO): Reject block with invalid tx: DisabledOpcode_OP_CAT

...

💬 theStack commented on pull request "test: fix and augment block tests of invalid_txs":
(https://github.com/bitcoin/bitcoin/pull/32591#discussion_r2104754535)
nit: alternatively, could just remove the line since `False` is the default anyways, but no blocker

🤔 pinheadmz reviewed a pull request: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051#pullrequestreview-2864676242)
concept ACK and untested code review 93b07997e9

I believe this also closes an 11-year-old issue: https://github.com/bitcoin/bitcoin/issues/5097

💬 pinheadmz commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r2104750539)
64b956f4220300254126b166deb97849b236c2ed

Nit: could you use `NetPermissions::ToStrings(NetPermissionFlags flags)` just to avoid hard coding things?

💬 pinheadmz commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2904687014)
> I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?

If we're considering a case where the user specifically chooses specific peers for IBD for privacy or whatever reasons, don't we just let them have the slow sync they trade-off for?

👍 rkrux approved a pull request: "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs"
(https://github.com/bitcoin/bitcoin/pull/32596#pullrequestreview-2864719763)
ACK e5cbea416b2f63e5d99819052f3e69a6383336d6

💬 rkrux commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104780565)
Nice, thanks: https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2042020967

💬 rkrux commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104775279)
I assume this is a breaking change but fine because it will go in the same release that contains the removal of legacy wallets as well.

💬 maflcko commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104782256)
Does quoting not work?

💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104784521)
> Does quoting not work?

Not for me.

📝 marcofleon opened a pull request: "fuzz: Add target for coins database"
(https://github.com/bitcoin/bitcoin/pull/32602)
This reopens https://github.com/bitcoin/bitcoin/pull/28216.

The current `coins_view` target only tests `CCoinsViewCache` using a basic `CCoinsView` instance. The addition of the `coins_view_db` target enables testing with an actual `CCoinsViewDB` as the backend.

💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104788947)
`subprocess` uses space and tab as delimiters:https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/util/subprocess.h#L307-L334

💬 marcofleon commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#issuecomment-2904747826)
The coverage between the two targets can be compared [here](https://marcofleon.github.io/coverage/coins_view/) and [here](https://marcofleon.github.io/coverage/coins_view_db/).

Instability was a bit of a concern in the last PR, but was answered in https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2754900599.

💬 fanquake commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2904748427)
Picked up in #32602.

💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2904755024)
This approach here should also be applied to replace the `Find mt.exe tool` step in the `Windows, test cross-built` job.