💬 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

💬 pablomartin4btc commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2815995857)
`-legacy` argument shouldn't be removed from `bitcoin-wallet`/ `wallettool.cpp`? (leaving `-descriptors` `true` by default):

```
/build/bin/bitcoin-wallet -legacy -wallet="pepe" create
Topping up keypool...
Wallet info
===========
Name: pepe
Format: sqlite
Descriptors: no
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2000
Transactions: 0
Address Book: 0

```

💬 wicherfree commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#discussion_r2050990904)
Bitcoin

💬 wicherfree commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#discussion_r2050994379)
1

💬 jonatack commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2051045020)
In commit 0a6547d26b570625922478e7e993ad1c97ec3ab4 maybe reference from root (feel free to ignore)

```suggestion
You can find installation instructions in the `/doc/build-*.md` file for your platform, or self-compile
```

🤔 jonatack reviewed a pull request: "doc: Improve `dependencies.md`"
(https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2779295634)
Swung by here a couple times previously and this pull is looking better now.

ACK fefe8fc3c38701bdc3a3b1c6e4d0c8885f94e75f modulo question below

💬 jonatack commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2051041929)
In commit fefe8fc3c38701bdc3a3b1c6e4d0c8885f94e75f ISTM the CMake entry shouldn't change in this commit, apart from ordering alphabetically (why does it change here)

🚀 glozow merged a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247)

💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r2051114781)
Prior to this PR, the multisig descriptors would not insert any pubkeys int `out.pubkeys` inside of `MakeScripts`. The pubkeys that are inserted to that `FlatSigningProvider` are the ones inserted into the keypool.

💬 jonatack commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2051136269)
> Other help texts include \n, maybe do that here as well?

I'm agnostic as to that.

A few text compaction ideas, feel free to pick/choose/ignore:

s/The CPU time (user + system)/Total CPU time/

s/spent processing/processing/

s/duration. Will be omitted on platforms that do not support this or if still not measured./duration, if supported by the platform and measured./

s/Note that high CPU time is not necessarily a bad thing - new valid transactions and blocks require CPU time t
...