💬 hodlinator commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2056049582)
I think it might make sense to introduce our own version of `clamp` that includes such an `assert` since the standard library has no guarantees if cppreference is to be believed. (Or just start off with an `assert`/`Assume` before the call for now).
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2056049582)
I think it might make sense to introduce our own version of `clamp` that includes such an `assert` since the standard library has no guarantees if cppreference is to be believed. (Or just start off with an `assert`/`Assume` before the call for now).
📝 nervana21 opened a pull request: "doc: add missing top-level description to pruneblockchain RPC"
(https://github.com/bitcoin/bitcoin/pull/32330)
Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.
This adds a concise description noting that the method deletes block and undo data, requires `-prune` to be enabled at startup, and is irreversible.
(https://github.com/bitcoin/bitcoin/pull/32330)
Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.
This adds a concise description noting that the method deletes block and undo data, requires `-prune` to be enabled at startup, and is irreversible.
✅ nervana21 closed a pull request: "doc: add missing top-level description to pruneblockchain RPC"
(https://github.com/bitcoin/bitcoin/pull/32330)
(https://github.com/bitcoin/bitcoin/pull/32330)
⚠️ fanquake opened an issue: "cmake: GUI build broken with `-stdlib=libc++`"
(https://github.com/bitcoin/bitcoin/issues/32331)
```bash
master @ 9a4c92eb9ac29204df3d826f5ae526ab16b8ad65
clang version 20.1.2 (Fedora 20.1.2-3.fc42)
cmake -B build -DBUILD_GUI=ON -DAPPEND_CXXFLAGS="-stdlib=libc++" -DAPPEND_LDFLAGS="-stdlib=libc++"
cmake --build build
<snip>
cmake --build build -j17 --target bitcoin-qt
[ 0%] Generating bitcoin-build-info.h
[ 0%] Built target secp256k1_precomputed
[ 0%] Built target crc32c
[ 1%] Built target bitcoin_consensus
[ 3%] Built target bitcoin_cli
[ 4%] Built target univalue
[ 7%] Built target
...
(https://github.com/bitcoin/bitcoin/issues/32331)
```bash
master @ 9a4c92eb9ac29204df3d826f5ae526ab16b8ad65
clang version 20.1.2 (Fedora 20.1.2-3.fc42)
cmake -B build -DBUILD_GUI=ON -DAPPEND_CXXFLAGS="-stdlib=libc++" -DAPPEND_LDFLAGS="-stdlib=libc++"
cmake --build build
<snip>
cmake --build build -j17 --target bitcoin-qt
[ 0%] Generating bitcoin-build-info.h
[ 0%] Built target secp256k1_precomputed
[ 0%] Built target crc32c
[ 1%] Built target bitcoin_consensus
[ 3%] Built target bitcoin_cli
[ 4%] Built target univalue
[ 7%] Built target
...
💬 glozow commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824360562)
Concept NACK - even if there are good reasons to use conservative fee estimation, using a config option instead of the RPC parameter is more rigid, requires coordination with the node runner, wouldn't be available for another 6 months, and further complicates the current mess of config options. Passing a 'conservative' arg should be a one-line change to their source code and is already supported by all maintained versions of Core.
> While I do not have specific details on their reasons, the k
...
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824360562)
Concept NACK - even if there are good reasons to use conservative fee estimation, using a config option instead of the RPC parameter is more rigid, requires coordination with the node runner, wouldn't be available for another 6 months, and further complicates the current mess of config options. Passing a 'conservative' arg should be a one-line change to their source code and is already supported by all maintained versions of Core.
> While I do not have specific details on their reasons, the k
...
💬 hebasto commented on issue "cmake: GUI build broken with `-stdlib=libc++`":
(https://github.com/bitcoin/bitcoin/issues/32331#issuecomment-2824399437)
Is it supposed to work without using depends built with `-stdlib=libc++`?
(https://github.com/bitcoin/bitcoin/issues/32331#issuecomment-2824399437)
Is it supposed to work without using depends built with `-stdlib=libc++`?
💬 Sjors commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#issuecomment-2824428654)
@l0rinc I was rebasing a branch that used the old name and didn't know what it was renamed too, but otherwise no problem.
(https://github.com/bitcoin/bitcoin/pull/31490#issuecomment-2824428654)
@l0rinc I was rebasing a branch that used the old name and didn't know what it was renamed too, but otherwise no problem.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056077020)
That's better. But I still think it's redundant as the code block now ends with
```C++
m_obfuscation = obfuscate_key_vector;
```
and all code paths lead there.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056077020)
That's better. But I still think it's redundant as the code block now ends with
```C++
m_obfuscation = obfuscate_key_vector;
```
and all code paths lead there.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056150490)
```suggestion
const bool all_zero = !obfuscation || std::ranges::all_of(std::span{key_bytes.begin(), write_size}, [](auto& b) { return b == std::byte{0}; });
```
```suggestion
const bool all_zero = !obfuscation || std::all_of(key_bytes.begin(), key_bytes.begin() + write_size, [](auto& b) { return b == std::byte{0}; });
```
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056150490)
```suggestion
const bool all_zero = !obfuscation || std::ranges::all_of(std::span{key_bytes.begin(), write_size}, [](auto& b) { return b == std::byte{0}; });
```
```suggestion
const bool all_zero = !obfuscation || std::all_of(key_bytes.begin(), key_bytes.begin() + write_size, [](auto& b) { return b == std::byte{0}; });
```
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056093869)
Seems you forgot to remove "The `CreateObfuscateKey` method and its private helper were also removed."?
I think saying CreateObfuscateKey was inlined is sufficient and no private helper was removed as stated above in (1.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056093869)
Seems you forgot to remove "The `CreateObfuscateKey` method and its private helper were also removed."?
I think saying CreateObfuscateKey was inlined is sufficient and no private helper was removed as stated above in (1.
🤔 stickies-v reviewed a pull request: "Add rpcestimateconservativefees"
(https://github.com/bitcoin/bitcoin/pull/32329#pullrequestreview-2787516215)
Concept NACK, for reasons outlined by @glozow and @ismaelsadeeq - this seems like way too niche of a use case to add the maintenance burden and UX confusion of a new RPC/startup option into Bitcoin Core.
I think you can easily solve this by running a small JSON-RPC proxy that listens on the usual RPC port, injects a `estimate_mode` argument if none is provided and forwards that to the `bitcoind` instance? Then your clients won't need to make any changes for as long as you're willing to mainta
...
(https://github.com/bitcoin/bitcoin/pull/32329#pullrequestreview-2787516215)
Concept NACK, for reasons outlined by @glozow and @ismaelsadeeq - this seems like way too niche of a use case to add the maintenance burden and UX confusion of a new RPC/startup option into Bitcoin Core.
I think you can easily solve this by running a small JSON-RPC proxy that listens on the usual RPC port, injects a `estimate_mode` argument if none is provided and forwards that to the `bitcoind` instance? Then your clients won't need to make any changes for as long as you're willing to mainta
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056174501)
Yeah, can you tell me what's wrong with it? If you have a suggestion that passes ci (and local IBD for some blocks), let me know
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056174501)
Yeah, can you tell me what's wrong with it? If you have a suggestion that passes ci (and local IBD for some blocks), let me know
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056177529)
I had that one coming. :)
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056177529)
I had that one coming. :)
💬 furszy commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824481962)
> I think you can easily solve this by running a small JSON-RPC proxy that listens on the usual RPC port, injects a estimate_mode argument if none is provided and forwards that to the bitcoind instance? Then your clients won't need to make any changes for as long as you're willing to maintain backwards compatibility.
Or.. while the clients adapt, simpler to just downgrade or compile the repository yourself.
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824481962)
> I think you can easily solve this by running a small JSON-RPC proxy that listens on the usual RPC port, injects a estimate_mode argument if none is provided and forwards that to the bitcoind instance? Then your clients won't need to make any changes for as long as you're willing to maintain backwards compatibility.
Or.. while the clients adapt, simpler to just downgrade or compile the repository yourself.
🤔 rkrux reviewed a pull request: "test: Test that migration automatically repairs corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2787605306)
LGTM to me at bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633, ACK - it's just one functional test now.
Couple of observations/questions after rotating the seed and unloading the wallet that I found noteworthy enough to share here.
1. The `listreceivedbyaddress` RPC doesn't show any entry for `addr` to which the 1 BTC was sent, but `listunspent` RPC does show.
2. The `desc` field in the `bad_deriv_wallet.listunspent` and `bad_deriv_wallet_master.listunspent` calls for the same `addr` was diff
...
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2787605306)
LGTM to me at bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633, ACK - it's just one functional test now.
Couple of observations/questions after rotating the seed and unloading the wallet that I found noteworthy enough to share here.
1. The `listreceivedbyaddress` RPC doesn't show any entry for `addr` to which the 1 BTC was sent, but `listunspent` RPC does show.
2. The `desc` field in the `bad_deriv_wallet.listunspent` and `bad_deriv_wallet_master.listunspent` calls for the same `addr` was diff
...
💬 maflcko commented on issue "cmake: GUI build broken with `-stdlib=libc++`":
(https://github.com/bitcoin/bitcoin/issues/32331#issuecomment-2824604630)
IIRC it used to work with qt5, but it is unclear if that was intentional or by accident. I guess in any case this is an upstream issue that would need to be fixed in the distro or in qt6, if it is an issue at all?
(https://github.com/bitcoin/bitcoin/issues/32331#issuecomment-2824604630)
IIRC it used to work with qt5, but it is unclear if that was intentional or by accident. I guess in any case this is an upstream issue that would need to be fixed in the distro or in qt6, if it is an issue at all?
💬 fanquake commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824608069)
Agree with the other comments here. Closing for now.
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824608069)
Agree with the other comments here. Closing for now.
✅ fanquake closed a pull request: "Add rpcestimateconservativefees"
(https://github.com/bitcoin/bitcoin/pull/32329)
(https://github.com/bitcoin/bitcoin/pull/32329)
💬 juanmigdr commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824637123)
You're right, @ismaelsadeeq, @glozow, @stickies-v, @furszy, and @fanquake. Thanks for the feedback and suggestions. I see that adding this config option isn't the best solution, as this change is quite specific to my situation, and the cost of maintaining it outweighs the benefits. I'll consider alternative ways to handle this. Appreciate the help!
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824637123)
You're right, @ismaelsadeeq, @glozow, @stickies-v, @furszy, and @fanquake. Thanks for the feedback and suggestions. I see that adding this config option isn't the best solution, as this change is quite specific to my situation, and the cost of maintaining it outweighs the benefits. I'll consider alternative ways to handle this. Appreciate the help!
💬 hebasto commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2824676516)
@whitslack @eli-schwartz @thesamesam
Setting the required conditions aside, can you confirm whether the following patch resolves the issue on Gentoo:
```diff
--- a/src/qt/CMakeLists.txt
+++ b/src/qt/CMakeLists.txt
@@ -250,6 +250,8 @@ if(qt_lib_type STREQUAL "STATIC_LIBRARY")
)
endif()
+list(TRANSFORM CMAKE_CXX_LINK_OPTIONS_PIE REPLACE "${CMAKE_CXX_COMPILE_OPTIONS_PIE}" "${CMAKE_CXX_COMPILE_OPTIONS_PIC}")
+
add_executable(bitcoin-qt
main.cpp
../init/bitcoin-qt.cpp
```
?
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2824676516)
@whitslack @eli-schwartz @thesamesam
Setting the required conditions aside, can you confirm whether the following patch resolves the issue on Gentoo:
```diff
--- a/src/qt/CMakeLists.txt
+++ b/src/qt/CMakeLists.txt
@@ -250,6 +250,8 @@ if(qt_lib_type STREQUAL "STATIC_LIBRARY")
)
endif()
+list(TRANSFORM CMAKE_CXX_LINK_OPTIONS_PIE REPLACE "${CMAKE_CXX_COMPILE_OPTIONS_PIE}" "${CMAKE_CXX_COMPILE_OPTIONS_PIC}")
+
add_executable(bitcoin-qt
main.cpp
../init/bitcoin-qt.cpp
```
?