⚠️ bitcoin-tools opened an issue: "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for a JSON parsing error"
(https://github.com/bitcoin/bitcoin/issues/30938)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Tested the 28.0rc2 packages from [bitcoincore.org/bin](https://bitcoincore.org/bin/bitcoin-core-28.0/test.rc2/) for Linux x86_64, Darwin x86_64, and Darwin arm64.

Seeing consistent failures in the rc2 `test_bitcoin` binary running:
- Docker with base image of Arch ([1](https://github.com/bitcoin-tools/nodebuilder/actions/runs/10958688270/job/30429843131), [2](https://github.com/bitcoin
...

💬 instagibbs commented on pull request "refactor: move `SignSignature` helpers to test utils":
(https://github.com/bitcoin/bitcoin/pull/30561#issuecomment-2363854460)
ACK 58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070

thanks for the cleanup!

💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363865740)
This can be worked around by installing python3. An alternative would be something like done here: https://github.com/bitcoin/bitcoin/pull/29868/files#r1747435819 (but that diff currently does not compile, as can be seen in the CI output)

💬 instagibbs commented on pull request "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work":
(https://github.com/bitcoin/bitcoin/pull/30918#issuecomment-2363866498)
reACK 284bd17309ab3b124d9dcddfec62f5506383343b

💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363873370)
Another alternative would be to drop 199bb09d88e28d951c5068eb65643390dbedd066 from the 28.x branch. It fixed one bug, but introduced another.

💬 instagibbs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2363876329)
> I'd suspect we probably wouldn't want that, given that the additional peers introduced here are also likely to be inbound peers (if full), so attacker controlling many of our inbound slots could stall us effectively.

If we were to go that direction, I suspect we would force the "last slot" to always be an outbound, just like the parallel compact block download attempts do now. The parallel cb attempts could instead be a bit more courteous to outbound connections in almost every case.

Jus
...

💬 instagibbs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#discussion_r1768742804)
`MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` seems like a variable we could rename to be more general and use here, especially if both mechanisms could fire

💬 bitcoin-tools commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363889298)
> worked around by installing python3

That explains why it doesn't run on certain docker base images. Is python3 considered a runtime dependency for test_bitcoin?

💬 fanquake commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363889851)
I think the bug is that commit introduced a (silent) runtime dependency that isn't actually checked/enforced in anyway.

💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363895917)
Yeah, python3 is needed for ctest, IIUC. However, test_bitcoin is probably considered stand-alone and shouldn't depend on anything other than itself.

💬 fanquake commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363896359)
Yea. Even if `ctest`/development likely requires it, the `test_bitcoin` release binary we are shipping to end users shouldn't have a (undocumented) dependency on invoking Python.

💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363906135)
The cleanest fix would probably be https://github.com/bitcoin/bitcoin/pull/29868/files#diff-4ea52ba9a5642d6946fb0016ca168b6647a8c6403d9e15e2a7197e72022824d4R87 `const std::string command{"sh -c 'echo err 1>&2 && false'"};`.

Previously `ls` was a requirement, so making it `sh` seems fine as well?

cc @hebasto

💬 maflcko commented on pull request "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work":
(https://github.com/bitcoin/bitcoin/pull/30918#issuecomment-2363907360)
review ACK 284bd17309ab3b124d9dcddfec62f5506383343b

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1768765758)
I think this line got pulled out of the `else if` conditional. Won't this set the value for all unique parents and reject any transaction with two unique prevout hashes?

✅ fanquake closed a pull request: "depends: Qt 5.15.15"
(https://github.com/bitcoin/bitcoin/pull/30774)

🚀 fanquake merged a pull request: "refactor: move `SignSignature` helpers to test utils"
(https://github.com/bitcoin/bitcoin/pull/30561)

💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768725247)
I'm not sure I understand why we need this.

Why not make them all runtime instead, why are we "testing" compile time behavior here at all, aren't we already doing that in the rest of the source code?

It seems to me the whole situation would simplify a lot if we would only test runtime behavior here.

💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768784651)
I like the idea of testing the validator together with the implementation!

But I'm not in love with the current approach.

Why are we testing `PassFmt<1>("%02d");` with `foo` - I don't think it helps with understanding how formatter works.

Also `PassFmt<1>("%s")` made sense when we were only validating the number of args, but we've extended it since, I think we should extend examples with the actual parameters, e.g.
* `PassFmt("%s", "test");`
* `PassFmt("%02d", 42);`
* `PassFmt("%12$s
...

💬 tdb3 commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1768746217)
nit: If touching the file again, looks like this could be simplified.
```diff
- bool IsVersionSelected() const { return m_details_level == 3 || m_details_level >= 4; }
+ bool IsVersionSelected() const { return m_details_level >= 3; }
```

🤔 tdb3 reviewed a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2318488868)
Left a couple comments.
Tested that using `5` does include only outbound peers by connecting two nodes locally.