💬 hebasto commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435501488)
This code was introduced in https://github.com/bitcoin-core/gui/pull/831.
cc @pablomartin4btc
(https://github.com/bitcoin-core/gui/pull/904#discussion_r2435501488)
This code was introduced in https://github.com/bitcoin-core/gui/pull/831.
cc @pablomartin4btc
💬 stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2435508418)
> why wouldn't we want to be a bit spammy in this scenario?
I don't disagree, but it seems orthogonal to this PR which is addressing db corruption issues? It's okay for warning messages to not cover all edge cases, but I think it is good to try to avoid incorrect diagnostics or solutions if we can help it without too much complexity. `invalidateblock` is debug-only, but is also commonly referenced as a break-in-case-of-emergency RPC. With the current change, usage of the RPC is guaranteed to
...
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2435508418)
> why wouldn't we want to be a bit spammy in this scenario?
I don't disagree, but it seems orthogonal to this PR which is addressing db corruption issues? It's okay for warning messages to not cover all edge cases, but I think it is good to try to avoid incorrect diagnostics or solutions if we can help it without too much complexity. `invalidateblock` is debug-only, but is also commonly referenced as a break-in-case-of-emergency RPC. With the current change, usage of the RPC is guaranteed to
...
💬 stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2435514483)
Alternatively, I would be okay with (but not prefer) the log message to be generalized to highlight that db corruption or consensus failure are possible, and the `invalidateblock` RPC documentation updated to highlight that this warning will be flooded upon usage.
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2435514483)
Alternatively, I would be okay with (but not prefer) the log message to be generalized to highlight that db corruption or consensus failure are possible, and the `invalidateblock` RPC documentation updated to highlight that this warning will be flooded upon usage.
🤔 janb84 reviewed a pull request: "Update secp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33625#pullrequestreview-3344304291)
ACK
<details> <summary>Guix Build Output</summary>
**Host architecture:** `aarch64`
**Commit:** `879c21045eba`
```shell
22b8a342b9e47069d05d1fca40437ce2b19820e3f3c6b7ef17f2132a462500f9 guix-build-879c21045eba/output/aarch64-linux-gnu/SHA256SUMS.part
8f156c6e1cea62efb25007ab62ce02292eac014fe81f526249e4ff6724292a0c guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21045eba-aarch64-linux-gnu-debug.tar.gz
9afd57b78623c0ca553a6e8bf794baf8e31ee55b3dcbe9e265b02810cfaebb82
...
(https://github.com/bitcoin/bitcoin/pull/33625#pullrequestreview-3344304291)
ACK
<details> <summary>Guix Build Output</summary>
**Host architecture:** `aarch64`
**Commit:** `879c21045eba`
```shell
22b8a342b9e47069d05d1fca40437ce2b19820e3f3c6b7ef17f2132a462500f9 guix-build-879c21045eba/output/aarch64-linux-gnu/SHA256SUMS.part
8f156c6e1cea62efb25007ab62ce02292eac014fe81f526249e4ff6724292a0c guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21045eba-aarch64-linux-gnu-debug.tar.gz
9afd57b78623c0ca553a6e8bf794baf8e31ee55b3dcbe9e265b02810cfaebb82
...
💬 maflcko commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3410381160)
I'd expect that cross-compilation in this CI task always succeeds, if all the other CI tasks (including the other cross-compile ones). For s390x the value really is in running the full tests to see any endian bugs.
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3410381160)
I'd expect that cross-compilation in this CI task always succeeds, if all the other CI tasks (including the other cross-compile ones). For s390x the value really is in running the full tests to see any endian bugs.
💬 naiyoma commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3410382253)
> With this PR, if an address older than 10 days (but not older than 30 days) is part of a `GetAddr` answer, the receiving peer will _postdate_ its timestamp, giving it more time until it gets Terrible. There is a chaining effect, because the receiving peer will also relay it longer when it answers `GetAddr` requests itself, and so on. If this would happen frequently enough, the addr of the node that has left the network would always stay in the 10-30 day range and never cease to be relayed, a
...
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3410382253)
> With this PR, if an address older than 10 days (but not older than 30 days) is part of a `GetAddr` answer, the receiving peer will _postdate_ its timestamp, giving it more time until it gets Terrible. There is a chaining effect, because the receiving peer will also relay it longer when it answers `GetAddr` requests itself, and so on. If this would happen frequently enough, the addr of the node that has left the network would always stay in the 10-30 day range and never cease to be relayed, a
...
👍 stickies-v approved a pull request: "[28.x] Backport & finalise 28.3"
(https://github.com/bitcoin/bitcoin/pull/33613#pullrequestreview-3344326684)
ACK 2dfb3a06902e9a98e00a422705afa002a3744545
No changes except for 1 CI change, which I (utACK) verified. And finalization.
(https://github.com/bitcoin/bitcoin/pull/33613#pullrequestreview-3344326684)
ACK 2dfb3a06902e9a98e00a422705afa002a3744545
No changes except for 1 CI change, which I (utACK) verified. And finalization.
💬 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?