💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2739398089)
My guix output matches @hebasto's
<details>
<summary>hashes</summary>
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
7638ac47fcd9ea8a4e2bbb265876aaeba0d8fe774f5bcc884753331d6ffe3429 guix-build-94967c353ed8/output/aarch64-linux-gnu/SHA256SUMS.part
3d7ff7c2c1ad1608946452645d9047ceef0e77f589098c94b5d628476ee5192d guix-build-94967c353ed8/output/aarch64-linux-gnu/bitcoin-94967c353ed8-aarch64-linux-gnu-debug.tar.gz
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2739398089)
My guix output matches @hebasto's
<details>
<summary>hashes</summary>
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
7638ac47fcd9ea8a4e2bbb265876aaeba0d8fe774f5bcc884753331d6ffe3429 guix-build-94967c353ed8/output/aarch64-linux-gnu/SHA256SUMS.part
3d7ff7c2c1ad1608946452645d9047ceef0e77f589098c94b5d628476ee5192d guix-build-94967c353ed8/output/aarch64-linux-gnu/bitcoin-94967c353ed8-aarch64-linux-gnu-debug.tar.gz
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004835358)
In 9b8fa3cd2e0d5851355352e1de37c76bd3821e96:
It's not clear why this change is needed, and I think it's going in the wrong direction; `gcc` should not be getting hardcoded here (#30206).
The commit message also seems to be a mashup of things that aren't necessarily related to Qt, and makes out that this is macOS related (even though the change is being applied to all platforms). It also doesn't explain the actual issue, just that apparently the current code is overkill/undesirable. What's th
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004835358)
In 9b8fa3cd2e0d5851355352e1de37c76bd3821e96:
It's not clear why this change is needed, and I think it's going in the wrong direction; `gcc` should not be getting hardcoded here (#30206).
The commit message also seems to be a mashup of things that aren't necessarily related to Qt, and makes out that this is macOS related (even though the change is being applied to all platforms). It also doesn't explain the actual issue, just that apparently the current code is overkill/undesirable. What's th
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004895158)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
Why is `-pkg-config` usage being reintroduced here?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004895158)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
Why is `-pkg-config` usage being reintroduced here?
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004952081)
see also the TODO item
> * Ability to inspect the would-be-constructed clusters (so they can be "fixed" if they violate policy rules, as opposed to just accepted/rejected, before applying - this is needed for reorgs which are not optional).
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004952081)
see also the TODO item
> * Ability to inspect the would-be-constructed clusters (so they can be "fixed" if they violate policy rules, as opposed to just accepted/rejected, before applying - this is needed for reorgs which are not optional).
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2739415197)
(conflicts with just merged #31519, Span to std::span)
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2739415197)
(conflicts with just merged #31519, Span to std::span)
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004983417)
This links to a bug report for Qt 6.2.2, where the last comment was 3 years ago. Should we leave a new comment that this is still an issue for all current Qt versions (I assume this is the case otherwise we'd have a patch to apply?).
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004983417)
This links to a bug report for Qt 6.2.2, where the last comment was 3 years ago. Should we leave a new comment that this is still an issue for all current Qt versions (I assume this is the case otherwise we'd have a patch to apply?).
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004991819)
See #31553, which adds a `TxGraph::Trim` function that removes some subset of transactions + their descendants, such that the result is no longer oversized, in O(n log n) time.
When I started this PR, the idea was to expose ways to inspect would-be-oversized clusters, so they could be fixed by the user, before committing.
It turned out that really nothing beats an internal "figure it out, and fix it" method in terms of efficiency and convenience, so that's what 31553 does. It's a bit of a desi
...
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004991819)
See #31553, which adds a `TxGraph::Trim` function that removes some subset of transactions + their descendants, such that the result is no longer oversized, in O(n log n) time.
When I started this PR, the idea was to expose ways to inspect would-be-oversized clusters, so they could be fixed by the user, before committing.
It turned out that really nothing beats an internal "figure it out, and fix it" method in terms of efficiency and convenience, so that's what 31553 does. It's a bit of a desi
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005002572)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
Shouldn't this be `NOTICE` unless `V=1` or `DEBUG=1`; otherwise, why are we defaulting to the most verbose logging?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005002572)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
Shouldn't this be `NOTICE` unless `V=1` or `DEBUG=1`; otherwise, why are we defaulting to the most verbose logging?
💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r2005012921)
fixed.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r2005012921)
fixed.
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005022060)
Can we move the translations into `/translations` and then remove the need for this condition?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005022060)
Can we move the translations into `/translations` and then remove the need for this condition?
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005024517)
Still not sure I understand this addtion. The people reading these docs (self compiling) don't need it, because all the deps they need should be in the docs already, and the person using the release binaries isn't reading the self-compilation docs, so isn't going to see it?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005024517)
Still not sure I understand this addtion. The people reading these docs (self compiling) don't need it, because all the deps they need should be in the docs already, and the person using the release binaries isn't reading the self-compilation docs, so isn't going to see it?
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005028449)
What would be a better way to proceed here?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005028449)
What would be a better way to proceed here?
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005045288)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
I don't think `--parallel` should be used here, because it's not going to respect any `-j` argument given to depends? I'm constantly seeing issues where the build fails, basically because ninja is trying to spawn infinite amounts of compile jobs.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005045288)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
I don't think `--parallel` should be used here, because it's not going to respect any `-j` argument given to depends? I'm constantly seeing issues where the build fails, basically because ninja is trying to spawn infinite amounts of compile jobs.
💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2005048885)
In terms of hashing, it is no different than opcodes like `OP_SHA256`, as it hashes a stack element (prefixed with an x-only key, so up to 552 bytes in total).
So restricting the opcode to just the current input shouldn't make any difference.
That should be significantly smaller than the the cost of the double tweak, which should instead be comparable (and priced appropriately in sigops) to signatures – hopefully a bit less than that, but this needs a proper benchmark.
Note that no non-wi
...
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2005048885)
In terms of hashing, it is no different than opcodes like `OP_SHA256`, as it hashes a stack element (prefixed with an x-only key, so up to 552 bytes in total).
So restricting the opcode to just the current input shouldn't make any difference.
That should be significantly smaller than the the cost of the double tweak, which should instead be comparable (and priced appropriately in sigops) to signatures – hopefully a bit less than that, but this needs a proper benchmark.
Note that no non-wi
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2739574777)
@willcl-ark
Could you take a look at this PR?
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2739574777)
@willcl-ark
Could you take a look at this PR?
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005061988)
:+1: I think it makes sense for TxGraph to take care of all the topology checks and feerate comparison decisions, which includes finding the best txs (block building), finding the worst txs (eviction) and finding good subsets when limits would otherwise be exceeded (reorgs, "rbf"/"carve out" cases maybe). So this doesn't feel like a design break at all to me, but rather pretty much exactly parallel to `GetWorstMainChunk()` -- "here's a bunch of txs that you want to remove".
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005061988)
:+1: I think it makes sense for TxGraph to take care of all the topology checks and feerate comparison decisions, which includes finding the best txs (block building), finding the worst txs (eviction) and finding good subsets when limits would otherwise be exceeded (reorgs, "rbf"/"carve out" cases maybe). So this doesn't feel like a design break at all to me, but rather pretty much exactly parallel to `GetWorstMainChunk()` -- "here's a bunch of txs that you want to remove".
💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2739603182)
Rebased and fix comment by @yancyribbens
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2739603182)
Rebased and fix comment by @yancyribbens
💬 ismaelsadeeq commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2005076794)
There is no unintended white space in the current list hence it is redundant not needed see my comment here https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1927220284
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2005076794)
There is no unintended white space in the current list hence it is redundant not needed see my comment here https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1927220284
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005082494)
The dependencies required to build `depends` are listed in [`/depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md), and the second commit already updates them to include the `ninja-build` package.
As for mentioning the CRB repository, [`/depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) does not have a CentOS-specific section.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005082494)
The dependencies required to build `depends` are listed in [`/depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md), and the second commit already updates them to include the `ninja-build` package.
As for mentioning the CRB repository, [`/depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) does not have a CentOS-specific section.
🤔 ismaelsadeeq reviewed a pull request: "test: Add test coverage for rpcwhitelistdefault when unset"
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2701745045)
Thanks for following up!
cb5e26d6f2dafa150d87545890afaeeceb5c610a is doing multiple things.
Can you make the commits atomic just as you did in the PR description?
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2701745045)
Thanks for following up!
cb5e26d6f2dafa150d87545890afaeeceb5c610a is doing multiple things.
Can you make the commits atomic just as you did in the PR description?