💬 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 :)

💬 jonatack commented on pull request "docs: remove passing CI section in guidelines for PR merging as it's common sense":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2815704455)
ACK 17a1630f226e1c8eb13eb7c7425290e933ad9368

"Make sure pull requests pass CI before merging." -> Indeed, this line seems pointless (maintainers are aware), ACK on removing it.

📝 instagibbs opened a pull request: "test: test MAX_SCRIPT_SIZE for block validity"
(https://github.com/bitcoin/bitcoin/pull/32304)
I don't believe there are direct tests for this.

🤔 ryanofsky reviewed a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2779056673)
Updated df40f46571bc7b8f0a85a437130e99bad5b0de63 -> 538521a37c3788ce2a0ba1bda76844a25cfd3e06 ([`pr/ipc-cli.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.2) -> [`pr/ipc-cli.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-cli.2..pr/ipc-cli.3)) handling -rpcconnect option, adding a test, making various cleanups, adding comments, and fixing CI failures in non-ipc builds

💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2050896405)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2049097617

> Passing `-rpcconnect` and `-ipcconnect` at the same time should probably result in an error. (at least, the logic of trying IPC first then RPC if that fails is a bit confusing to me, as they're completely different protocols)

Thanks, this makes sense and also makes sense to skip IPC if -rpcconnect is specified. Both these changes are implemented now

💬 Sjors commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#issuecomment-2815888994)
If they don't exist yet, can you also add a test to show `MAX_SCRIPT_SIZE` doesn't "apply" to `OP_RETURN`?

💬 instagibbs commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#issuecomment-2815920324)
@Sjors can you detail what the test would be? OP_RETURNs are already not entered into the utxo set at any length

👋 ryanofsky's pull request is ready for review: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297)

💬 sipa commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2050956877)
Nitty nit: I think both the original and the suggestion are wrong.

"The compressed aggregate public key ~for~ which this pubnonce is for."

💬 stickies-v commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2815963984)
re-ACK 65fcfbb2b38bef20a58daa6c828c51890180611d - no changes except addressing style nits and adding missing includes.

nit: some of the new `util/transaction_identifier.h` includes, such as in `interfaces/wallet.h` could have been a forward declaration instead to minize compile time