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

💬 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_r1960605964)
Fixed and reworded the comment.

`SIGHASH_ANYONECANPAY` does not commit to any input information, so none of the protections that allow us to drop the `non_witness_utxo` apply.

💬 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_r1960606092)
Fixed

👍 ryanofsky approved a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2625099756)
Code review ACK 3252f3bab02605129a568112f650ef3cb29703b7. All changes look good. No real opinion on the third commit, seems fine to keep or drop

💬 ryanofsky commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960634114)
re: https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1959923000

Mostly agree with this suggestion. It would be nice to remove this `IsArgSet` call (in a separate commit or PR), because like [many](https://github.com/bitcoin/bitcoin/pull/30529) [other](https://github.com/bitcoin/bitcoin/pull/31212) `IsArgSet` calls, this one introduces a bug and doesn't handle negation properly. A common sense interpretation of `-nogetinfo` would be to run the getinfo command, not to run it.

Only p
...

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960644463)
@Sjors , does `tip_changed` ever become `true` at line 956 and we return from there? I think it will always be false hence can be deleted.

1. When we first enter this scope, the predicate is executed before blocking. If `tip_changed` is updated to `true`, it is returned. And when the predicate returned value is `true`, we do not wait; instead, we proceed to build and return a block template.

2. In subsequent executions of the predicate `tip_changed` will be `false` and we will check if the
...

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960455380)
style nit:
Break line to be more readable according to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960648041)
We can generate a template before a tick

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960653518)
re: https://github.com/bitcoin/bitcoin/pull/31283/files#r1960644463

Agree it looks like this line can be deleted. Also scope of `tip_changed` can be reduced so it is inside the do..while loop.

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2667030347)
@l0rinc agreed, I'll drop it. However, I appreciate the more in depth explanation since this helps me know for the future so I don't make the mistakes. And yes, I agree that the comment `This refactor is a no-brainer` in retrospect was poor wording and I could have done better overall in a few ways. Anyway, thanks again for the detailed feedback.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1960663432)
It was a leftover from a previous implementation. But see https://github.com/bitcoin/bitcoin/pull/27469.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1960664340)
Ended up changing the testing approach, preferring functional over unit tests. But thanks.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2667038911)
Updated 6f0405177f89377ed272bae42becec1c75b8943c -> 185c6a416df679ddc7b38750aa4d181c0b043d5b ([`pr/wrap.21`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.21) -> [`pr/wrap.22`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.22), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.21..pr/wrap.22)) just incorporating some python cleanups from #31866

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960668271)
> replace set<NodeId> announcers in OrphanTxBase with a std::map<NodeId, size_t> announcers, where the value is the orphan's position in the PeerOrphanInfo::m_iter_list

Went ahead with this solution.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#issuecomment-2667045493)
Ended up changing the testing approach, opting for a functional test over a unit test due to its reduced maintenance costs across code refactorings. Made a few modifications to mzumzande's #31824 functional tests.

💬 yancyribbens commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#issuecomment-2667074129)
> It is confusing, because the provided fallback is dead code. So it would be better to just call GetArg without a fallback.

I'm not 100% sure when reading the commit what is meant by dead code. Is it that the arg `-blockmintxfee` is a no longer used switch here? I ask because I notice some other calls to GetArg with fallbacks such as:

```
`src/init.cpp:879: if (args.IsArgSet("-upnp")) {`
``

💬 yancyribbens commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1960687514)
Is this TODO still accurate? Not what arguments need Harmonizing.

💬 yancyribbens commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#issuecomment-2667078691)
Looks like a worthwhile refactor. Better than an LLM could do ;)

💬 yancyribbens commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960692073)
> It would be nice to remove this IsArgSet call (in a separate commit or PR)

Looks like that was followed up on already? https://github.com/bitcoin/bitcoin/pull/31896