💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790792236)
> Can you explain the patch? What is the actual problem, and how does this fix it? Seems a bit odd to have to patch version detection checks inside Qt, can the correct values not be passed in from the outside?
In macOS SDK, the problematic `os/activity.h` header relies on the `OS_LOG_TARGET_HAS_10_12_FEATURES` macro:
```c++
#if OS_LOG_TARGET_HAS_10_12_FEATURES
#ifndef OS_ACTIVITY_OBJECT_API
#define OS_ACTIVITY_OBJECT_API 1
#endif // OS_ACTIVITY_OBJECT_API
#else
#if OS_ACTIVITY_OBJECT_A
...
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790792236)
> Can you explain the patch? What is the actual problem, and how does this fix it? Seems a bit odd to have to patch version detection checks inside Qt, can the correct values not be passed in from the outside?
In macOS SDK, the problematic `os/activity.h` header relies on the `OS_LOG_TARGET_HAS_10_12_FEATURES` macro:
```c++
#if OS_LOG_TARGET_HAS_10_12_FEATURES
#ifndef OS_ACTIVITY_OBJECT_API
#define OS_ACTIVITY_OBJECT_API 1
#endif // OS_ACTIVITY_OBJECT_API
#else
#if OS_ACTIVITY_OBJECT_A
...
💬 fanquake commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790817038)
> the problematic os/activity.h header
What is the problem with the header?
The (availability.h) macros are set via the `-mmacosx-version-min` flag (which we set):
> For these macros to function properly, a program must specify the OS version range
> it is targeting. The min OS version is specified as an option to the compiler:
> -mmacosx-version-min=10.x when building for Mac OS X,
Why do we need to hardcode them to the values that they should already be being set too?
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790817038)
> the problematic os/activity.h header
What is the problem with the header?
The (availability.h) macros are set via the `-mmacosx-version-min` flag (which we set):
> For these macros to function properly, a program must specify the OS version range
> it is targeting. The min OS version is specified as an option to the compiler:
> -mmacosx-version-min=10.x when building for Mac OS X,
Why do we need to hardcode them to the values that they should already be being set too?
💬 vasild commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1790819342)
`e13401084f...93cd65566a`: fix https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1790785177, thanks!
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1790819342)
`e13401084f...93cd65566a`: fix https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1790785177, thanks!
💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1380216381)
In 983f8c6e305fd9707c109c2a92637825262b9b09: Correct me if I'm wrong, but we're going use `inbounds_nonrcncl_tx_relay` and `outbounds_nonrcncl_tx_relay` only whether `reconciles_txs` is true, couldn't we only fill them if so?:
```suggestion
if (reconciles_txs) {
```
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1380216381)
In 983f8c6e305fd9707c109c2a92637825262b9b09: Correct me if I'm wrong, but we're going use `inbounds_nonrcncl_tx_relay` and `outbounds_nonrcncl_tx_relay` only whether `reconciles_txs` is true, couldn't we only fill them if so?:
```suggestion
if (reconciles_txs) {
```
👍 MarnixCroes approved a pull request: "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog"
(https://github.com/bitcoin-core/gui/pull/762#pullrequestreview-1710440253)
tack 30c235b2e6fd9b7e3634d795cedc171d4d8d0e27
(https://github.com/bitcoin-core/gui/pull/762#pullrequestreview-1710440253)
tack 30c235b2e6fd9b7e3634d795cedc171d4d8d0e27
💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790840873)
> > the problematic os/activity.h header
>
> What is the problem with the header?
The `OS_ACTIVITY_OBJECT_API` is set to `0`, which makes a bunch of code unavailable, which in turn causes compile errors.
> The (availability.h) macros are set via the `-mmacosx-version-min` flag (which we set):
>
> > For these macros to function properly, a program must specify the OS version range
> > it is targeting. The min OS version is specified as an option to the compiler:
> > -mmacosx-versio
...
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790840873)
> > the problematic os/activity.h header
>
> What is the problem with the header?
The `OS_ACTIVITY_OBJECT_API` is set to `0`, which makes a bunch of code unavailable, which in turn causes compile errors.
> The (availability.h) macros are set via the `-mmacosx-version-min` flag (which we set):
>
> > For these macros to function properly, a program must specify the OS version range
> > it is targeting. The min OS version is specified as an option to the compiler:
> > -mmacosx-versio
...
💬 maflcko commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1790859091)
I don't think a mutex is required for read-only access to a const blob of memory?
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1790859091)
I don't think a mutex is required for read-only access to a const blob of memory?
💬 hebasto commented on pull request "Replace RecursiveMutex m_cs_banned with Mutex, and rename it":
(https://github.com/bitcoin/bitcoin/pull/24097#issuecomment-1790875301)
@fanquake @achow101
Is there anything else to do here?
(https://github.com/bitcoin/bitcoin/pull/24097#issuecomment-1790875301)
@fanquake @achow101
Is there anything else to do here?
💬 dergoegge commented on issue "make cov fails with lcov-2":
(https://github.com/bitcoin/bitcoin/issues/28468#issuecomment-1790901853)
> Can't locate Capture/Tiny.pm in @INC (you may need to install the Capture::Tiny module)
Can be fixed by `apt install libcapture-tiny-perl libdatetime-perl` but it's still broken after that when trying to generate coverage.
e.g. from `make cov_fuzz`:
```
geninfo: ERROR: "/workdir/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/date_time/gregorian/greg_month.hpp":40: function _ZN5boost9gregorian9bad_monthD0Ev found on line but no corresponding 'line' coverage data point. Cannot deri
...
(https://github.com/bitcoin/bitcoin/issues/28468#issuecomment-1790901853)
> Can't locate Capture/Tiny.pm in @INC (you may need to install the Capture::Tiny module)
Can be fixed by `apt install libcapture-tiny-perl libdatetime-perl` but it's still broken after that when trying to generate coverage.
e.g. from `make cov_fuzz`:
```
geninfo: ERROR: "/workdir/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/date_time/gregorian/greg_month.hpp":40: function _ZN5boost9gregorian9bad_monthD0Ev found on line but no corresponding 'line' coverage data point. Cannot deri
...
💬 fanquake commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790947834)
> We rather are adjusting the current inappropriate values, no?
The default values shouldn't need adjusting in the qt source code, if our version setting works. So this would point to that not being the case?
> The OS_ACTIVITY_OBJECT_API is set to 0, which makes a bunch of code unavailable, which in turn causes compile errors.
In that case, maybe it would be easier to set something like `-DOS_ACTIVITY_OBJECT_API=1` for the Qt build, and not have to patch any source?
However that wou
...
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790947834)
> We rather are adjusting the current inappropriate values, no?
The default values shouldn't need adjusting in the qt source code, if our version setting works. So this would point to that not being the case?
> The OS_ACTIVITY_OBJECT_API is set to 0, which makes a bunch of code unavailable, which in turn causes compile errors.
In that case, maybe it would be easier to set something like `-DOS_ACTIVITY_OBJECT_API=1` for the Qt build, and not have to patch any source?
However that wou
...
📝 BrandonOdiwuor opened a pull request: "Wallet: Functions to enable adding used balance to GUI overview page"
(https://github.com/bitcoin/bitcoin/pull/28776)
First part to solving https://github.com/bitcoin-core/gui/issues/769
Functions to enable adding used balance to the overview page for wallets with the avoid_reuse flag
(https://github.com/bitcoin/bitcoin/pull/28776)
First part to solving https://github.com/bitcoin-core/gui/issues/769
Functions to enable adding used balance to the overview page for wallets with the avoid_reuse flag
📝 fanquake opened a pull request: "build: remove `CHECK_ATOMIC`"
(https://github.com/bitcoin/bitcoin/pull/28777)
This is not required to perform a Guix build, which uses GCC 10 (our minimum supported GCC), so if it is still needed for a certain platform / target combination:
> Some versions of gcc/libstdc++ require linking with -latomic if
> using the C++ atomic library.
we should clarify where it is needed in the documentation. Otherwise, we should remove it, and save porting this to CMake.
`libatomic.so.1` is still a runtime dependency for the riscv binaries.
```bash
0c1a2d47ea1e798469d9eb6e7
...
(https://github.com/bitcoin/bitcoin/pull/28777)
This is not required to perform a Guix build, which uses GCC 10 (our minimum supported GCC), so if it is still needed for a certain platform / target combination:
> Some versions of gcc/libstdc++ require linking with -latomic if
> using the C++ atomic library.
we should clarify where it is needed in the documentation. Otherwise, we should remove it, and save porting this to CMake.
`libatomic.so.1` is still a runtime dependency for the riscv binaries.
```bash
0c1a2d47ea1e798469d9eb6e7
...
🤔 glozow reviewed a pull request: "Fee Estimator updates from Validation Interface/CScheduler thread"
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1704427495)
Approach ACK, but some of the changes seem to be interleaved across multiple commits / lumped together in a single commit. For example, `processBlock`'s `txs_removed_for_block` param changes type 3 times in this PR. In an intermediate step where you don't have fee information for a tx, you add a `TxStatsInfo::CAmount` to store it, but then don't delete it at the end when we don't need it anymore.
I'd suggest making this PR 1 change per commit, and 1 commit per change:
- move `removeTx` into
...
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1704427495)
Approach ACK, but some of the changes seem to be interleaved across multiple commits / lumped together in a single commit. For example, `processBlock`'s `txs_removed_for_block` param changes type 3 times in this PR. In an intermediate step where you don't have fee information for a tx, you add a `TxStatsInfo::CAmount` to store it, but then don't delete it at the end when we don't need it anymore.
I'd suggest making this PR 1 change per commit, and 1 commit per change:
- move `removeTx` into
...
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1376404497)
Maybe create a `NewMempoolTransactionInfo` constructor that takes a `CTxMemPoolEntry` as a param instead?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1376404497)
Maybe create a `NewMempoolTransactionInfo` constructor that takes a `CTxMemPoolEntry` as a param instead?
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1376400525)
nit 4986edb99f8aa73f72e87f3bdc09387c3e516197
I don't think these comments are necessary since they're just facts about block transactions.
```suggestion
```
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1376400525)
nit 4986edb99f8aa73f72e87f3bdc09387c3e516197
I don't think these comments are necessary since they're just facts about block transactions.
```suggestion
```
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1376411380)
It's quite confusing to store info about a transaction that isn't new and isn't in the mempool in a struct called `NewMempoolTransactionInfo`. It also has fields that don't apply to a removed transaction - we aren't able to populate any of the validforfeeestimate bools. Why not make separate structs for newly added transactions and removed transactions?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1376411380)
It's quite confusing to store info about a transaction that isn't new and isn't in the mempool in a struct called `NewMempoolTransactionInfo`. It also has fields that don't apply to a removed transaction - we aren't able to populate any of the validforfeeestimate bools. Why not make separate structs for newly added transactions and removed transactions?
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380283948)
You're making this const in 9b0843042ab5c30e6ef4753d590c69c4811d9a70 - why not just introduce it as const?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380283948)
You're making this const in 9b0843042ab5c30e6ef4753d590c69c4811d9a70 - why not just introduce it as const?
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380264004)
nit: This log should probably say "num txs removed=" instead of "txs=".
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380264004)
nit: This log should probably say "num txs removed=" instead of "txs=".
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380269895)
bb22a5148af0f8aad3d8e7b242cc65fe6dd4ced9 this is missing a doxygen comment
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380269895)
bb22a5148af0f8aad3d8e7b242cc65fe6dd4ced9 this is missing a doxygen comment
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380273335)
f03720ee9962b3c4657cd59ce1ea2125ec9b1cbe
what is the purpose of adding `m_fee_per_k` here? it adds 64b to every `TxStatsInfo` we store (note that there is one for every unconfirmed transaction). You can get the same information from the `NewMempoolTransactionInfo` in `processBlock`.
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380273335)
f03720ee9962b3c4657cd59ce1ea2125ec9b1cbe
what is the purpose of adding `m_fee_per_k` here? it adds 64b to every `TxStatsInfo` we store (note that there is one for every unconfirmed transaction). You can get the same information from the `NewMempoolTransactionInfo` in `processBlock`.