🤔 kevkevinpal reviewed a pull request: "test: Run framework unit tests in parallel"
(https://github.com/bitcoin/bitcoin/pull/29771#pullrequestreview-2019885446)
Approach ACK f19f0a2e5af6c2a64900f1f229e21b6f1668bd3d

I still need to run this on my local machine

💬 kevkevinpal commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1577898651)
is there any reason we picked this location in `BASE_SCRIPTS`, I would think we would want to run `TEST_FRAMEWORK_UNIT_TESTS` earlier on even if they are running in parallel

since if the unit tests fail other tests will probably fail as well.

I think making it the first script in `BASE_SCRIPTS` would make sense to me and then making an exception in regards to run times since other tests deepened on this.

👍 ryanofsky approved a pull request: "validation: improve performance of CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/28339#pullrequestreview-2019819063)
Code review ACK 3c3895ccfa108e10f4a0b1a78f64ffedade1fd11

Left another suggestion, but this looks good in its current form.

💬 ryanofsky commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1577858964)
In commit "validation: improve performance of CheckBlockIndex" (7183bc90483cc9361cfffb157b91fd4245ab77e0)

This might be a little simpler and clearer since it consolidates the best header chain cases:

```diff
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5292,15 +5292,14 @@ void ChainstateManager::CheckBlockIndex()
pindex = range.first->second;
nHeight++;
continue;
- } else if (best_hdr_chain.Contains(pindex) && nHeight < best_hdr
...

💬 ryanofsky commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1577887301)
In commit "validation: improve performance of CheckBlockIndex" (7183bc90483cc9361cfffb157b91fd4245ab77e0)

Note: this assert is a little different than what I suggested https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1569423672 because it is not enforcing that if the parent block is the best header, then pindex must be null. So the statement that there will not be a best block if the parent block is the best header is not checked by the assert.

But I think this is fine. The commen
...

💬 maflcko commented on pull request "ci: disable `_FORTIFY_SOURCE` with MSAN":
(https://github.com/bitcoin/bitcoin/pull/29837#issuecomment-2075003021)
lgtm ACK 08ff17d1420a3d1c14c6b1a5436678fbb1dd9cbc

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1577936862)
I don't think that is much simpler. Also I think this is easier to delete if/when we don't try orphans from different peers in the future.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1577947487)
Ok, marking this as resolved

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1577949702)
Added a similar test

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1577950806)
Added

💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2075053593)
> > Resolved linter warnings.
>
> Can you mention this in the PR description?

Thanks! The PR description has been updated.

👍 instagibbs approved a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2020006914)
ACK https://github.com/bitcoin/bitcoin/pull/28970/commits/30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

Changes are test only, and new subtest causes a debug-build crash when the `PCKG_POLICY` change is reverted.

💬 iotamega commented on issue "Crashes in v27 on Windows 10 and 11":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2075063698)
Hate to be the deliverer of bad news but I am also experiencing this issue on my Ubuntu installations after extensive testing.

💬 maflcko commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2075073807)
On Ubuntu it should be trivial to run in gdb, can you try that please?

👍 TheCharlatan approved a pull request: "refactor: Rename `subprocess.hpp` to follow our header name conventions"
(https://github.com/bitcoin/bitcoin/pull/29910#pullrequestreview-2020030582)
ACK 08f756bd370c3e100b186c7e24bef6a033575b29

📝 hebasto opened a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953)
On FreeBSD 14.0, in the `depends` directory:

- without `bash`:
```
$ gmake print-bdb_build_id_long
env: bash: No such file or directory
env: bash: No such file or directory
bdb_build_id_long=bdb-4.8.30-4b0c6f8e95251b9c6731844fc34111c04b75fd9f15c671d6e34f2a4d014ec1be-release
$ gmake print-final_build_id
env: bash: No such file or directory
env: bash: No such file or directory
final_build_id=722b2d3e264
```

- with `bash`:
```
$ gmake print-bdb_build_id_long
bdb_build_id_long=
...

💬 hebasto commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#issuecomment-2075088822)
cc @vasild @fanquake

💬 fanquake commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#issuecomment-2075094287)
This should be added to the depends docs.

💬 hebasto commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#issuecomment-2075104778)
> This should be added to the depends docs.

Thanks! Added.

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578006194)
Hm, ok, that's another behavior change I didn't anticipate. Let me first describe my understanding of happens: In [the libevent version](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3492) each of the two characters following the `%` are [looked at individually if they are valid hex characters](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3508). The space is not so the if fails and the fallback case takes
...