💬 fanquake commented on pull request "ci: re-add Valgrind job to the CI":
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2435545807)
Just dropped the exclusion.
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2435545807)
Just dropped the exclusion.
💬 diegoviola commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435575477)
@hebasto Thanks for letting me know. I managed to reproduce the artifacts/flicker with the original code on sway and labwc (wlroots-based compositors). In both cases, getting rid of the flags fixed the issue.
I've not tested this on KWin and Mutter yet, I plan on doing that later today. Hopefully, we can come up with a solution that works everywhere.
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435575477)
@hebasto Thanks for letting me know. I managed to reproduce the artifacts/flicker with the original code on sway and labwc (wlroots-based compositors). In both cases, getting rid of the flags fixed the issue.
I've not tested this on KWin and Mutter yet, I plan on doing that later today. Hopefully, we can come up with a solution that works everywhere.
💬 diegoviola commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435577485)
@hebasto Thanks for letting me know. I managed to reproduce the artifacts/flicker with the original code on sway and labwc (wlroots-based compositors). In both cases, getting rid of the flags fixed the issue.
I've not tested this on KWin and Mutter yet, I plan on doing that later today. Hopefully we can come up with a solution that works everywhere.
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435577485)
@hebasto Thanks for letting me know. I managed to reproduce the artifacts/flicker with the original code on sway and labwc (wlroots-based compositors). In both cases, getting rid of the flags fixed the issue.
I've not tested this on KWin and Mutter yet, I plan on doing that later today. Hopefully we can come up with a solution that works everywhere.
💬 marcofleon commented on pull request "[28.x] Backport & finalise 28.3":
(https://github.com/bitcoin/bitcoin/pull/33613#issuecomment-3410469971)
ACK 2dfb3a06902e9a98e00a422705afa002a3744545
(https://github.com/bitcoin/bitcoin/pull/33613#issuecomment-3410469971)
ACK 2dfb3a06902e9a98e00a422705afa002a3744545
💬 maflcko commented on pull request "ci: re-add Valgrind job to the CI":
(https://github.com/bitcoin/bitcoin/pull/33411#issuecomment-3410475429)
Again, this seems fine, and I don't want to raise an objection. However, the general question of how to deal with the false-positives (see my previous comment) here is still unanswered. IIUC, valgrind may not work under a given compiler (version). For example, currently it doesn't work under clang out of the box. There are workarounds needed, but they come with drawbacks:
* Add a suppression, but if this one is too broad, it decreases the value of running valgrind
* Use `-O1`, but valgrind i
...
(https://github.com/bitcoin/bitcoin/pull/33411#issuecomment-3410475429)
Again, this seems fine, and I don't want to raise an objection. However, the general question of how to deal with the false-positives (see my previous comment) here is still unanswered. IIUC, valgrind may not work under a given compiler (version). For example, currently it doesn't work under clang out of the box. There are workarounds needed, but they come with drawbacks:
* Add a suppression, but if this one is too broad, it decreases the value of running valgrind
* Use `-O1`, but valgrind i
...
💬 maflcko commented on pull request "Update GitHub Action to github-script@v8":
(https://github.com/bitcoin/bitcoin/pull/33634#issuecomment-3410485493)
> policy
I think the policy is "Ensure your workflows use the latest versions of actions that are running on Node 20. For more information, see [Using Versions for Actions](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-versioned-actions).". Source: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/#for-actions-users
Something similar may happen with node-24. However, I am not familiar with GHA
...
(https://github.com/bitcoin/bitcoin/pull/33634#issuecomment-3410485493)
> policy
I think the policy is "Ensure your workflows use the latest versions of actions that are running on Node 20. For more information, see [Using Versions for Actions](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-versioned-actions).". Source: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/#for-actions-users
Something similar may happen with node-24. However, I am not familiar with GHA
...
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435629619)
I did make a few changes (the linter alerted on a few lines) so please take a look again at what is here.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435629619)
I did make a few changes (the linter alerted on a few lines) so please take a look again at what is here.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435644194)
Oh, actually we need the weight further down for the check to see if the chunk will fit in the block, so I'm inclined to leave this as-is?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435644194)
Oh, actually we need the weight further down for the check to see if the chunk will fit in the block, so I'm inclined to leave this as-is?
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435654614)
Done.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435654614)
Done.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435668780)
Incorporated in a8be743aeb42ec8ab613f822989a11a2f2ce70ac
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435668780)
Incorporated in a8be743aeb42ec8ab613f822989a11a2f2ce70ac
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435683806)
Done in 6881d21c57ce122f0c2f5900e6caab7ba806d279.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435683806)
Done in 6881d21c57ce122f0c2f5900e6caab7ba806d279.
💬 alexanderwiederin commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3410666329)
re-ACK https://github.com/bitcoin/bitcoin/pull/30595/commits/a755e00a13541b3b5a707cf385f1cbec0449c6a9
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3410666329)
re-ACK https://github.com/bitcoin/bitcoin/pull/30595/commits/a755e00a13541b3b5a707cf385f1cbec0449c6a9
💬 hebasto commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435752838)
Is it necessary now to call `w->show();` here?
Would it be more clear to simply revert https://github.com/bitcoin-core/gui/commit/15aa7d023688700a47997b92108de95f2d864f5a?
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435752838)
Is it necessary now to call `w->show();` here?
Would it be more clear to simply revert https://github.com/bitcoin-core/gui/commit/15aa7d023688700a47997b92108de95f2d864f5a?
💬 diegoviola commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3410727752)
Just FYI: the Wayland developers have been working on the [ext-zones](https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/264) protocol for a while now, which I think is the sort of functionality that provides "always on top" and similar:
> Professional apps like DAWs rely a lot on having windows "always on top" floating above the other UI elements. On Wayland, this is not possible and the usability of these apps suffers a lot.
Until this isn't ready, I think we shoul
...
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3410727752)
Just FYI: the Wayland developers have been working on the [ext-zones](https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/264) protocol for a while now, which I think is the sort of functionality that provides "always on top" and similar:
> Professional apps like DAWs rely a lot on having windows "always on top" floating above the other UI elements. On Wayland, this is not possible and the usability of these apps suffers a lot.
Until this isn't ready, I think we shoul
...
⚠️ fanquake opened an issue: "ci: short read: expected xxxxxxxxx bytes but got xxxxxxxxx: unexpected EOF"
(https://github.com/bitcoin/bitcoin/issues/33640)
This issue is happening semi-frequently. i.e https://github.com/bitcoin/bitcoin/actions/runs/18560885434/job/52909339714?pr=33591#step:9:231:
```bash
#10 [5/5] RUN ["bash", "-c", "cd /ci_container_base/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/01_base_install.sh"]
#10 sha256:9ff7145d1456945488a8d8d2466983d85b23a449070f110460965a1547aec2c0 122.68MB / 660.19MB 5.7s
#10 sha256:3d058fa1c6c9853659e7c57f9938cc3e7bc8b256a1221e2fa5dfc955b8055a50 3.86kB / 3.86kB 0.3s done
#10 s
...
(https://github.com/bitcoin/bitcoin/issues/33640)
This issue is happening semi-frequently. i.e https://github.com/bitcoin/bitcoin/actions/runs/18560885434/job/52909339714?pr=33591#step:9:231:
```bash
#10 [5/5] RUN ["bash", "-c", "cd /ci_container_base/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/01_base_install.sh"]
#10 sha256:9ff7145d1456945488a8d8d2466983d85b23a449070f110460965a1547aec2c0 122.68MB / 660.19MB 5.7s
#10 sha256:3d058fa1c6c9853659e7c57f9938cc3e7bc8b256a1221e2fa5dfc955b8055a50 3.86kB / 3.86kB 0.3s done
#10 s
...
💬 diegoviola commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435784579)
@hebasto Yeah, I think a revert would also do the trick.
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435784579)
@hebasto Yeah, I think a revert would also do the trick.
🤔 janb84 reviewed a pull request: "test: [move-only] binary utils to utils.py"
(https://github.com/bitcoin/bitcoin/pull/33633#pullrequestreview-3344696191)
ACK fa75ef4328f638221bcf85fcbefa885122084622
Pr moves Helper class for information about bitcoin binaries to util.py.
To move the a helper class outside `Test framework` makes sense to me, this cleans up the `test_framework.py` code
Not completely convinced that these functions in the helper class will be used externally due to the tight coupling with these config settings `config["environment"]["BUILDDIR"], bin",` (this could be a good follow-up, if needed by external testers)
(https://github.com/bitcoin/bitcoin/pull/33633#pullrequestreview-3344696191)
ACK fa75ef4328f638221bcf85fcbefa885122084622
Pr moves Helper class for information about bitcoin binaries to util.py.
To move the a helper class outside `Test framework` makes sense to me, this cleans up the `test_framework.py` code
Not completely convinced that these functions in the helper class will be used externally due to the tight coupling with these config settings `config["environment"]["BUILDDIR"], bin",` (this could be a good follow-up, if needed by external testers)
📝 fanquake opened a pull request: "Update leveldb subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33641)
Rather than continue to close PRs/"Send these upstream" i.e: #33638, #33148, #22664, #13781; just fix the typos.
Includes https://github.com/bitcoin-core/leveldb-subtree/pull/57.
(https://github.com/bitcoin/bitcoin/pull/33641)
Rather than continue to close PRs/"Send these upstream" i.e: #33638, #33148, #22664, #13781; just fix the typos.
Includes https://github.com/bitcoin-core/leveldb-subtree/pull/57.
💬 diegoviola commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435815330)
@hebasto compiling 30.0 with https://github.com/bitcoin-core/gui/commit/15aa7d023688700a47997b92108de95f2d864f5a reverted, will confirm soon.
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435815330)
@hebasto compiling 30.0 with https://github.com/bitcoin-core/gui/commit/15aa7d023688700a47997b92108de95f2d864f5a reverted, will confirm soon.
💬 vasild commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2435833376)
```cpp
// pindexLastCommonBlock is required to be an ancestor of pindexBestKnownBlock
...
state->pindexBestKnownBlock->GetAncestor(state->pindexLastCommonBlock->nHeight)
```
If `Last` is an ancestor of `Best` and we get `Best`'s ancestor at height `Last.height`, isn't this always going to be `Last`?
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2435833376)
```cpp
// pindexLastCommonBlock is required to be an ancestor of pindexBestKnownBlock
...
state->pindexBestKnownBlock->GetAncestor(state->pindexLastCommonBlock->nHeight)
```
If `Last` is an ancestor of `Best` and we get `Best`'s ancestor at height `Last.height`, isn't this always going to be `Last`?