💬 fanquake commented on pull request "test: skip some backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#issuecomment-1460507219)
This seems arbitrary, and the `can't run this test under valgrind` message is not useful. I assume the first questions someone will have are going to be "Why can't I run _this_ test under Valgrind?", "Does it need fixing then?" etc.

💬 willcl-ark commented on pull request "doc: docment json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#discussion_r1129764196)
Personally I feel it's clearer just to use the endpoints themselves, rather than cluttering with hosts and ports.

Also, we have examples now!

💬 achow101 commented on pull request "rpc, wallet: use the same `next_index` key in `listdescriptors` and `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/26194#issuecomment-1460516035)
ACK b082f28101773e0ef0281d97025e65d0364d6f29

💬 MarcoFalke commented on pull request "test: Make the unlikely race in p2p_invalid_messages impossible?":
(https://github.com/bitcoin/bitcoin/pull/27212#issuecomment-1460519235)
> Would it make any more sense to instead add wait_for_verack=True to the initial add_p2p_connection()?

Yes, this is the default value already.

💬 MarcoFalke commented on pull request "test: Make the unlikely race in p2p_invalid_messages impossible?":
(https://github.com/bitcoin/bitcoin/pull/27212#discussion_r1129786075)
> Is the flush on the node side? Like the pong has been sent but is still sitting in nodes buffer?

Correct, it is in the node's receive buffer (from `conn`).

💬 pinheadmz commented on pull request "test: Make the unlikely race in p2p_invalid_messages impossible?":
(https://github.com/bitcoin/bitcoin/pull/27212#discussion_r1129773661)
Is the flush on the node side? Like the pong has been sent but is still sitting in nodes buffer? I'm just trying to reliably break the test on master to examine the patch

🚀 achow101 merged a pull request: "rpc, wallet: use the same `next_index` key in `listdescriptors` and `importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/26194)

👍 theStack approved a pull request: "test: Flatten miniwallet array and remove random fee in longpoll"
(https://github.com/bitcoin/bitcoin/pull/26996)
re-ACK fa0abcdafeb74aa7a312095becd55b1cee48cd99

💬 glozow commented on pull request "test: add coverage for sigop limit policy (`-bytespersigop` setting)":
(https://github.com/bitcoin/bitcoin/pull/27171#discussion_r1129803993)
Ah I see where that comes from, thanks for clarifying! Comment does look better to me now.

💬 MarcoFalke commented on pull request "test: skip some backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#issuecomment-1460524502)
I presume the reason is that the valgrind supp file is not valid for previous releases. Actually, it might not even be valid for guix-compiled binaries on master?

💬 Sjors commented on pull request "test: skip some backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#issuecomment-1460547152)
I disabled all tests that use previous releases under valgrind. Also tried a different error message.

> "Does it need fixing then?"

I added a code comment and some more context to the commit to help answer that, if anyone _does_ want to run these under valgrind.

💬 MarcoFalke commented on pull request "test: skip some backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1129815496)
nit: Might be a smaller diff, and more clear, if this was put into the `skip_if_no_previous_releases` function?

🚀 fanquake merged a pull request: "test: Flatten miniwallet array and remove random fee in longpoll"
(https://github.com/bitcoin/bitcoin/pull/26996)

💬 Sjors commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1129816790)
Thought about that, but there may other (future) tests we want to skip under valgrind (e.g. things that use a brittle dependency). Conversely, since some of tests _do_ work, maybe someone want to enable one later.

💬 fanquake commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1129816793)
Yea. If they are all going to be disabled in that case any way, no point duplicating all this logic and commentary.

💬 Sjors commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1129822563)
I was also thinking existing tests, e.g. I haven't been able to run valgrind with BDB configured (didn't try very hard).

💬 fanquake commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1129819763)
> (future) tests we want to skip under valgrind

Let's not worry about unwritten tests, that we apparently might not want to test properly for some arbitrary reason.

💬 fanquake commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1129828491)
> I was also thinking existing tests, e.g. I haven't been able to run valgrind with BDB configured (didn't try very hard).

It definitely works. The native valgrind CI job is already configured to run with BDB.

💬 pinheadmz commented on pull request "doc: docment json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#discussion_r1129850950)
If this gets merged before #27101, remind me to update to `2.0` ;-)

Technically I think `jsonrpc:"1.0"` isn't necessary and doesn't actually do anything.

💬 kristapsk commented on pull request "doc: docment json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#issuecomment-1460633707)
Nit - there is typo in commit message / PR title (s/docment/document/).