💬 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

💬 yancyribbens commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960699600)
> The upper bound check is no longer needed after the merge of PR 21192.

Nit, looks like the bound check is for upper and lower bounds (above zero and bellow 5).

💬 yancyribbens commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2667106119)
Concept Ack https://github.com/bitcoin/bitcoin/commit/3252f3bab02605129a568112f650ef3cb29703b7