💬 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

💬 achow101 commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2667345072)
ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2

💬 achow101 commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2667347029)
ACK 09b150bb8adae00854f02ece69fc6ef222fb07d9

🚀 achow101 merged a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156)

💬 achow101 commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2667356792)
Since guix builds from this tarball, how is it that the guix build results in binaries the output the correct version?

🚀 achow101 merged a pull request: "wallet: abandon orphan coinbase txs, and their descendants, during startup"
(https://github.com/bitcoin/bitcoin/pull/31794)

✅ achow101 closed an issue: "GetRandBytes() Hangs on Samsung Galaxy S25 and OnePlus 13"
(https://github.com/bitcoin/bitcoin/issues/31817)

🚀 achow101 merged a pull request: "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop"
(https://github.com/bitcoin/bitcoin/pull/31826)

💬 davidgumberg commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2667434518)
> > [bitcoin-096525e92cc2-arm64-apple-darwin.zip](https://github.com/user-attachments/files/18781660/bitcoin-096525e92cc2-arm64-apple-darwin.zip)
>
> Tested this on a Mac M1. I could download it flawlessly through Firefox and perform most of IBD.
>
> I tried downloading it through Safari and likewise other reviewers i encountered an error:
>
> The system language was set to French but i guess what matters here is the error ID: -47.

I don't know if this had the same cause as what you
...