💬 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
...

💬 jonatack commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2816220345)
Latest changes since my last review LGTM. I plan to review the code more closely.

Empirically, running a node on this branch, it looks like over time most peers settle into less than 0.5 per milles, only a few are higher, with maybe one peer around 4.

📝 theStack opened a pull request: "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)"
(https://github.com/bitcoin/bitcoin/pull/32305)
This PR is a follow-up to #31247 (see https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the `decodepsbt` RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.