💬 paplorinc commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593896834)
> I see you are a newcomer too.

this isn't necessary...

```Python
assert_greater_than("0.001", balance - newbalance)
```
I wasn't suggesting comparing it to a string, of course, but to a decimal, as in the example I've provided, i.e.:
```Python
assert_greater_than(0.001, balance - newbalance)
```

This seems to be working for me, is it failing for you?

📝 brunoerg opened a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062)
This PR adds two new fields in `getrawaddrman` RPC: "mapped_as" and "source_mapped_as". These fields are used to return the ASN (Autonomous System Number) mapped to the peer and its source. With these informations we can have a better view of the bucketing logic with ASMap specially in projects like [addrman-observer](https://github.com/0xb10c/addrman-observer).

💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2100435272)
ping @0xB10C

👍 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