💬 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

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960713859)
A weak counter-argument could be that if this change is included in the last release with wallet.dat-support, it would be nice to include it in the help text for people in the future going back in releases to recover their old wallets.

But the argument to remove it is growing on me.

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2667150061)
Latest push should changes `detached-sig-create.sh` to also notarize the individual binaries. Also updates signapple to latest.

💬 ryanofsky commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#issuecomment-2667168056)
> > 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.

It is just saying the fallback argument is unused. For example in:

```c++
if (gArgs.IsArgSet("-color")) {
{
const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
...
}
```

The `DEFAULT_COLOR_SETTING` argument is unused, and any fallback v
...

🤔 ryanofsky reviewed a pull request: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2625238938)
Rebased e12edf7661384f593b5868b3f3374613773019d9 -> 215f55567b2af1677f9971a0ca03e86d09fbb726 ([`pr/scripty.14`](https://github.com/ryanofsky/bitcoin/commits/pr/scripty.14) -> [`pr/scripty.15`](https://github.com/ryanofsky/bitcoin/commits/pr/scripty.15), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/scripty.14-rebase..pr/scripty.15)) due to various conflicts.

Thanks for the review! I definitely want to follow up on the HelpFn suggestion.

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1960734099)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959711651

> Similar feedback here: The default value seems like one that could have been provided by a user, so there should be no risk in setting it via `DefaultFn` and having it be the default. (With the added benefits already mentioned).

Yes, again this would need to be a manual cleanup, but this could be implemented as follows (with no change in behavior):

<details><summary>diff</summary>
<p>

```diff
--- a/src/wall
...

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1960742588)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959977366

> nit: I find the `!Value(gArgs).isNull()` with the double negation and verbosity over just `Get(gArgs)` a bit ugly and I think it would be better to remove `IsArgSet` as much as possible. In most cases it is not needed anyway, or used incorrectly. About this one, I left a comment here: https://github.com/bitcoin/bitcoin/pull/31887/files#r1959923000

Yes, the `!isNull` calls are just replacements for `IsArgSet()` and o
...

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1960725979)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959703860

> shouldn't this be using `DefaultFn<[] { return FormatOutputType(DEFAULT_ADDRESS_TYPE); }>` instead?

Yes that would probably make sense as a followup. This scripted-diff can't make that replacement, because the only default value it actually [sees being used](https://github.com/bitcoin/bitcoin/blob/43e287b3ff5f0d223b0911b6ef90054ce5eb69d2/src/wallet/wallet.cpp#L3103-L3110) is `""`, but a manual change like the follow
...

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960749255)
Fixed

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960750927)
I opted to remove because
a) such users going back in releases can check nearby historical commits (not greatest / reliable user experience I realize...)
b) less need to make changes in future for a minor doc change (get reviews, etc...)

💬 darosior commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2667296332)
> I've notarized the arm64 binaries, does running the downloaded the binaries still result in an error?

Just tried again the arm64 binary on the Mac M1. Downloading from Safari and running works fine now.

💬 yancyribbens commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#issuecomment-2667306138)
Concept ACK https://github.com/bitcoin/bitcoin/pull/31896/commits/fa3fb3c23fae287b161112b9b04cf0937a1051c6

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2667343034)
ACK bb633c9407c46eeadf6fe85e859cea1fed24473f