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

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1837939655)
In commit "coins, refactor: Make AddFlags, SetDirty, SetFresh static" (d5e3bbf440aa948df8159999fd4eb5275c354b93)

It would make sense to rename `self` to `pair` this commit instead of in later commit afb0a3c88691cadc07fc985483f4cae1f486f6be so this code does not have to change twice and so later the change in the later commit can be move-only. Right now the later commit is tedious to review (or at least I don't know an easy way to review it) because the argument is renamed at the same time as
...

👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2429297641)
Code review ACK 9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24. Only small changes since last review like adding an Assume, changing commit messages, renaming self to pair. Thanks for the updates!

I left two suggestions which are not important and don't affect the final code, just meant to make review a little easier for the next reviewer. Feel free to ignore them or save for a later update.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1837922879)
re: https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1829401813

> Wouldn't I need to make `AddFlags` static as well in that case?

No that should not be necessary, and wouldn't suggest that. The suggestion is to make the new `SetFresh` and `SetClean` methods static and have them call `self.second.AddFlags` instead of `AddFlags` in this commit so all the new calls to these methods don't need to change again in commit d5e3bbf440aa948df8159999fd4eb5275c354b93.

👍 ryanofsky approved a pull request: "Prune mining interface"
(https://github.com/bitcoin/bitcoin/pull/31196#pullrequestreview-2429358194)
Code review ACK a67ba64f8f63a0f9f2c86d72cf7240bee7c5388c. Only changes since last review were a tweak to fix a failing test, and a commit message update.

💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1837989979)
Yeah, adding `mempool.m_opts.require_standard` to the if condition would make sense - `acceptnonstdtxn` should turn off all dust checks including this one imo

🤔 ryanofsky reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2429375066)
Updated ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba -> fe39acf88ff552bfc4a502c99774375b91824bb1 ([`pr/tcheck.4`](https://github.com/ryanofsky/bitcoin/commits/pr/tcheck.4) -> [`pr/tcheck.5`](https://github.com/ryanofsky/bitcoin/commits/pr/tcheck.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/tcheck.4..pr/tcheck.5)) with suggested changes. Thanks for the reviews and suggestions!

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1837966979)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834472950

> As you say, only the length and type are no longer checked. This PR now implements rudimentary checking of flags, width and precision.

Thanks, applied suggestion