💬 maflcko commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895822246)
> But I've occasionally managed to trip up one or two CI instances on e.g. memory leaks. Sometimes I can reproduce them locally with a configure change, but other configurations are trickier. For those cases I can further improve the gist to only run a subset of the jobs.

In that case it seems easier to just run the "one or two" CI tasks on your machine that would become the "self-hosted runner", instead of going through the extra step to set up GitHub CI and the self-hosted runner with GHA?

💬 maflcko commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895830004)
> > A Cirrus account is free for public open source projects, as well as persistent workers connected to it.
>
> Oh that's nice.

So it should be possible to achieve the same without touching any code here, by moving your GHA self-hosted runners to Cirrus.

Regardless, given that GHA minutes are "free", feel free to move the Cirrus self-hosted runners over to GHA-hosted runners. Not sure if this is trivially possible for all tasks, but if it is, it would make it easier to run the CI on fo
...

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1455588103)
yeah, leftover from the previous implementation. Cleaning them. Thanks!

💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895839707)
It looks like Github's "test each commit" makes an assumption that doesn't hold for forks: https://github.com/Sjors/bitcoin/actions/runs/7556559882/job/20573828392?pr=28

💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895847621)
> So it should be possible to achieve the same without touching any code here, by moving your GHA self-hosted runners to Cirrus.

I haven't looked into how self-hosted runners work with Cirrus. Adding another SAAS company into the loop seems suboptimal. And if Cirrus also reads all it configuration out of a yaml file in the repo, would it actually reduce the amount of code changes in the main repo?

💬 maflcko commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895848089)
> It looks like Github's "test each commit" makes an assumption that doesn't hold for forks: https://github.com/Sjors/bitcoin/actions/runs/7556559882/job/20573828392?pr=28

Yes, see https://github.com/Sjors/bitcoin/actions/runs/7556559882/job/20573828392?pr=28#step:4:42

💬 maflcko commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895850175)
> And if Cirrus also reads all it configuration out of a yaml file in the repo, would it actually reduce the amount of code changes in the main repo?

The workers currently use Cirrus self-hosted ones, so no changes are needed. This is documented in the cirrus yaml

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1895858869)
Updated per review, thanks @mzumsande!

Have reduced the limited peers connections window to 144 blocks (previously set to the limited peers threshold value) as it is encouraged by the [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki). And cleaned previous implementation leftovers in the test. [Code diff](https://github.com/bitcoin/bitcoin/compare/79e10351b1ce1f8db98ece67aacc24f323008b37..83e77ce1de2d4c4739bba71e4e27a4690c577375).

💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895881089)
> what alternative encoding scheme(s) the inscribers can be expected to migrate to, on a cost minimization basis, and what implications that will have on things like full node resource usage, and whether we would be happy with that.

If it had been about cost minimisation only then they would have spammed testnets (for virtually free) instead of the Mainchain.

💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895881805)
> A Cirrus account is free for public open source projects, as well as persistent workers connected to it.

> The workers currently use Cirrus self-hosted ones, so no changes are needed. This is documented in the cirrus yaml

This is a bit confusing. It's free but we use self hosting instead? Is that for performance, security some configuration issue?

I just gave Cirrus permission to run on my fork, but it complains "No public persistent worker pools found!" for all jobs. So I guess self
...

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455658183)
CompareFeerateDiagram won't return much very interesting, but you're right for the `err_string`, will return it

💬 tromp commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895893583)
> If it had been about cost minimisation then they would have spammed testnets

Their perceived value is of inscriptions on the bitcoin *main* chain of course. Not on a testnet, not on an altcoin chain, and not of an inscribed link to some other hosting site. The want their data to still be downloadable a century from now, guaranteed. Any cost minimization is conditional on that.

💬 maflcko commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895893619)
The assumptions for the runners are documented where they are configured: https://github.com/bitcoin/bitcoin/blob/master/.cirrus.yml#L16

💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895905890)
I'll try to get that to work. At first glance it doesn't seem more complicated than the Github approach. If so then I'll close this and salvage the first two commits.

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455548750)
second commit: I still don't understand why sometimes a nullptr check is done and sometimes not

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455680501)
What is the point of asserting here, when previously it wasn't (and already ran into UB)?

🤔 maflcko reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1827284733)
The assertions still seem a bit arbitrary

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455681352)
Maybe assert in the first line of this function and create a reference?

💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895913847)
> and not of an inscribed link to some other hosting site

To the contrary, they put text (link) in many cases, e.g. txid: ddcec4687acb054e94f5ad803b1f87574e93a37e560b0011b76ed8f36d6ad88a

💬 jamesob commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1895934159)
Oddly enough, this change benched out as being slightly slower (~4.8%) than the commit before it in master.

Not a huge regression, but not what I expected. Compiled with `gcc 10.2.1`.


### #29169 vs. f8ca1357 (absolute)
| bench name | x | #29169 | f8ca1357 |
| --------
...