💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2056007024)
https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055969055
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2056007024)
https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055969055
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2824233359)
Hi All @achow101 @furszy @mprenditore @maflcko @w0xlt @davidrobinsonau request to re-review this PR and provide ACK.
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2824233359)
Hi All @achow101 @furszy @mprenditore @maflcko @w0xlt @davidrobinsonau request to re-review this PR and provide ACK.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2056022026)
Yes, this loose handling makes it possible to do the job with less code. My reasoning is that the worst outcome from the race is acceptable.
I am looking into keeping track of more stuff with respect to the future RPC stats (see at the bottom of https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2042286270) which would also give us more fine grained control here. If it looks reasonable and is not too much code, I will propose the change in https://github.com/bitcoin/bitcoin/pull/29415#
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2056022026)
Yes, this loose handling makes it possible to do the job with less code. My reasoning is that the worst outcome from the race is acceptable.
I am looking into keeping track of more stuff with respect to the future RPC stats (see at the bottom of https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2042286270) which would also give us more fine grained control here. If it looks reasonable and is not too much code, I will propose the change in https://github.com/bitcoin/bitcoin/pull/29415#
...
💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2056025288)
Good catch, thanks. I should have re-checked with my follow-up PR which introduces validation before pushing the change for the fuzz target. Will do that this time.
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2056025288)
Good catch, thanks. I should have re-checked with my follow-up PR which introduces validation before pushing the change for the fuzz target. Will do that this time.
💬 fanquake commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2824264910)
Re-ran tidy as it looks like 20 will pickup more issues: https://cirrus-ci.com/task/5041587878625280?logs=ci#L1916:
```bash
[09:08:35.465] /ci_container_base/src/rpc/util.h:527:50: error: parameter 'pow_limit' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls,-warnings-as-errors]
[09:08:35.465] 527 | uint256 GetTarget(const CBlockIndex& blockindex, const uint256 pow_limit);
`
...
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2824264910)
Re-ran tidy as it looks like 20 will pickup more issues: https://cirrus-ci.com/task/5041587878625280?logs=ci#L1916:
```bash
[09:08:35.465] /ci_container_base/src/rpc/util.h:527:50: error: parameter 'pow_limit' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls,-warnings-as-errors]
[09:08:35.465] 527 | uint256 GetTarget(const CBlockIndex& blockindex, const uint256 pow_limit);
`
...
💬 hodlinator commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2056032514)
> you mean we usually ignore the folder when sorting?
Think it's a case of keeping our headers separate from external dependencies/libraries.
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2056032514)
> you mean we usually ignore the folder when sorting?
Think it's a case of keeping our headers separate from external dependencies/libraries.
💬 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.