💬 m3dwards commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3577089817)
> I’m unable to reproduce this in a fresh Trixie container using Docker
I did almost the same apart from I checked out this PR rather than building from master.
This works on x86 but isn't working on Apple ARM for me. I don't think it's anything wrong with this PR, but it's a problem with the UCRT MinGW compiler. That being the case, should this hold up this PR?
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3577089817)
> I’m unable to reproduce this in a fresh Trixie container using Docker
I did almost the same apart from I checked out this PR rather than building from master.
This works on x86 but isn't working on Apple ARM for me. I don't think it's anything wrong with this PR, but it's a problem with the UCRT MinGW compiler. That being the case, should this hold up this PR?
📝 Fibonacci747 opened a pull request: "fix: remove redundant mempool lock in ChainImpl::isInMempool()"
(https://github.com/bitcoin/bitcoin/pull/33946)
removes an unnecessary LOCK(mempool->cs) in ChainImpl::isInMempool(). The method calls CTxMemPool::exists(), which already locks mempool->cs internally. Because the mempool mutex is a RecursiveMutex, double-locking was safe but redundant. Dropping the outer lock matches patterns used elsewhere in ChainImpl (e.g., hasDescendantsInMempool() and GetTransactionAncestry() callers) where mempool read APIs are invoked without an additional lock and rely on the callee’s internal locking. isRBFOptIn() re
...
(https://github.com/bitcoin/bitcoin/pull/33946)
removes an unnecessary LOCK(mempool->cs) in ChainImpl::isInMempool(). The method calls CTxMemPool::exists(), which already locks mempool->cs internally. Because the mempool mutex is a RecursiveMutex, double-locking was safe but redundant. Dropping the outer lock matches patterns used elsewhere in ChainImpl (e.g., hasDescendantsInMempool() and GetTransactionAncestry() callers) where mempool read APIs are invoked without an additional lock and rely on the callee’s internal locking. isRBFOptIn() re
...
💬 janb84 commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2561205810)
I find this a bit inconsistent with this enum :
```C
typedef uint8_t btck_ValidationMode;
#define btck_ValidationMode_VALID ((btck_ValidationMode)(0))
#define btck_ValidationMode_INVALID ((btck_ValidationMode)(1))
#define btck_ValidationMode_INTERNAL_ERROR ((btck_ValidationMode)(2))
```
Where `validationmode` runs from 0 to 2 and 2 is the internal error.
Also isn't it better to expose this as an ENUM so that it can be re-used by the handlers ? (not sure because of the single use)
...
(https://github.com/bitcoin/bitcoin/pull/33943#discussion_r2561205810)
I find this a bit inconsistent with this enum :
```C
typedef uint8_t btck_ValidationMode;
#define btck_ValidationMode_VALID ((btck_ValidationMode)(0))
#define btck_ValidationMode_INVALID ((btck_ValidationMode)(1))
#define btck_ValidationMode_INTERNAL_ERROR ((btck_ValidationMode)(2))
```
Where `validationmode` runs from 0 to 2 and 2 is the internal error.
Also isn't it better to expose this as an ENUM so that it can be re-used by the handlers ? (not sure because of the single use)
...
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561218853)
Fixed in 9e7410479.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561218853)
Fixed in 9e7410479.
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561221040)
Took this (with one small fix) in 375682ec
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561221040)
Took this (with one small fix) in 375682ec
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561222587)
Fixed in 472e3e16
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561222587)
Fixed in 472e3e16
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561225440)
Replaced this with AssertLockHeld(pool.cs), and added lock annotations to both the single and package TRUC checks in 70893b6b
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561225440)
Replaced this with AssertLockHeld(pool.cs), and added lock annotations to both the single and package TRUC checks in 70893b6b
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561233005)
I'm going with `chunkweight` and `clusterweight` for now. Not sure what to do for the output of `getmempoolfeeratediagram()` though?
I also took your doc suggestion and created a new file in doc/policy/ that explains the terminology. Would it better to consolidate some of those files into one?
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561233005)
I'm going with `chunkweight` and `clusterweight` for now. Not sure what to do for the output of `getmempoolfeeratediagram()` though?
I also took your doc suggestion and created a new file in doc/policy/ that explains the terminology. Would it better to consolidate some of those files into one?
💬 pablomartin4btc commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3577355929)
> > The fix is in qt6.3, so this is expected. Your options are:
>
> > ```
> > * Use depends yourself
> > ```
>
> I think I tried that before... I'll check it again, thanks!
Sorry, that (building `depends`) uses `xcb`... and that works well.
<details>
<summary>Built correctly from source both Qt 6.3.2 and 6.10.0, and <code>bringToFront</code> doesn't work on either of them using Wayland. I'll upgrade Ubuntu to 24.04 at some point and will check it again.</summary>
```
2025-11-2
...
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3577355929)
> > The fix is in qt6.3, so this is expected. Your options are:
>
> > ```
> > * Use depends yourself
> > ```
>
> I think I tried that before... I'll check it again, thanks!
Sorry, that (building `depends`) uses `xcb`... and that works well.
<details>
<summary>Built correctly from source both Qt 6.3.2 and 6.10.0, and <code>bringToFront</code> doesn't work on either of them using Wayland. I'll upgrade Ubuntu to 24.04 at some point and will check it again.</summary>
```
2025-11-2
...
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561237544)
Oops, fixed in 62b2f2144
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561237544)
Oops, fixed in 62b2f2144
💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561239191)
Added a comment in 875f1b98fcc
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561239191)
Added a comment in 875f1b98fcc
🤔 instagibbs reviewed a pull request: "Cluster mempool followups"
(https://github.com/bitcoin/bitcoin/pull/33591#pullrequestreview-3506023834)
reviewed through 875f1b98fcc9a992a3280bf32edc2d30c1fb034d
(https://github.com/bitcoin/bitcoin/pull/33591#pullrequestreview-3506023834)
reviewed through 875f1b98fcc9a992a3280bf32edc2d30c1fb034d
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560781764)
f1cfc07b8af90d9c80c84e9db56d61c56f50d6c2
From a cursory glance I could not figure out when this happens. I believe behavior matches, but I'm still curious.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560781764)
f1cfc07b8af90d9c80c84e9db56d61c56f50d6c2
From a cursory glance I could not figure out when this happens. I believe behavior matches, but I'm still curious.
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560856387)
05a7b963c93501939ec0523fe0182af87ee40056
Light preference for splitting out the checks to make the `visited()` mutation more obvious
```
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 5efdfbf133..7765f27b79 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -365,5 +365,6 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check
WITH_FRESH_EPOCH(m_epoch);
for (indexed_transaction_set::const_iterator it = mapTx.begin(); it !
...
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560856387)
05a7b963c93501939ec0523fe0182af87ee40056
Light preference for splitting out the checks to make the `visited()` mutation more obvious
```
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 5efdfbf133..7765f27b79 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -365,5 +365,6 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check
WITH_FRESH_EPOCH(m_epoch);
for (indexed_transaction_set::const_iterator it = mapTx.begin(); it !
...
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560753152)
f1cfc07b8af90d9c80c84e9db56d61c56f50d6c2
Are we using this elsewhere or could this be private?
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560753152)
f1cfc07b8af90d9c80c84e9db56d61c56f50d6c2
Are we using this elsewhere or could this be private?
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560767330)
f1cfc07b8af90d9c80c84e9db56d61c56f50d6c2
```Suggestion
// be sure to remove any descendants that are in the pool. This can
```
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560767330)
f1cfc07b8af90d9c80c84e9db56d61c56f50d6c2
```Suggestion
// be sure to remove any descendants that are in the pool. This can
```
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560813764)
05a7b963c93501939ec0523fe0182af87ee40056
not related to cluster mempool, but doesn't this seem pretty expensive, wondering if we can make this an `Assume()` instead
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2560813764)
05a7b963c93501939ec0523fe0182af87ee40056
not related to cluster mempool, but doesn't this seem pretty expensive, wondering if we can make this an `Assume()` instead
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561106726)
d2b0872461158e69675985e8c9e2eb929d904683
seems like these checks are backwards now compared to before, but seems right after these changes...
@glozow ?
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561106726)
d2b0872461158e69675985e8c9e2eb929d904683
seems like these checks are backwards now compared to before, but seems right after these changes...
@glozow ?
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561149521)
c11faf779a1c415f8a04463bfb3febc990eb297b
maybe:
"Transactions submitted to the mempol must not result in clusters which would exceed cluster limits set by"
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561149521)
c11faf779a1c415f8a04463bfb3febc990eb297b
maybe:
"Transactions submitted to the mempol must not result in clusters which would exceed cluster limits set by"
💬 instagibbs commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561313165)
486b441971912bdc63921425d5e40a8c168186ea
pretty sure there's at least one bug here. f.e., if you have three chunks in one cluster, I think the conditional here is only hit the first time, as `current_chunk_feerate` is never reset to the next chunk's size, so it'll just start going negative.
Wrote a test which I think covers it
```
diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
index 4b4812e619..a7eabef039 100755
--- a/test/functional/mempool_clu
...
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2561313165)
486b441971912bdc63921425d5e40a8c168186ea
pretty sure there's at least one bug here. f.e., if you have three chunks in one cluster, I think the conditional here is only hit the first time, as `current_chunk_feerate` is never reset to the next chunk's size, so it'll just start going negative.
Wrote a test which I think covers it
```
diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
index 4b4812e619..a7eabef039 100755
--- a/test/functional/mempool_clu
...