👍 rkrux approved a pull request: "test: use sleepy wait-for-log in reindex readonly"
(https://github.com/bitcoin/bitcoin/pull/30006#pullrequestreview-2045542646)
ACK [fd6a7d3](https://github.com/bitcoin/bitcoin/pull/30006/commits/fd6a7d3a13d89d74e161095b0e9bd3570210a40c)

Make and tests successful.
I found this answer helpful that explains how `yield` and `with` interact with each other: https://stackoverflow.com/a/35489897/12218815

💬 hebasto commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2100521158)
https://github.com/bitcoin/bitcoin/pull/29961#discussion_r1582021758:

> The `Popen::poll()` function is used in #29868. I hope that an improved/fixed Windows implementation of the `Popen::retcode()`, which I don't have now, will allow to drop it.

That is no longer the case with [current](https://github.com/bitcoin/bitcoin/commit/b69b9e85e98cac2c7585c9b613185dc6c80320cc) implementation of #29868.

💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593996196)
Oh, I see.

Still, better to leave to [preserve potential bugs](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring) since this is refactoring.

💬 sipa commented on issue "libxcb-xinerama0 Library required by bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2100544625)
@laanwj Does #29923 address this?

💬 kanzure commented on issue "Consideration of a Code of Conduct and/or written rules for moderation":
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2100549946)
Here is a "moderation guidelines" document that was created 5 days ago apparently: https://github.com/bitcoin-core/meta/blob/main/MODERATION-GUIDELINES.md as it seems directly related to the previous discussion in this issue, I thought I would bring it up. All previous comments here, I believe, still apply.

In https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-1980365499 @ajtowns proposes a "Moderation Guidelines" document. I like the idea of writing about goals, explaining the avai
...

💬 fanquake commented on issue "libxcb-xinerama0 Library required by bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2100550793)
> @laanwj Does #29923 address this?

No. That just removes our need to compile all the libs. Everything in Qt is still loaded at runtime.

👍 rkrux approved a pull request: "test: Assumeutxo: import snapshot in a node with a divergent chain"
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2045600755)
tACK [ce80f7e](https://github.com/bitcoin/bitcoin/pull/29996/commits/ce80f7ec9a66d594c3a6a6e249f911e9e543a623)

Make and tests successful, thanks for adding this test - it's easy to follow through.

💬 rkrux commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1593998574)
Nit: Because this test involves dealing with multiple nodes, for verbosity and clarity we can call this variable as `second_node`.

💬 rkrux commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1594000248)
`nblocks=99`
We can make this value dependent on SNAPSHOT_BASE_HEIGHT such as (SNAPSHOT_BASE_HEIGHT - 200) or (SNAPSHOT_BASE_HEIGHT / 2) so that it's tied to SNAPSHOT_BASE_HEIGHT, and any future changes will automatically account for this variable as well.

💬 rkrux commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1594014627)
`-32603`

Although not necessary for this PR, I am wondering how big of a lift would it be to tie these errors to the ones defined here: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/protocol.h

💬 rkrux commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1594008624)
Got to know from the CPP code that `-reindex-chainstate` will load up from the blk.dat files. That's why I understand only 2 more blocks are needed to exceed the work of the snapshot chain.

📝 hebasto opened a pull request: "build, test: Remove unused `TIMEOUT` environment variable"
(https://github.com/bitcoin/bitcoin/pull/30063)
Setting the `TIMEOUT` environment variable has been a noop in both cases since its introduction.

It seems to have been inadvertently copy-pasted from existing code. For example, in commit d80e3cbece857b293a4903ef49c4d543bb2cfb7f, it was needlessly copied from a valid case a few line above for the `qa/pull-tester/run-bitcoind-for-test.sh` script.

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1593964758)
Here the caught `UniValue&` is not explicitly `std::move`d. But on line 272 it is. Is that intentional?

🤔 cbergqvist reviewed a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2045544882)
reACK 8583c2f93d4eb020bf1e3b74860b691ec12e5eee

Ran most functional tests including `interface_rpc.py` without any failures. Ran `make check` without failures or skips.

Happy that the `version` parameters to `JSONRPCReplyObj()` etc are now explicit.

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594034591)
Might be more correct & compatible to append `"\r\n"`. See https://stackoverflow.com/questions/5757290/http-header-line-break-style

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594082052)
That's a reasonable comment but I kept the single character which is how it worked on master (see below). I see a few examples of `\r\n` in the codebase but its rare and I'm not sure how it's decided.

https://github.com/bitcoin/bitcoin/blob/63d0b930f821132badd804822a46232a5f98bbef/src/rpc/request.cpp#L52-L56

💬 maflcko commented on pull request "build, test: Remove unused `TIMEOUT` environment variable":
(https://github.com/bitcoin/bitcoin/pull/30063#issuecomment-2100654472)
utACK 189d0da3f6f561c808fdd9fbd4dfd34ccfa23fe1

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594099660)
Good catch thanks. I'll need @ryanofsky to weigh in here because I keep misunderstanding c++ move semantics. But here's what I think is happening:

In `JSONRPCReplyObj()` in `request.cpp` the really import move-instead-of-copy is when we call `reply.pushKV(...)`: https://github.com/pinheadmz/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/rpc/request.cpp#L64

That makes me think that the `std::move()` currently in `JSONErrorReply()` in `httprpc.cpp` is unnecessary: https://github.
...

💬 sipa commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-2100691492)
The discussion here led to #28824 being opened, where more discussion happened, and the idea evolved further. Having re-read things there, I have a concrete proposal that perhaps goes a bit further, so I'm posting it here to keep PR discussion about the implementation.

The goal is addressing ambiguity and readability, but not necessarily compatibility (because being compatibility with something broken is silly).

### Decoding rules

* If the asm string in its entirety is an **even length
...

💬 sipa commented on pull request "Fix ASM ambiguity":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-2100693203)
Attempt to revive the discussion: https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-2100691492