💬 maflcko commented on pull request "Refactor: Redefine CTransaction equality to include witness data":
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-3004059285)
lgtm. Also, seems fine to remove it and add a `Equals(const CTransaction& other, const EqualsOptions = {})`, where

```cpp
struct EqualsOptions{
bool include_script_sig{true};
bool include_witness_data{true};
};
```

review ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2 🦋

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugD
...

💬 willcl-ark commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3004082028)
> > but I couldn't figure out how to do this override globally
>
> shouldn't this just be `CC=... CXX=...`, like in CI (
>
> https://github.com/bitcoin/bitcoin/blob/ad654a4807cd584be9ffcd8640f628ab40cb5170/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh#L13
> ), or is this issue `MULTIPROCESS=1 capnp` specific?

Seems to be `MULTIPROCESS=1`-specific. e.g.:

```bash
docker run -it nixos/nix

# Start a shell in container with no `gcc`/`g++` present
nix-shell --pure -E 'with
...

👍 willcl-ark approved a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32589#pullrequestreview-2957497706)
ACK 0922f6bbc33ac2abe3f3d9dc98dade896718864f

Checked all backports are clean and match their parent commits. All backports which include a `Github-Pull: #xxxxx` line in the commit message are mentioned in `release-notes.md`.

🚀 fanquake merged a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32589)

💬 willcl-ark commented on pull request "test: disable secp256 tests by default":
(https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3004247908)
I think if there is concern about configurations then I agree it would make more sense to drop the iterations to 4 or 8 as suggested by @hebasto and @real-or-random

💬 vasild commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3004262238)
> shouldn't this just be `CC=... CXX=...`

Yes, I think it should be. But is not because currently `depends/hosts/default.mk` contains:

```make
default_host_CC = $(host_toolchain)gcc
default_host_CXX = $(host_toolchain)g++
```

`gcc` and `g++` are hardcoded there. The following would make it possible to override the C and C++ compiler from the environment:

```diff
-default_host_CC = $(host_toolchain)gcc
-default_host_CXX = $(host_toolchain)g++
+default_host_CC = $(host_toolchain)
...

💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#discussion_r2166392824)
Thanks! Reworked.

💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#discussion_r2166395676)
Correct. I've just picked @sipa's [commit](https://github.com/bitcoin/bitcoin/pull/29167/commits/a51ebae75edcd33bcfd83eeae7e4ca4d6e8f74f7).

✅ fanquake closed an issue: "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install"
(https://github.com/bitcoin/bitcoin/issues/32428)

🚀 fanquake merged a pull request: "build: add root dir to CMAKE_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798)

💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3004291231)
@hodlinator

Thank you for the review. Your feedback has been addressed.

📝 fanquake opened a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32810)
Backports:
* #32798

💬 fanquake commented on pull request "build: add root dir to CMAKE_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3004302416)
Backported to 29.x in #32810.

📝 fanquake opened a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32811)
Backports:
* #32765
* #32776
* #32777

🤔 maflcko reviewed a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2957517215)
lgtm. Nice test for both branches

review ACK 8d257ed5937a47e1a60b7fe085f2e984eb60a956 👋

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
tru
...

💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166328136)

getheader -> getheaders [matches the method and plural usage elsewhere]

💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166409317)
```suggestion
with node.assert_debug_log(expected_msgs=["initial getheaders (0) to peer=0"]):
peer1 = node.add_p2p_connection(P2PInterface())
peer1.wait_for_getheaders()
```

nit: it is a bit better to check the state (and wait for it directly) than to indirectly read the debug log in a loop until a specific log line is hit.

💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166390209)
```suggestion
variable_timeout_sec = math.ceil(HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER_MS /1_000 *
seconds_since_best_header / POW_TARGET_SPACING_SEC) # ceil, to make this function usable with mocktime, which only has second precision.

return (current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE_SEC + variable_timeout_sec)
```

seems a bit odd to be rounding down in this function and then apply a random +1 offset below. So it is actually rounding up. It would be
...

💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166420099)
also, could check the getheaders `block_hash=int(node.getbestblockhash(), 16)`

💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166403740)
nit: it is probably a bit faster to `self.nodes[0].disconnect_p2ps()` when the goal is to disconnect all peers (and throw away their peer state)