💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182672557)
> I will increase the size and see if it helps

Yes. Especially, considering too many cleanups.

💬 brunoerg commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2182683989)
I haven't looked at the code yet but I just tested it with two asmap files a few months apart and with 25k addresses, looks fine:
```
78 address(es) reassigned from unassigned to AS51167
47 address(es) reassigned from AS198949 to AS15557
38 address(es) reassigned from AS45758 to AS45629
18 address(es) reassigned from AS12715 to AS12479
15 address(es) reassigned from AS57269 to AS8708
12 address(es) reassigned from AS174 to AS212238
9 address(es) reassigned from AS41378 to AS60024
7 addr
...

💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182698553)
> I will increase the size and see if it helps

From my tests it follows that the `ccache` cache size is about 227 MB. So setting `CCACHE_MAXSIZE=250MB` or `CCACHE_MAXSIZE=300MB` should work.

🚀 fanquake merged a pull request: "doc: clarify Cirrus self-hosted workers setup"
(https://github.com/bitcoin/bitcoin/pull/30314)

💬 Sjors commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2182707177)
tACK 545bb6c96080

I looked at the diff between the packaged 2.2.7 and 2.2.8. There's a `MINIUPNPC_API_VERSION` bump from 17 to 18, which was addressed in #30283. I also see our dropped patches reflected in the diff. Didn't study the rest of the changes.

Tested on macOS 14.5 with `-upnp=1 -natpmp=0`.

When I turn UPnP support off in the router:

```
[mapport] No valid UPnP IGDs found
```

When I turn it on:

```
2024-06-21T12:55:33Z [mapport] UPnP: ExternalIPAddress = x.x.x.x
20
...

👋 Sjors's pull request is ready for review: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274)

💬 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
...