💬 umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3466146355)
I feel a bit stupid as I just did git pull and it brought some new changes.
Anyway I am still struggling to build successfully. @l0rinc you gave me a good tip. It's using /usr/local/include/boost/ instead of /opt/homebrew/Cellar/boost/1.89.0/include/boost/
Any idea how can switch boost?
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3466146355)
I feel a bit stupid as I just did git pull and it brought some new changes.
Anyway I am still struggling to build successfully. @l0rinc you gave me a good tip. It's using /usr/local/include/boost/ instead of /opt/homebrew/Cellar/boost/1.89.0/include/boost/
Any idea how can switch boost?
💬 trevarj commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2476471723)
@hebasto This builds correctly with `guix build -f [save below to tmp file]`. The `native-inputs` override solves the issue with recursion.
```scheme
(use-modules
(guix packages)
(guix gexp)
(guix utils)
(guix build-system gnu)
(gnu packages mingw))
(define (override-msvcrt pkg)
(package
(inherit pkg)
(name (string-append (package-name pkg) "-ucrt"))
(arguments
(substitute-keyword-arguments (package-arguments pkg)
((#:configure-flags flags)
...
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2476471723)
@hebasto This builds correctly with `guix build -f [save below to tmp file]`. The `native-inputs` override solves the issue with recursion.
```scheme
(use-modules
(guix packages)
(guix gexp)
(guix utils)
(guix build-system gnu)
(gnu packages mingw))
(define (override-msvcrt pkg)
(package
(inherit pkg)
(name (string-append (package-name pkg) "-ucrt"))
(arguments
(substitute-keyword-arguments (package-arguments pkg)
((#:configure-flags flags)
...
💬 umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3466231229)
Oh looks like I made some progress by using:
```
rm -rf build
cmake -B build \
-DCMAKE_C_COMPILER=/opt/homebrew/opt/llvm/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++ \
-DENABLE_IPC=OFF \
-DBUILD_GUI=OFF \
-DWITH_QRENCODE=OFF \
-DENABLE_WALLET=OFF
```
configure logs: https://gist.github.com/umrashrf/dabd3c476da9fa727add2db1522bf3c7
It failed but with a different error.
Build logs: https://gist.github.com/umrashrf/919fbed81177e5829b16bddea6220aae
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3466231229)
Oh looks like I made some progress by using:
```
rm -rf build
cmake -B build \
-DCMAKE_C_COMPILER=/opt/homebrew/opt/llvm/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++ \
-DENABLE_IPC=OFF \
-DBUILD_GUI=OFF \
-DWITH_QRENCODE=OFF \
-DENABLE_WALLET=OFF
```
configure logs: https://gist.github.com/umrashrf/dabd3c476da9fa727add2db1522bf3c7
It failed but with a different error.
Build logs: https://gist.github.com/umrashrf/919fbed81177e5829b16bddea6220aae
✅ umrashrf closed an issue: "Can I compile on OSX Tahoe?"
(https://github.com/bitcoin/bitcoin/issues/33733)
(https://github.com/bitcoin/bitcoin/issues/33733)
💬 umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3466251180)
Doing the following fixed the issue!
```
export LDFLAGS="-L$(brew --prefix llvm)/lib/c++"
export CXXFLAGS="-I$(brew --prefix llvm)/include"
```
Build success 100%
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3466251180)
Doing the following fixed the issue!
```
export LDFLAGS="-L$(brew --prefix llvm)/lib/c++"
export CXXFLAGS="-I$(brew --prefix llvm)/include"
```
Build success 100%
💬 maflcko commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3466299136)
```
# uname --machine && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
riscv64
12cce8584ce64a7e9064e62ca7d1674e59edb8ebfaada9745074f3b873307472 guix-build-a99bbec14500/output/aarch64-linux-gnu/SHA256SUMS.part
c664f6e1c6fc89f822e81c1307ce278e6f53becae398e97b6b708ca2f8b93115 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu-debug.tar.gz
799d4a62daec4fece5c408875ee8fbffdf0856426e84b6
...
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3466299136)
```
# uname --machine && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
riscv64
12cce8584ce64a7e9064e62ca7d1674e59edb8ebfaada9745074f3b873307472 guix-build-a99bbec14500/output/aarch64-linux-gnu/SHA256SUMS.part
c664f6e1c6fc89f822e81c1307ce278e6f53becae398e97b6b708ca2f8b93115 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu-debug.tar.gz
799d4a62daec4fece5c408875ee8fbffdf0856426e84b6
...
💬 TheCharlatan commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#issuecomment-3466336991)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33738#issuecomment-3466336991)
Concept ACK
💬 maflcko commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476602340)
in theory this seems racy, when the logging is toggled in another thread, but this is probably good enough to ignore.
Though, on a different note, I wonder if there are other places that could benefit from CTransaction caching. Two full hashes are cached, so caching the size should be cheap in both time and space?
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476602340)
in theory this seems racy, when the logging is toggled in another thread, but this is probably good enough to ignore.
Though, on a different note, I wonder if there are other places that could benefit from CTransaction caching. Two full hashes are cached, so caching the size should be cheap in both time and space?
🤔 maflcko reviewed a pull request: "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled"
(https://github.com/bitcoin/bitcoin/pull/33738#pullrequestreview-3397666497)
is there a benchmark that detects this?
(https://github.com/bitcoin/bitcoin/pull/33738#pullrequestreview-3397666497)
is there a benchmark that detects this?
💬 l0rinc commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476611373)
the hash is actually eagerly calculated as well - which should also be hidden behind a similar condition, was hoping someone will bring it up :)
----
there aren't any benchmarks for this but a ton of tests failed for the throw check above, so at lest it has coverage.
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476611373)
the hash is actually eagerly calculated as well - which should also be hidden behind a similar condition, was hoping someone will bring it up :)
----
there aren't any benchmarks for this but a ton of tests failed for the throw check above, so at lest it has coverage.
👋 l0rinc's pull request is ready for review: "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled"
(https://github.com/bitcoin/bitcoin/pull/33738)
(https://github.com/bitcoin/bitcoin/pull/33738)
💬 maflcko commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466385489)
I don't think you did. The conflicts are still there and the DrahtBot summary is correct. You can also try it locally:
```
# git checkout b97db6f06185a1d63828f958b33a7e6780aee73c && git merge 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a
HEAD is now at b97db6f061 refactor: unify container presence checks - non-trivial counts
Auto-merging src/init.cpp
Auto-merging src/net_processing.cpp
Auto-merging src/node/interfaces.cpp
Auto-merging src/node/miner.cpp
Auto-merging src/rpc/mempool.cpp
Au
...
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466385489)
I don't think you did. The conflicts are still there and the DrahtBot summary is correct. You can also try it locally:
```
# git checkout b97db6f06185a1d63828f958b33a7e6780aee73c && git merge 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a
HEAD is now at b97db6f061 refactor: unify container presence checks - non-trivial counts
Auto-merging src/init.cpp
Auto-merging src/net_processing.cpp
Auto-merging src/node/interfaces.cpp
Auto-merging src/node/miner.cpp
Auto-merging src/rpc/mempool.cpp
Au
...
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476660588)
It could be, but this is safe as-is, because the `[]` operator defined on the Chain does the check for us.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476660588)
It could be, but this is safe as-is, because the `[]` operator defined on the Chain does the check for us.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476663196)
Unlike many of the other hashes, we do cache the transaction hashes internally. Since a lookup by txid is a fairly common operation, I think it is fair that we surface this optimization to developers using the API as well.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476663196)
Unlike many of the other hashes, we do cache the transaction hashes internally. Since a lookup by txid is a fairly common operation, I think it is fair that we surface this optimization to developers using the API as well.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3466427632)
> Are the assert statements in src/kernel/bitcoinkernel.cpp really necessary, or could they be replaced with runtime validation?
Replacing them with validation and returning an error works too. However that does not solve the problem of handling assertions within the code, since we have many places within validation that assert on an expected invariant. They are supposed to guard against mistakes from the programmer. Have you tried stubbing out the assertion code / header in a similar way to
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3466427632)
> Are the assert statements in src/kernel/bitcoinkernel.cpp really necessary, or could they be replaced with runtime validation?
Replacing them with validation and returning an error works too. However that does not solve the problem of handling assertions within the code, since we have many places within validation that assert on an expected invariant. They are supposed to guard against mistakes from the programmer. Have you tried stubbing out the assertion code / header in a similar way to
...
🤔 maflcko reviewed a pull request: "rpc: Optionally print feerates in sat/vb"
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3397729983)
I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
There could be a global option to toggle the default, to improve the UX.
Though, please don't use floating point types. For monetary values, a fixed point type should be used.
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3397729983)
I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
There could be a global option to toggle the default, to improve the UX.
Though, please don't use floating point types. For monetary values, a fixed point type should be used.
💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476651745)
Please don't encode default values in how the code is written. This makes it impossible to change the default values without rewriting the code logic.
Also, could use `self.(Maybe)Arg<bool>` instead?
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476651745)
Please don't encode default values in how the code is written. This makes it impossible to change the default values without rewriting the code logic.
Also, could use `self.(Maybe)Arg<bool>` instead?
💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476689034)
this is wrong, you can't use `double` to denote monetary amounts. `double` is a floating point type, which can not represent decimal values accurately.
For example, if `GetFeePerK()` returns `CAmount{665714}`, the Univalue will be `665.7140000000001`, which is confusing at best, but also wrong.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476689034)
this is wrong, you can't use `double` to denote monetary amounts. `double` is a floating point type, which can not represent decimal values accurately.
For example, if `GetFeePerK()` returns `CAmount{665714}`, the Univalue will be `665.7140000000001`, which is confusing at best, but also wrong.
💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466597361)
Thanks, looks like a missed a few manual undos.
Reverted the 3 remaining files that seem related to the cluster mempool work and verified that it merges cleanly with https://github.com/bitcoin/bitcoin/pull/33629 and https://github.com/bitcoin/bitcoin/pull/33591
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466597361)
Thanks, looks like a missed a few manual undos.
Reverted the 3 remaining files that seem related to the cluster mempool work and verified that it merges cleanly with https://github.com/bitcoin/bitcoin/pull/33629 and https://github.com/bitcoin/bitcoin/pull/33591
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710)
Good question. I wanted to keep it limited to member fields (and their constructors) only. Otherwise, there'd be way too many function calls to touch. Happy to do those in a follow-up. Also, happy to drop the `src/node/blockstorage.cpp` changes from this commit, to keep it more focussed on just member fields.
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710)
Good question. I wanted to keep it limited to member fields (and their constructors) only. Otherwise, there'd be way too many function calls to touch. Happy to do those in a follow-up. Also, happy to drop the `src/node/blockstorage.cpp` changes from this commit, to keep it more focussed on just member fields.