💬 hebasto commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182716043)
Rebased to resolve a conflict with https://github.com/bitcoin/bitcoin/pull/30193.

👋 hebasto's pull request is ready for review: "ci: Add FreeBSD GitHub Actions job"
(https://github.com/bitcoin/bitcoin/pull/30164)

💬 hebasto commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182724573)
> I am running nighly freeBSD CI runs for years, and I haven't yet seen an issue pop up, so the benefit seems unclear

Not encountering an issue in the past does not guarantee the same in the future :)

> Maybe, if CI resources are really scarce

FWIW, the new job does not use the GHA cache storage, which is limited.

💬 Sjors commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182758582)
> I am running nighly freeBSD CI runs for years, and I haven't yet seen an issue pop up, so the benefit seems unclear, albeit having the CI config could certainly be useful for manual testing sometimes.

We ran into a FreeBSD compilation error here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610209650

Might be limited to low level networking code and other places where FreeBSD is materially different from macOS and Linux. It seems good to at least do a build.

👍 vasild approved a pull request: "ci: Add FreeBSD GitHub Actions job"
(https://github.com/bitcoin/bitcoin/pull/30164#pullrequestreview-2132670813)
ACK f9f20ed001ee021a98b72e70ea18fe28f32ac6a5

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2182766170)
> I think this can be closed for now. It seems too controversial for a simple test change?

I think so. I'll try to take a look at other issues, I'd be happy if I can help.

💬 sipa commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2182768070)
I only had a very brief look, but my guess would be that it would be easier if `Sv2NetMsg` did not contain an `Sv2NetHeader`, and just stored type and message. The Sv2 Transport would then construct the header at submitting or sending time instead. This means `Sv2NetMsg` would be more of a dumb container for what higher-level code cares about, while the protocol details would be more abstracted away in the Transport.

🤔 stickies-v reviewed a pull request: "Stratum v2 Template Provider (take 3)"
(https://github.com/bitcoin/bitcoin/pull/29432#pullrequestreview-2132699185)
I'm strongly in favour of having TP support for SV2, so thank you for your hard work on this @Sjors. I am very concerned about the scope of this approach though, and therefore leaning to an Approach NACK (but open to being persuaded otherwise).

From my reading of the discussion in #27854, it seems the main technical hurdle to just having a separate `sv2tpd` daemon using `bitcoind`'s existing interfaces is `getblocktemplate`s having serious limitations as outlined [here](https://github.com/bit
...

✅ maflcko closed a pull request: "test: switch from curl to urllib for HTTP requests"
(https://github.com/bitcoin/bitcoin/pull/29970)

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2182818953)
Ok, closing for now.

You can also help with review of other open pull requests. This will teach you about the project and naturally you will find leftovers, follow-ups, or (un)related changes that you can tackle yourself.

💬 hebasto commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182835413)
> Looks good to merge to allow devs to enable this for testing in their fork. However, it should not be enabled in this repo...

Merging without enabling [won't work](https://github.com/bitcoin/bitcoin/actions/runs/9614060766):
> Error
> vmactions/freebsd-vm@v1 is not allowed to be used in bitcoin/bitcoin. Actions in this workflow must be: within a repository owned by bitcoin or matching the following: actions/cache@*, actions/cache/restore@*, actions/cache/save@*, actions/checkout@*, ilamm
...

💬 fanquake commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182839533)
> Merging without enabling won't work:

That seems like a GitHub bug. We shouldn't have to change permissions/configurations in this repository for people to do things in forks.

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1649027490)
hmm good point. I've added `garbage_len` as an optional argument in `generate_keypair_and_garbage()` so that some of the test cases which don't support 0 length garbage can avoid it.

💬 maflcko commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182856049)
> Not encountering an issue in the past does not guarantee the same in the future :)

Correct, but the cost of a trivial post-merge fixup, like adding a missing header or header-guard, in case it happens, should be trivial compared to the overhead to deal with redundant or unrelated intermittent issues, which are ignored by most devs, left to be dealt with by others (see https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue+is%3Aopen+label%3A%22CI+failed%22). If it was free to add more CI tas
...

✅ hebasto closed a pull request: "ci: Add FreeBSD GitHub Actions job"
(https://github.com/bitcoin/bitcoin/pull/30164)

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1649049643)
In any case, I don't see (or forgot) what problem is this trying to solve? It is just a display issue where the task is displayed as "skipped" vs something-else? Not sure if the CI config is the right place to provide fixes/fiddles for cosmetic stuff in forks.

💬 maflcko commented on pull request "ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2182877327)
> Merging without enabling [won't work](https://github.com/bitcoin/bitcoin/actions/runs/9614060766):

Would have been nice to merge it this way, but unfortunate that it doesn't work.

Good to have the config sitting around in this pull request, anyway. This way it can be picked up easily by anyone in the future.

📝 fanquake opened a pull request: "[26.x] upnp: fix build with miniupnpc 2.2.8 "
(https://github.com/bitcoin/bitcoin/pull/30319)
Backports https://github.com/bitcoin/bitcoin/pull/30283 to the 26.x branch.

💬 fanquake commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2182905009)
Backported to 26.x in #30319.

💬 mzumsande commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2182927906)
re-ACK 2f9bde6