📝 polespinasa opened a pull request: "rpc: [PoC] Optionally print feerates in sat/vb"
(https://github.com/bitcoin/bitcoin/pull/33741)
Part of https://github.com/bitcoin/bitcoin/issues/32093
Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB. This PR aims to show a PoC of how we could migrate to sat/vb in a backwards compatible manner.
The RPC affectd by this PR are `getmempoolinfo`, `getnetworkinfo`, `getwalletinfo`, `estimatesmartfee` and `estimaterawfee`.
Because of sub 1sat/vB environment we cannot rely on GetFee() because it internally uses
...
(https://github.com/bitcoin/bitcoin/pull/33741)
Part of https://github.com/bitcoin/bitcoin/issues/32093
Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB. This PR aims to show a PoC of how we could migrate to sat/vb in a backwards compatible manner.
The RPC affectd by this PR are `getmempoolinfo`, `getnetworkinfo`, `getwalletinfo`, `estimatesmartfee` and `estimaterawfee`.
Because of sub 1sat/vB environment we cannot rely on GetFee() because it internally uses
...
💬 polespinasa commented on issue "Migrate from BTC/kvB to sat/vB on RPC and startup options":
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-3465697269)
https://github.com/bitcoin/bitcoin/pull/33741 implements a PoC on how we can print feerates using sat/vB in a backwards compatible way.
Some similar approach could be used for the arguments, where the user can set feerates in sat/vB and it will be interpreted as it only if he also sets satvb flag to true.
That would create inconsistencies with the approach already taken by `fundrawtransaction`. But being honest I don't like the two arguments approach I think the UX is horrible and it can lead
...
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-3465697269)
https://github.com/bitcoin/bitcoin/pull/33741 implements a PoC on how we can print feerates using sat/vB in a backwards compatible way.
Some similar approach could be used for the arguments, where the user can set feerates in sat/vB and it will be interpreted as it only if he also sets satvb flag to true.
That would create inconsistencies with the approach already taken by `fundrawtransaction`. But being honest I don't like the two arguments approach I think the UX is horrible and it can lead
...
🤔 w0xlt reviewed a pull request: "rpc: Optionally print feerates in sat/vb"
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3397062784)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3397062784)
Concept ACK
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3465766818)
> Can you run the commits through clang-format-diff.py? There are a bunch of formatting issues in the first two commits.
Pushed.
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3465766818)
> Can you run the commits through clang-format-diff.py? There are a bunch of formatting issues in the first two commits.
Pushed.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2476143122)
Pushed.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2476143122)
Pushed.
💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#issuecomment-3465768705)
Thanks for catching this, the PR seems correct to me, why is it in draft?
(https://github.com/bitcoin/bitcoin/pull/33738#issuecomment-3465768705)
Thanks for catching this, the PR seems correct to me, why is it in draft?
💬 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.