💬 SOG-LANG commented on issue "Secure Bitcoin Source":
(https://github.com/bitcoin/bitcoin/issues/32303#issuecomment-2815414509)

🤔 l0rinc requested changes to a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2778504085)
As discussed in private, I've summarized my findings here shortly.

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050588477)
I know this is meant to be mostly random, but the existing `feature` ones seem to be in alphabetic order

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050585116)
this is a quite confusing way to check whether we're in a child process or not (not obvious to me why `internal_start_stop` is not even here)

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050590886)
this line might need explanation for why no `Traceback` is a `success=False`

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050590043)
could we add these into the try so that I don't have to understand where `result` is coming from?

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050586381)
do we need to batch all 3 tests into a single test?

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050594392)
"various startup failures" doesn't sound like a single test to me, could we maybe unwrap them somehow?

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050592861)
Do we really need 3 separate internal dummy args? Seems like a framework with a custom arg and 3 separate tests with different behaviors checking only a single thing would untangle this slightly better

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050589262)
could these be separate sub-tests instead of just test logs?

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050594684)
I find the `relaunches itself` part quite confusing here

✅ fanquake closed an issue: "Secure Bitcoin Source"
(https://github.com/bitcoin/bitcoin/issues/32303)

:lock: fanquake locked an issue: "Secure Bitcoin Source"
(https://github.com/bitcoin/bitcoin/issues/32303)

👋 l0rinc's pull request is ready for review: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296)

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050626830)
> this is a quite confusing way to check whether we're in a child process or not (not obvious to me why `internal_start_stop` is not even here)

Do you have a suggestion to make it less confusing? I feel like checking for an internal argument is a mostly straightforward way of checking whether this was called internally.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050631756)
I've realized that all child test processes fail before reaching `run_test()`, so I'm leaning toward changing this into an `assert`.

🤔 pablomartin4btc reviewed a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2778688281)
re-ACK 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a

Since my last review:
- commit "test: rpcs disabled for descriptor wallets were removed" was moved to #28710
- addressed feedback from @rkrux

👍 rkrux approved a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2778686856)
tACK e261eb8d50c7192260a449e653453e63f59dbeed

Thanks for addressing the comments. I have checked the test vectors as well, all of them from the BIP are there.

```
git range-diff 8e08b33...e261eb8
```

💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2050692514)
Now that the error is more detailed; the test passes because `assert_raises_rpc_error` checks for substring.
But I can pick this up in a follow up.

```diff
- Input musig2 pubnonce key is not expected size
+ Input musig2 pubnonce key is not expected size of 67 or 99 bytes
```

💬 suiyuan1314 commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2815665900)
> > My preference would be to either remove it or keep it short
>
> Same, suggest removing the section completely. It's very handhold-y, and the few people who might need to read this information are probably the least likely to do so.

Removed this section now :)