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

💬 theStack commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104795226)
Ah interesting, wasn't aware that this was proposed already in an earlier PR. Added a link to your comment in the PR description.

💬 gmaxwell commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2904766683)
FWIW, this is solvable by changing to more fibre like methods for block transmission. Because FEC coded missing data doesn't identify what was missing.

So I had *thought* the code would check if there were too many missing transactions and just request a full block if all were missing. That doesn't address the general attack (since the attacker could admit some real txn that are in the block) but it would mostly address the blocksonly case (and could easily get a if blocksonly check so that i
...