📝 Talej opened a pull request: "doc: corrected listunspent rpc quoting"
(https://github.com/bitcoin/bitcoin/pull/31275)
bitcoin-cli help lockunspent RPC example shows:

```
As a JSON-RPC call
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "lockunspent", "params": [false, "[{\"txid\":\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\",\"vout\":1}]"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
```
Including quotes around the transactions argument. Using the RPC in this way fails:

```
curl --data-binary '{"jsonrpc": "1.0", "id": "curltest
...

👋 Talej's pull request is ready for review: "doc: corrected listunspent rpc quoting"
(https://github.com/bitcoin/bitcoin/pull/31275)

👍 rkrux approved a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2428766522)
tACK 83fab3212c91d91fc5502f940c901a07772ff747

Make and functional tests successful.

Thanks for ensuring coverage for this RPC is not lost.

💬 rkrux commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#discussion_r1837603923)
Lacking before this change as well, can we add a comment here just like there is one in all the code blocks above and below?

`sign the rawtx with nsigs-1 signatures, then sign with the last sig, and then combine to create fully signed tx`

💬 rkrux commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#discussion_r1837609543)
For readability
```suggestion
full_signed_tx = node2.combinerawtransaction([rawtx2["hex"], rawtx3["hex"]])
```

💬 rkrux commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#discussion_r1837607106)
Although not a new change and not incorrect as well, but I noted that the last signature is from the `priv_keys[-1]`, and not `priv_keys[nsigs]`. Maybe the author intended to test signatures from non-consecutive keys as well.

🤔 hodlinator reviewed a pull request: "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2429009065)
Reviewed 57caa965b5ae284e501f892415d60fcb536f4c0e

`git range-diff master a123297 57caa96`

- Applied most of my nits that were still valid.

I understand you were burned by endianness but I disagree that it's worth sacrificing readability where endianness is a non-issue.

For cases where endianness is a concern, I suggest the following pattern:
```C++
uint64_t xor_key;
constexpr std::array<uint8_t, sizeof xor_key> xor_pat{std::to_array<uint8_t>({0xff, 0x00, 0xff, 0x00, 0xff, 0x00, 0x
...

🚀 fanquake merged a pull request: "doc: correct typos"
(https://github.com/bitcoin/bitcoin/pull/31271)

💬 fanquake commented on pull request "doc: corrected lockunspent rpc quoting":
(https://github.com/bitcoin/bitcoin/pull/31275#issuecomment-2470082122)
The same kind of quoting is used in multiple places in this file, so if it is incorrect / needs updating, all instances should be changed at the same time.

💬 fanquake commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2470110678)
I haven't really looked at the changes here, but just looking at the diff (+96'000 lines), my feedback would be that you'll need to change approach in regards to making your data available (if the intent is to have that included), as I doubt we'll be adding 96'000 lines to `bench/xor.cpp`. You could look at how we generate a header from bench/data/block413567.raw for a different approach, as including a small binary blob, and parsing it into a header at compile time is far more palatable.

💬 naumenkogs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2470187172)
ACK 0ca1e05d8bcbc42892156c15f128a5f829e9e48e

Spent a fair amount of time staring at `ConsensusScriptChecks` behavior change mentioned in the original post. It seems alright. Handling various limits is probably the most complex part of this code change.

Otherwise the code seems clear and nothing is broken. Still hard for me to reason about high-level design decisions on cluster mempool here, but I learned a lot while reviewing this pr at least.

👍 rkrux approved a pull request: "test: report detailed msg during utf8 response decoding error"
(https://github.com/bitcoin/bitcoin/pull/31251#pullrequestreview-2429240442)
tACK a2c45ae5480a2ee643665d6ecaee9714a287a70e

Certainly an improvement in understanding failures. I applied the suggested patch and compiled core with BDB wallet. Following is the trace at my end:

```
2024-11-12T10:15:36.800000Z TestFramework (INFO): Test migration of a basic keys only wallet without balance
2024-11-12T10:15:38.407000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/authproxy.p
...

💬 rkrux commented on pull request "test: report detailed msg during utf8 response decoding error":
(https://github.com/bitcoin/bitcoin/pull/31251#discussion_r1837885827)
A nit but it's easier to read as well, catches eyes quickly.

```suggestion
'code': -342, 'message': f'Cannot decode response in UTF-8 format, content: {data}, exception: {e}'})
```

💬 rkrux commented on pull request "test: report detailed msg during utf8 response decoding error":
(https://github.com/bitcoin/bitcoin/pull/31251#discussion_r1837891200)
`content: {data}`

Out of curiosity: Do you think we should limit printing the data here to a certain length or printing all of it is fine as well? Idk atm if the RPC responses from core have a size limit.

🚀 fanquake merged a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249)

💬 Sjors commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2470244740)
@m3dwards thanks! I'll give that a try. Any idea what was wrong with the syntax I used?

👋 Sjors's pull request is ready for review: "ci: skip Github CI on branch pushes for forks"
(https://github.com/bitcoin/bitcoin/pull/30487)

💬 Sjors commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2470265299)
That worked! See https://github.com/Sjors/bitcoin/pull/70/checks where the "push" actions are skipped.

📝 fanquake opened a pull request: "guix: scope pkg-config to Linux only"
(https://github.com/bitcoin/bitcoin/pull/31276)
After #31181, `pkg-config` is no-longer needed for macOS or Windows Guix builds. It's still needed for Linux, as it's used by a Qt subdependency (fontconfig to find freetype). However we should also no-longer need it for Qt itself, when building using depends.

💬 Sjors commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-2470288243)
I ended up working around the issue by downloading the file elsewhere:

```sh
guix download https://www.gnupg.org/ftp/gcrypt/gnutls/v3.8/gnutls-3.8.1.tar.xz
```

After that `guix build` is happy.

My guess is that `.xy` files have been purged from the internet left and right. Not sure if this is worth filing an upstream issue.