📝 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.
💬 fanquake commented on pull request "build: remove `CHECK_ATOMIC`":
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1791057989)
Although not always, because the 32-bit CentOS build using GCC 11 compiled fine. Will adjust the docs.
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1791057989)
Although not always, because the 32-bit CentOS build using GCC 11 compiled fine. Will adjust the docs.
📝 fanquake converted_to_draft 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
...
📝 fanquake opened a pull request: "depends: drop -O1 workaround from arm64 apple Qt build"
(https://github.com/bitcoin/bitcoin/pull/28778)
Drop the workaround of setting optimization flags to -O1 for the `arm64-apple-darwin` builds. I no-longer see reproducibility issues when building across `x86_64` and `aarch64`:
```bash
real 7m21.192s
user 67m41.047s
sys 5m8.596s
fedora-32gb-hel1-1 bitcoin]# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
62373549d2884e8ef8f46a77b9a93f64ebfc88603569e9d33b68fc67beaf2226 guix-build-664c87354f9e/output/arm64-apple-darwin/S
...
(https://github.com/bitcoin/bitcoin/pull/28778)
Drop the workaround of setting optimization flags to -O1 for the `arm64-apple-darwin` builds. I no-longer see reproducibility issues when building across `x86_64` and `aarch64`:
```bash
real 7m21.192s
user 67m41.047s
sys 5m8.596s
fedora-32gb-hel1-1 bitcoin]# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
62373549d2884e8ef8f46a77b9a93f64ebfc88603569e9d33b68fc67beaf2226 guix-build-664c87354f9e/output/arm64-apple-darwin/S
...