📝 l0rinc converted_to_draft a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906)
Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 by simplifying the coin cache public interface.
`CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).
Once it was marked as `dirty`, we couldn’t set the state back t
...
(https://github.com/bitcoin/bitcoin/pull/30906)
Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 by simplifying the coin cache public interface.
`CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).
Once it was marked as `dirty`, we couldn’t set the state back t
...
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1760397936)
aha, that makes much more sense!
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1760397936)
aha, that makes much more sense!
✅ glozow closed an issue: "Mempool + Validation Code Organization"
(https://github.com/bitcoin/bitcoin/issues/25584)
(https://github.com/bitcoin/bitcoin/issues/25584)
💬 glozow commented on issue "Mempool + Validation Code Organization":
(https://github.com/bitcoin/bitcoin/issues/25584#issuecomment-2351846451)
closing as (1) my main gripe with the fee estimator has been resolved, and (2) there isn't 1 correct actionable plan here. There are some general ideas of separation that we should incorporate into e.g. cluster mempool stuff, but I don't think there's any point in keeping this issue open.
(https://github.com/bitcoin/bitcoin/issues/25584#issuecomment-2351846451)
closing as (1) my main gripe with the fee estimator has been resolved, and (2) there isn't 1 correct actionable plan here. There are some general ideas of separation that we should incorporate into e.g. cluster mempool stuff, but I don't think there's any point in keeping this issue open.
💬 edilmedeiros commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2351881650)
Confirming this behavior is still happening after #28417. I'll rework #30130 on top of it.
```
2024-09-15 12:39:41 INFO Mined block at height 10078; next in -5784h44m40s (mine)
2024-09-15 12:39:58 INFO Mined block at height 10079; next in -5784h42m28s (mine)
2024-09-15 12:41:01 INFO Mined block at height 10080; next in -5784h41m0s (mine)
2024-09-15 12:41:43 INFO Mined block at height 10081; next in -5784h39m12s (mine)
Traceback (most recent call last):
File "/Users/jose.edil/2-develop
...
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2351881650)
Confirming this behavior is still happening after #28417. I'll rework #30130 on top of it.
```
2024-09-15 12:39:41 INFO Mined block at height 10078; next in -5784h44m40s (mine)
2024-09-15 12:39:58 INFO Mined block at height 10079; next in -5784h42m28s (mine)
2024-09-15 12:41:01 INFO Mined block at height 10080; next in -5784h41m0s (mine)
2024-09-15 12:41:43 INFO Mined block at height 10081; next in -5784h39m12s (mine)
Traceback (most recent call last):
File "/Users/jose.edil/2-develop
...
💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1760434344)
> I'm not sure what "available" means here?
From [man free](https://man7.org/linux/man-pages/man1/free.1.html):
> **available**
> Estimation of how much memory is available for starting new applications, without swapping. Unlike the data provided by the cache or free fields, this field takes into account page cache and also that not all reclaimable memory slabs will be reclaimed due to items being in use
So having "enough RAM" will work but having "enough available memory" is a more accurate
...
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1760434344)
> I'm not sure what "available" means here?
From [man free](https://man7.org/linux/man-pages/man1/free.1.html):
> **available**
> Estimation of how much memory is available for starting new applications, without swapping. Unlike the data provided by the cache or free fields, this field takes into account page cache and also that not all reclaimable memory slabs will be reclaimed due to items being in use
So having "enough RAM" will work but having "enough available memory" is a more accurate
...
💬 rob-scheepens commented on issue "Porting bcc tools to libbpf":
(https://github.com/bitcoin/bitcoin/issues/30298#issuecomment-2352217075)
@willcl-ark sorry for the delay in response. I'd love to implement this, but I doubt I currently have the necessary development skills (I'm just a performance engineer). I'll give it some thought and experiment a bit in the near future, let's see how far I get.
@0xB10C I just saw your eBPF Summit session, thanks for the update on the state of things. Have you by any chance seen the session on bpftime (https://youtu.be/YZbCBaTTkeE?si=a7qOB-arjxXBuTJP)? That appears to be useful in order to red
...
(https://github.com/bitcoin/bitcoin/issues/30298#issuecomment-2352217075)
@willcl-ark sorry for the delay in response. I'd love to implement this, but I doubt I currently have the necessary development skills (I'm just a performance engineer). I'll give it some thought and experiment a bit in the near future, let's see how far I get.
@0xB10C I just saw your eBPF Summit session, thanks for the update on the state of things. Have you by any chance seen the session on bpftime (https://youtu.be/YZbCBaTTkeE?si=a7qOB-arjxXBuTJP)? That appears to be useful in order to red
...
💬 maflcko commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2352219690)
> > > when opening or updating a PR that only touches a markdown file, I'd use this
> >
> >
> > We might still want to run spell check and such things for documentation changes.
>
> Insofar as the spelling linter in the CI doesn’t raise
There are many other linters that do raise, and even many that only serve to lint docs. If they serve no purpose, they should be removed, instead of being (required to be) skipped.
Regardless, many doc updates are only checked at compile time (doxyg
...
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2352219690)
> > > when opening or updating a PR that only touches a markdown file, I'd use this
> >
> >
> > We might still want to run spell check and such things for documentation changes.
>
> Insofar as the spelling linter in the CI doesn’t raise
There are many other linters that do raise, and even many that only serve to lint docs. If they serve no purpose, they should be removed, instead of being (required to be) skipped.
Regardless, many doc updates are only checked at compile time (doxyg
...
💬 willcl-ark commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352254067)
> The re-bucketed test in the commit doesn't match the one in the commit/PR title (p2p_timeouts vs. p2p_node_network_limited), so I suppose one of the two has to be adapted.
Ugh, I cherry-picked the wrong tests from my working branch. The title is correct, tests in the commit here updated.
> I think the sorting isn't really based on the "correct bucket", but rather the inverse overall duration, the buckets are just a guideline/reminder. That is, as long as the longest running test is start
...
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352254067)
> The re-bucketed test in the commit doesn't match the one in the commit/PR title (p2p_timeouts vs. p2p_node_network_limited), so I suppose one of the two has to be adapted.
Ugh, I cherry-picked the wrong tests from my working branch. The title is correct, tests in the commit here updated.
> I think the sorting isn't really based on the "correct bucket", but rather the inverse overall duration, the buckets are just a guideline/reminder. That is, as long as the longest running test is start
...
💬 maflcko commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352259405)
Thanks, the explanation is correct and the two links with data are useful to support this change.
review ACK 45663cd02cfb99c0bf91828b33a0611bb223497e
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352259405)
Thanks, the explanation is correct and the two links with data are useful to support this change.
review ACK 45663cd02cfb99c0bf91828b33a0611bb223497e
💬 maflcko commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352270431)
Nit: May be good to reword the pull request description to say that this better reflects the reality in CI runs, and may even speed them up. The mention of "CI timeouts" could be removed, because adding or removing 200 seconds from a run shouldn't be a fix (or cause) of a timeout. Any run that takes longer than half of the overall timeout value is a warning that should be investigated and fixed, because the CI timeout is really only there to catch cases where there is a true timeout (zombie proc
...
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352270431)
Nit: May be good to reword the pull request description to say that this better reflects the reality in CI runs, and may even speed them up. The mention of "CI timeouts" could be removed, because adding or removing 200 seconds from a run shouldn't be a fix (or cause) of a timeout. Any run that takes longer than half of the overall timeout value is a warning that should be investigated and fixed, because the CI timeout is really only there to catch cases where there is a true timeout (zombie proc
...
💬 willcl-ark commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352281417)
> Nit: May be good to reword the pull request description to say that this better reflects the reality in CI runs, and may even speed them up. The mention of "CI timeouts" could be removed, because adding or removing 200 seconds from a run shouldn't be a fix (or cause) of a timeout. Any run that takes longer than half of the overall timeout value is a warning that should be investigated and fixed, because the CI timeout is really only there to catch cases where there is a true timeout (zombie pr
...
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2352281417)
> Nit: May be good to reword the pull request description to say that this better reflects the reality in CI runs, and may even speed them up. The mention of "CI timeouts" could be removed, because adding or removing 200 seconds from a run shouldn't be a fix (or cause) of a timeout. Any run that takes longer than half of the overall timeout value is a warning that should be investigated and fixed, because the CI timeout is really only there to catch cases where there is a true timeout (zombie pr
...
💬 maflcko commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1760728552)
not sure this is going in the right direction. The namespace just happens to be a single value for all raw files. However, in theory, one could imagine several raw files having a different namespace each. This would then again increase the verbosity and complexity.
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1760728552)
not sure this is going in the right direction. The namespace just happens to be a single value for all raw files. However, in theory, one could imagine several raw files having a different namespace each. This would then again increase the verbosity and complexity.
💬 fanquake commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2352316231)
> Resolves https://github.com/bitcoin/bitcoin/issues/30876.
Not sure it does, as that issue applies to all dependencies, not just ZMQ.
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2352316231)
> Resolves https://github.com/bitcoin/bitcoin/issues/30876.
Not sure it does, as that issue applies to all dependencies, not just ZMQ.
💬 maflcko commented on pull request "kernel: Move background load thread to node context":
(https://github.com/bitcoin/bitcoin/pull/30896#discussion_r1760748575)
I think the doxygen links in this doc are dead anyway, or at least stale. (At least I can't remember when they last worked and were up-to-date).
(https://github.com/bitcoin/bitcoin/pull/30896#discussion_r1760748575)
I think the doxygen links in this doc are dead anyway, or at least stale. (At least I can't remember when they last worked and were up-to-date).
💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2352325167)
Seems like you missed some review comments?
https://github.com/bitcoin/bitcoin/pull/30861/files#r1752126150
https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752127638
Seems like https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752135299 is now also more relevant given #30905, so it'd be good to know how this is going to be handed going forward.
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2352325167)
Seems like you missed some review comments?
https://github.com/bitcoin/bitcoin/pull/30861/files#r1752126150
https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752127638
Seems like https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752135299 is now also more relevant given #30905, so it'd be good to know how this is going to be handed going forward.
✅ fanquake closed an issue: "translations: `28.x` update pulled in random strings?"
(https://github.com/bitcoin/bitcoin/issues/30897)
(https://github.com/bitcoin/bitcoin/issues/30897)
🚀 fanquake merged a pull request: "qt: Translations update"
(https://github.com/bitcoin/bitcoin/pull/30899)
(https://github.com/bitcoin/bitcoin/pull/30899)
💬 hebasto commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2352358227)
> > Resolves #30876.
>
> Not sure it does, as that issue applies to all dependencies, not just ZMQ.
Clarified.
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2352358227)
> > Resolves #30876.
>
> Not sure it does, as that issue applies to all dependencies, not just ZMQ.
Clarified.
💬 hebasto commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2352376552)
> I am not really familiar with GHA, nor with macOS, so it would be good if someone else checked:
>
> * Does the macOS 13 GHA task succeed at all with an empty cache?
https://github.com/hebasto/bitcoin/actions/runs/10880757045
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2352376552)
> I am not really familiar with GHA, nor with macOS, so it would be good if someone else checked:
>
> * Does the macOS 13 GHA task succeed at all with an empty cache?
https://github.com/hebasto/bitcoin/actions/runs/10880757045