💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2824224700)
Hi @furszy could you please finalize this review and merge this.
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2824224700)
Hi @furszy could you please finalize this review and merge this.
💬 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. :)