💬 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_r1455426826)
can you be more explicit in suggestion? The errors are being returned?
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455426826)
can you be more explicit in suggestion? The errors are being returned?
💬 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_r1455433932)
this is testing that subtraction works correctly, not that an individual chunk can have negative size
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455433932)
this is testing that subtraction works correctly, not that an individual chunk can have negative size
📝 stickies-v opened a pull request: "refactor: remove CTxMemPool::queryHashes()"
(https://github.com/bitcoin/bitcoin/pull/29260)
`CTxMemPool::queryHashes()` is only used in `MempoolToJSON()`, where it can just as easily be replaced with the more general `CTxMemPool::entryAll()`. No behaviour change, just cleans up the code.
(https://github.com/bitcoin/bitcoin/pull/29260)
`CTxMemPool::queryHashes()` is only used in `MempoolToJSON()`, where it can just as easily be replaced with the more general `CTxMemPool::entryAll()`. No behaviour change, just cleans up the code.
🤔 furszy reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1827261571)
> If I understand it correctly, the logic deciding whether to try and accept outbound connections, using `HasAllDesirableServiceFlags()`, relies on `IsInitialBlockDownload()` / `DEFAULT_MAX_TIP_AGE` (24h) on master, but is changed to rely on `NODE_NETWORK_LIMITED_MIN_BLOCKS` (288 blocks, ~48h) in this PR.
>
> I think this means that the safety buffer of 144 blocks proposed in [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki) would have been removed. I'm not sure about th
...
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1827261571)
> If I understand it correctly, the logic deciding whether to try and accept outbound connections, using `HasAllDesirableServiceFlags()`, relies on `IsInitialBlockDownload()` / `DEFAULT_MAX_TIP_AGE` (24h) on master, but is changed to rely on `NODE_NETWORK_LIMITED_MIN_BLOCKS` (288 blocks, ~48h) in this PR.
>
> I think this means that the safety buffer of 144 blocks proposed in [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki) would have been removed. I'm not sure about th
...
💬 ismaelsadeeq 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_r1455527026)
I meant what is being returned by `err_string` and `CompareFeerateDiagram` was not used. why were you unable to compute mining score, same for `CompareFeerateDiagram` why is the fee rate insufficient.
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455527026)
I meant what is being returned by `err_string` and `CompareFeerateDiagram` was not used. why were you unable to compute mining score, same for `CompareFeerateDiagram` why is the fee rate insufficient.
💬 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_r1455534078)
also, `Subtrack` isn't a word...
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455534078)
also, `Subtrack` isn't a word...
💬 ismaelsadeeq 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_r1455539143)
Yeah, maybe to be consistent define the labels and do the same in the `OLD` `chunk` computation loop above.
Though the explicit comments in the `OLD` `chunk` computation loop is more easier to parse for me :).
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455539143)
Yeah, maybe to be consistent define the labels and do the same in the `OLD` `chunk` computation loop above.
Though the explicit comments in the `OLD` `chunk` computation loop is more easier to parse for me :).
💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#discussion_r1455543617)
Hours and hours of headache :-)
Because I run four of those as different users, all builds would silently fail for the other users because they didn't have write permission on `/tmp/env`.
(https://github.com/bitcoin/bitcoin/pull/29259#discussion_r1455543617)
Hours and hours of headache :-)
Because I run four of those as different users, all builds would silently fail for the other users because they didn't have write permission on `/tmp/env`.
💬 ismaelsadeeq 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_r1455544265)
Yes, My question was... whats the rationale for `FeeFrac` to be generally have negative size ?
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455544265)
Yes, My question was... whats the rationale for `FeeFrac` to be generally have negative size ?
💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895813220)
> A Cirrus account is free for public open source projects, as well as persistent workers connected to it.
Oh that's nice.
> GitHub "free" minutes should also be unlimited for public open source, no?
Ah, that could explain why my minutes aren't decreasing in Github settings despite having done a few CI runs. If that's documented somewhere then I'll drop a3b5b87a6a366579d6aa286956462dfadc6a13d9.
---
>> Related: I wrote a [gist](https://gist.github.com/Sjors/cd55370fa67aa095df90c433
...
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895813220)
> A Cirrus account is free for public open source projects, as well as persistent workers connected to it.
Oh that's nice.
> GitHub "free" minutes should also be unlimited for public open source, no?
Ah, that could explain why my minutes aren't decreasing in Github settings despite having done a few CI runs. If that's documented somewhere then I'll drop a3b5b87a6a366579d6aa286956462dfadc6a13d9.
---
>> Related: I wrote a [gist](https://gist.github.com/Sjors/cd55370fa67aa095df90c433
...
💬 maflcko commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#discussion_r1455566352)
Ah. May be better to use `$CONTAINER_NAME` then?
Could be a separate pull request, because this is a bug fix?
(https://github.com/bitcoin/bitcoin/pull/29259#discussion_r1455566352)
Ah. May be better to use `$CONTAINER_NAME` then?
Could be a separate pull request, because this is a bug fix?
💬 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?
(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
...
(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!
(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
(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?
(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
(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
(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).
(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.
(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.