💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2306149303)
Thank you!
It appears that we are no longer testing the case of `send 10 BTC with fee subtracted`, nor the case of `send 1 BTC with a higher fee rate`, since we are not specifying a fee in send_to().
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2306149303)
Thank you!
It appears that we are no longer testing the case of `send 10 BTC with fee subtracted`, nor the case of `send 1 BTC with a higher fee rate`, since we are not specifying a fee in send_to().
💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3231877768)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3231877768)
Concept ACK
💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306188488)
Could make it `the` instead of `The` for consistency with other options. Could consider doing the same for the main mediantime help text that this was copied from too.
As far as ordering goes, probably copy the ordering of the fields in `getblockchaininfo`?
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306188488)
Could make it `the` instead of `The` for consistency with other options. Could consider doing the same for the main mediantime help text that this was copied from too.
As far as ordering goes, probably copy the ordering of the fields in `getblockchaininfo`?
💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306180631)
Can just drop this field, and use the presence/absence of the object to indicate where background validation is occurring?
If not, "estimate of" isn't accurate here -- the node knows whether it's doing background validation or not.
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306180631)
Can just drop this field, and use the presence/absence of the object to indicate where background validation is occurring?
If not, "estimate of" isn't accurate here -- the node knows whether it's doing background validation or not.
💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306222771)
Should be able to test the field-exists case via feature_assumeutxo.py probably just prior to the invocation of `n1.submitblock(snapshot_block)`
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306222771)
Should be able to test the field-exists case via feature_assumeutxo.py probably just prior to the invocation of `n1.submitblock(snapshot_block)`
💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306215222)
This is telling you how far the background has progressed if it were going to go all the way to the current tip; but it will actually stop when it hits the snapshot block. So I think this should be calling a new method `chainman.GetBackgroundVerificationProgress(btip)`, something like:
```c++
double ChainstateManager::GetBackgroundVerificationProgress(const CBlockIndex* tip)
{
if (tip == nullptr) return 0.0;
auto base = GetSnapshotBaseBlock();
if (base == nullptr || base->m_
...
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306215222)
This is telling you how far the background has progressed if it were going to go all the way to the current tip; but it will actually stop when it hits the snapshot block. So I think this should be calling a new method `chainman.GetBackgroundVerificationProgress(btip)`, something like:
```c++
double ChainstateManager::GetBackgroundVerificationProgress(const CBlockIndex* tip)
{
if (tip == nullptr) return 0.0;
auto base = GetSnapshotBaseBlock();
if (base == nullptr || base->m_
...
💬 maflcko commented on pull request "threading: reduce the scope of lock in getblocktemplate":
(https://github.com/bitcoin/bitcoin/pull/33264#issuecomment-3232037034)
lgtm ACK 06ec4809045f5cbe51d54e8eda08db03ca4d5ca1
(https://github.com/bitcoin/bitcoin/pull/33264#issuecomment-3232037034)
lgtm ACK 06ec4809045f5cbe51d54e8eda08db03ca4d5ca1
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3232064557)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3232064557)
Are you still working on this?
💬 maflcko commented on pull request "ci: use LLVM 21":
(https://github.com/bitcoin/bitcoin/pull/33258#issuecomment-3232102351)
lgtm ACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4
(https://github.com/bitcoin/bitcoin/pull/33258#issuecomment-3232102351)
lgtm ACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4
👍 l0rinc approved a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258#pullrequestreview-3163409751)
utACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4
https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.0 was just released 2 days ago
(https://github.com/bitcoin/bitcoin/pull/33258#pullrequestreview-3163409751)
utACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4
https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.0 was just released 2 days ago
💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3232177376)
Yep. But been on holiday this week and last.
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3232177376)
Yep. But been on holiday this week and last.
🤔 BrandonOdiwuor reviewed a pull request: "Update `minisketch` subtree and switch to its build script"
(https://github.com/bitcoin/bitcoin/pull/32856#pullrequestreview-3163505949)
ACK a6b48f009f7bf88bc1f15ed9490276baa898884e
Verified migration to `minisketch` upstream CMake build script. Successfully built `minisketch` and `Bitcoin Core` on macOS 15.6 (Clang 17, CMake 4.0.2) and Ubuntu 24.04 (GCC 13, CMake 3.28.3)
**macOS**
```
$ cmake --build build -j9
```
<img width="1488" height="224" alt="Screenshot 2025-08-28 at 09 58 53" src="https://github.com/user-attachments/assets/538d6bc0-d55c-444d-9a81-4194678379dc" />
**Ubuntu**
```
$ cmake --build build -j9
`
...
(https://github.com/bitcoin/bitcoin/pull/32856#pullrequestreview-3163505949)
ACK a6b48f009f7bf88bc1f15ed9490276baa898884e
Verified migration to `minisketch` upstream CMake build script. Successfully built `minisketch` and `Bitcoin Core` on macOS 15.6 (Clang 17, CMake 4.0.2) and Ubuntu 24.04 (GCC 13, CMake 3.28.3)
**macOS**
```
$ cmake --build build -j9
```
<img width="1488" height="224" alt="Screenshot 2025-08-28 at 09 58 53" src="https://github.com/user-attachments/assets/538d6bc0-d55c-444d-9a81-4194678379dc" />
**Ubuntu**
```
$ cmake --build build -j9
`
...
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2306467573)
[17:54:00.577] = help: Remove unused import: `decimal.Decimal`
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2306467573)
[17:54:00.577] = help: Remove unused import: `decimal.Decimal`
💬 maflcko commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306531752)
> iterator
That doesn't work, because it is just what the current code does: Take a port and `+=1` it. The goal of this pull request is to properly derive (and check) each port by its own.
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306531752)
> iterator
That doesn't work, because it is just what the current code does: Take a port and `+=1` it. The goal of this pull request is to properly derive (and check) each port by its own.
💬 maflcko commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306533787)
thx, but I want to keep the changes here minimal, not rewrite the whole test
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306533787)
thx, but I want to keep the changes here minimal, not rewrite the whole test
💬 purpleKarrot commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306512554)
This option is useless and can be removed from minisketch because passing [`EXCLUDE_FROM_ALL`](https://cmake.org/cmake/help/latest/prop_dir/EXCLUDE_FROM_ALL.html) to `add_subdirectory` has the effect that
> Any install rules defined in the subdirectory or below will be ignored when installing the parent directory.
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306512554)
This option is useless and can be removed from minisketch because passing [`EXCLUDE_FROM_ALL`](https://cmake.org/cmake/help/latest/prop_dir/EXCLUDE_FROM_ALL.html) to `add_subdirectory` has the effect that
> Any install rules defined in the subdirectory or below will be ignored when installing the parent directory.
💬 purpleKarrot commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306490269)
Why?
We should not interfere with `CMAKE_EXPORT_COMPILE_COMMANDS`. If developers set this option globally in their dotfiles because it will give them code completion in their IDE, setting it to `OFF` here will break their setup.
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306490269)
Why?
We should not interfere with `CMAKE_EXPORT_COMPILE_COMMANDS`. If developers set this option globally in their dotfiles because it will give them code completion in their IDE, setting it to `OFF` here will break their setup.
💬 purpleKarrot commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306553108)
I find it a bit awkward to define this as a function in a module, when it is not really providing reusable functionality. The function may only called in a single place and there is only one valid argument.
A better approach would be to get rid of the `../cmake/minisketch.cmake` module and define the function inline without any arguments. Then, add a comment that makes clear that this is a workaround and also notes when it will be replaced:
```cmake
// TODO: use `block()` once the minimum cmak
...
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306553108)
I find it a bit awkward to define this as a function in a module, when it is not really providing reusable functionality. The function may only called in a single place and there is only one valid argument.
A better approach would be to get rid of the `../cmake/minisketch.cmake` module and define the function inline without any arguments. Then, add a comment that makes clear that this is a workaround and also notes when it will be replaced:
```cmake
// TODO: use `block()` once the minimum cmak
...
💬 maflcko commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#issuecomment-3232540549)
I expect those to be widely used (not only in the internal tests, as you noticed in the repeated force pushes), so this may or may not be a widely breaking change (depending on how much they are used in active projects).
There is no auto-generated schema or RPC interface, so those projects would have to fix them manually.
No strong opinion, but maybe this can be closed for now, and revisited when there is a schema generator?
(https://github.com/bitcoin/bitcoin/pull/33214#issuecomment-3232540549)
I expect those to be widely used (not only in the internal tests, as you noticed in the repeated force pushes), so this may or may not be a widely breaking change (depending on how much they are used in active projects).
There is no auto-generated schema or RPC interface, so those projects would have to fix them manually.
No strong opinion, but maybe this can be closed for now, and revisited when there is a schema generator?
💬 maflcko commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2306701610)
“for a wallet transactions” → “for wallet transactions” [“a” makes the noun plural ‘transactions’ ungrammatical]
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2306701610)
“for a wallet transactions” → “for wallet transactions” [“a” makes the noun plural ‘transactions’ ungrammatical]