💬 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`.
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380289810)
9b0843042ab5c30e6ef4753d590c69c4811d9a70 seems to be doing multiple things: (1) making `CBlockPolicyEstimator` react to validationinterface and (2) delegating the `validForFeeEstimation` from validation to the `CBlockPolicyEstimator` by providing these bools in the `TransactionAddedToMempool` signal (but this isn't fully done until the next commit which cleans things up).
These should be separate commits, e.g. (2) and then (1).
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380289810)
9b0843042ab5c30e6ef4753d590c69c4811d9a70 seems to be doing multiple things: (1) making `CBlockPolicyEstimator` react to validationinterface and (2) delegating the `validForFeeEstimation` from validation to the `CBlockPolicyEstimator` by providing these bools in the `TransactionAddedToMempool` signal (but this isn't fully done until the next commit which cleans things up).
These should be separate commits, e.g. (2) and then (1).
💬 glozow commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380307655)
This is quite verbose and repeated in several places. Why not create a dedicated constructor?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380307655)
This is quite verbose and repeated in several places. Why not create a dedicated constructor?
💬 maflcko commented on issue "Undefined behavior in AutoFile::write (gcc only)":
(https://github.com/bitcoin/bitcoin/issues/28761#issuecomment-1790993773)
Also, a suppression can be added in the meantime, if anyone is using g++
(https://github.com/bitcoin/bitcoin/issues/28761#issuecomment-1790993773)
Also, a suppression can be added in the meantime, if anyone is using g++
💬 glozow commented on pull request "Fuzz: Check individual and package transaction invariants":
(https://github.com/bitcoin/bitcoin/pull/28764#issuecomment-1791000546)
reACK fcb3069fa307942cf7f3edabcda1be96d615c91f, only whitespace changes
(https://github.com/bitcoin/bitcoin/pull/28764#issuecomment-1791000546)
reACK fcb3069fa307942cf7f3edabcda1be96d615c91f, only whitespace changes
💬 maflcko commented on pull request "build: remove `CHECK_ATOMIC`":
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1791001127)
It may be good to get matching guix hashes from different arches before merging this?
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1791001127)
It may be good to get matching guix hashes from different arches before merging this?
💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380403135)
these fields were added to solve silent merge conflict https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1713512238 , but you are right they should be added when needed.
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380403135)
these fields were added to solve silent merge conflict https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1713512238 , but you are right they should be added when needed.
💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380403540)
This is fixed in latest push.
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1380403540)
This is fixed in latest push.
💬 fanquake commented on pull request "build: remove `CHECK_ATOMIC`":
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1791049082)
Looks like it's still required for at least 32-bit Linux, using GCC 11.
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1791049082)
Looks like it's still required for at least 32-bit Linux, using GCC 11.