💬 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?
💬 musaHaruna commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2005097804)
Thanks for the clarification! Yeah the previous is making it redundant. Hence please @naiyoma ignore my nit
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2005097804)
Thanks for the clarification! Yeah the previous is making it redundant. Hence please @naiyoma ignore my nit
🤔 ajtowns reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2701725334)
ReACK 63b8f96e67f9ad649070a231532d48fb6c3573e4 . Needs a rebase anyway though.
The main/staging changes read better to me fwiw. The level stuff in the locators could be ungeneralised as well I suppose, but that doesn't seem likely to be much of an improvement.
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2701725334)
ReACK 63b8f96e67f9ad649070a231532d48fb6c3573e4 . Needs a rebase anyway though.
The main/staging changes read better to me fwiw. The level stuff in the locators could be ungeneralised as well I suppose, but that doesn't seem likely to be much of an improvement.
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005075531)
Could tidy this up:
```c++
if (!m_main_clusterset.empty()) return;
if (m_staging_clusterset.has_value() && !m_staging_clusterset->empty()) return;
bool ClusterSet::empty() {
return m_deps_to_add.empty() && m_to_remove.empty() && m_removed.empty();
}
```
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005075531)
Could tidy this up:
```c++
if (!m_main_clusterset.empty()) return;
if (m_staging_clusterset.has_value() && !m_staging_clusterset->empty()) return;
bool ClusterSet::empty() {
return m_deps_to_add.empty() && m_to_remove.empty() && m_removed.empty();
}
```
💬 l0rinc commented on pull request "leveldb: pull upstream C++23 changes":
(https://github.com/bitcoin/bitcoin/pull/31766#issuecomment-2739679633)
`reindex-chainstate` finished successfully
<details>
<summary>Details</summary>
```bash
COMMITS="5e934ebc82ca239cd98eed1689f7cdc2c672a9ae"; \
STOP_HEIGHT=888000; DBCACHE=5000; \
C_COMPILER=gcc; CXX_COMPILER=g++; \
git fetch --all -q && (for c in $COMMITS; do git log -1 --oneline $c || exit 1; done) && \
hyperfine \
--export-json "/mnt/my_storage/rdx-${COMMITS// /-}-${STOP_HEIGHT}-${DBCACHE}-${C_COMPILER}.json" \
--runs 1 \
--parameter-list COMMIT ${COMMITS// /,} \
--prepa
...
(https://github.com/bitcoin/bitcoin/pull/31766#issuecomment-2739679633)
`reindex-chainstate` finished successfully
<details>
<summary>Details</summary>
```bash
COMMITS="5e934ebc82ca239cd98eed1689f7cdc2c672a9ae"; \
STOP_HEIGHT=888000; DBCACHE=5000; \
C_COMPILER=gcc; CXX_COMPILER=g++; \
git fetch --all -q && (for c in $COMMITS; do git log -1 --oneline $c || exit 1; done) && \
hyperfine \
--export-json "/mnt/my_storage/rdx-${COMMITS// /-}-${STOP_HEIGHT}-${DBCACHE}-${C_COMPILER}.json" \
--runs 1 \
--parameter-list COMMIT ${COMMITS// /,} \
--prepa
...
💬 l0rinc commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2005143296)
Related to https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1926634436 - it's more uniform now
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2005143296)
Related to https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1926634436 - it's more uniform now