🤔 ryanofsky reviewed a pull request: "test, refactor: Add TestNode.binaries to hold binary paths"
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2624615771)
Thanks for the review! Updated with some simplifications and cleanups based on your suggestions.

Updated bfadc7d241a6288e70fb0c58b6ad5fa5a305af51 -> 11615b1025a09e62f12224ae55c92fe85c42b24c ([`pr/wrapp.1`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.1) -> [`pr/wrapp.2`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrapp.1..pr/wrapp.2)) adding get_binaries method, renaming some variables, and avoiding `getattr`.

...

💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960517840)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959430288

> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): I'd say it would be clearer to not have `self.paths` at all, and also remove the need to somehow import and handle `Binaries` in individual tests.

That's a good idea. Added a `TestFramework.get_binaries()` method to make this more convenient. There are only 3 tests that use it, but it is also convenient to use inter
...

💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960373605)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959328055

> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): Could name this `cli_argv` to keep inline with the name of the exe and to avoid confusion with the existing `TestNode::rpc` and `TestNode::cli` naming?

Followup commit 60b89ab4897e175c8d73a5dd21cd52a0c0c790ef from #31375 might provide context because these names just match names of `bitcoin` subcommands.

To answe
...

💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960332653)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959395105

> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): Is there a reason to use types, when a simple dict seems sufficient to store key-value pairs?

Imo, plain objects are simpler than dicts, more readable (`o.prop` vs `d["prop"]`) and more compatible with useful features like static type checking.

Right now the `paths` object is just a collection of paths to executa
...

💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2666883972)
Updated 11615b1025a09e62f12224ae55c92fe85c42b24c -> 8f5228d9239464ece06f3a56372aeec29685801c ([`pr/wrapp.2`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.2) -> [`pr/wrapp.3`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrapp.2..pr/wrapp.3)) to fix lint errors

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1960567077)
It might still do something; for example removals may be applied still, and grouping may be calculated. I feel it's unnecessary to elaborate that much about implementation details in the interface, though.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960567794)
Maybe I'm misunderstanding, but I don't think we can use `RemoveIterAt` (which is for updating the peer list) for this (which is the global `TxOrphanage::m_orphan_list`)?

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960570915)
Oh of course; it just looked very similar.

💬 s373nZ commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2666905054)
> I don't understand the motivation for setting `CMAKE_<lang>_COMPILER_LAUNCHER` from inside the project.

I'm also ignorantly curious about this. Is it primarily a requirement from the feature parity list with the prior `autotools`?

https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960581755)
thanks, done

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960582140)
good point, I've made it a `TxOrphanage` method now

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960582312)
yes, moved it up

💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666920246)
> If it's just (very) slow, that may be due to low I/O speed of your disk

Disagree. The alleged cause is not supported by the data:
%iowait = 30.51%
kB_read = 21 624 950
kB_wrtn = 10 959 841
Only 31 blocks (~120 MB) downloaded during the time (c.a. 90 minutes).

💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666927835)
> Does it happen on a different filesystem than exfat? Does it happen on a different filesystem hardware, maybe something that is faster? (Can you benchmark the hardware?)

Yes, the same occurs in the same environment except NTFS instead of exFAT and HDD instead of SD.

👍 ryanofsky approved a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2625043669)
Code review ACK fa3fb3c23fae287b161112b9b04cf0937a1051c6. Nice refactoring to get rid of pointless complexity and repetition in this code.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960600995)
Updated the commit message.

It's not a problem before, and not a problem now, because all of the things `SignPSBTInput` would fail on are checked by its callers.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960601469)
Changed `RemoveUnnecessaryTransactions` to take sighash type as optional in this commit as well.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960601610)
Done

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960602180)
This is all explained by the comment 2 lines up.

Took the suggestion, changed the comment to be less repetitive of things already said.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960602709)
This is already explained by the comment a few lines above.

Added a less repetitive comment.