💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120844393)
Using `const vector&` communicates that we are sending in the entire chain, not some subset. I like the specificity. `const vector`s feel more neutered than spans IMO.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120844393)
Using `const vector&` communicates that we are sending in the entire chain, not some subset. I like the specificity. `const vector`s feel more neutered than spans IMO.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120833106)
Is there anything to catch wrong order?
```C++
const auto [pow_validated_headers, success, request_more] = ...
```
vs
```C++
const auto [pow_validated_headers, request_more, success] = ...
```
(There's clang-tidy logic to catch `/*paramname=*/true` mismatches for function calls).
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120833106)
Is there anything to catch wrong order?
```C++
const auto [pow_validated_headers, success, request_more] = ...
```
vs
```C++
const auto [pow_validated_headers, request_more, success] = ...
```
(There's clang-tidy logic to catch `/*paramname=*/true` mismatches for function calls).
⚠️ ismaelsadeeq opened an issue: "wallet, node: Redundant opt-in RBF wallet config and RPC parameters"
(https://github.com/bitcoin/bitcoin/issues/32661)
Following the removal of the `-mempoolfullrbf` node option in [#30592](https://github.com/bitcoin/bitcoin/pull/30592), the Bitcoin Core wallet still retains the `-walletrbf` option.
This option determines whether transactions are marked as opt-in RBF (i.e., using sequence numbers `MAX - 1` or `MAX - 2`).
With full-RBF now the default behavior, `-walletrbf` is effectively redundant and should be removed.
However, removing `-walletrbf` alone is not sufficient, as several wallet RPCs also suppor
...
(https://github.com/bitcoin/bitcoin/issues/32661)
Following the removal of the `-mempoolfullrbf` node option in [#30592](https://github.com/bitcoin/bitcoin/pull/30592), the Bitcoin Core wallet still retains the `-walletrbf` option.
This option determines whether transactions are marked as opt-in RBF (i.e., using sequence numbers `MAX - 1` or `MAX - 2`).
With full-RBF now the default behavior, `-walletrbf` is effectively redundant and should be removed.
However, removing `-walletrbf` alone is not sufficient, as several wallet RPCs also suppor
...
🤔 dergoegge reviewed a pull request: "p2p: Add mutation check inside compact block's FillBlock"
(https://github.com/bitcoin/bitcoin/pull/32646#pullrequestreview-2888127199)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32646#pullrequestreview-2888127199)
Concept ACK
💬 dergoegge commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2120969666)
nit
```suggestion
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))};
```
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2120969666)
nit
```suggestion
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))};
```
💬 mabu44 commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121017491)
The third parameter should be 0, not false. The same apply for all occurrences of the AddCoins function.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121017491)
The third parameter should be 0, not false. The same apply for all occurrences of the AddCoins function.
📝 hebasto opened a pull request: "doc: Remove build instruction for running `clang-tidy`"
(https://github.com/bitcoin/bitcoin/pull/32662)
One of the benefits of using a compilation database, which is available after the CMake build system generation step, is that it is not necessary to actually build the code in order to run `clang-tidy`.
(https://github.com/bitcoin/bitcoin/pull/32662)
One of the benefits of using a compilation database, which is available after the CMake build system generation step, is that it is not necessary to actually build the code in order to run `clang-tidy`.
💬 ismaelsadeeq commented on pull request "init: deprecate `-blockmaxweight` startup option":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2930558750)
> (TBH, I think these options should move into the template request, rather than being node startup options)
Yes, the `BlockCreateOptions` have `block_reserved_weight` variable. From recent @Sjors comment I think there's no need for the `blockmaxweight` option for TP that's why it was not added.
It makes sense to add these options to the template request RPC parameter as well, since test networks and other protocols still rely on the RPC feels natural for this to be runtime option.
My
...
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2930558750)
> (TBH, I think these options should move into the template request, rather than being node startup options)
Yes, the `BlockCreateOptions` have `block_reserved_weight` variable. From recent @Sjors comment I think there's no need for the `blockmaxweight` option for TP that's why it was not added.
It makes sense to add these options to the template request RPC parameter as well, since test networks and other protocols still rely on the RPC feels natural for this to be runtime option.
My
...
💬 John-zhan commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2930621497)
> > Oh, no. I'm so sorry. I modify my ci.yml as this:
>
> What CI are you using? Is the `VCPKG_INSTALLATION_ROOT` environment variable defined there?
Yes. It's `
`sed -i '1s/^/set(ENV{CMAKE_POLICY_VERSION_MINIMUM} 3.5)\n/' "${VCPKG_INSTALLATION_ROOT}/scripts/ports.cmake"`
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2930621497)
> > Oh, no. I'm so sorry. I modify my ci.yml as this:
>
> What CI are you using? Is the `VCPKG_INSTALLATION_ROOT` environment variable defined there?
Yes. It's `
`sed -i '1s/^/set(ENV{CMAKE_POLICY_VERSION_MINIMUM} 3.5)\n/' "${VCPKG_INSTALLATION_ROOT}/scripts/ports.cmake"`
💬 maflcko commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2930638343)
Can you downgrade your cmake from 4.x to 3.x as a temporary workaround?
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2930638343)
Can you downgrade your cmake from 4.x to 3.x as a temporary workaround?
💬 Dakota48423 commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2930651364)
Leveta kalerei?
DakotaAllen
On Mon, Jun 2, 2025, 5:23 AM Max Edwards ***@***.***> wrote:
> *m3dwards* left a comment (bitcoin/bitcoin#32513)
> <https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2929886599>
>
> While touching this code, it seems reasonable to also address the other
> @davidgumberg <https://github.com/davidgumberg>'s comment
> <https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325>:
>
> Added
>
> —
> Reply to this email directly, view it o
...
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2930651364)
Leveta kalerei?
DakotaAllen
On Mon, Jun 2, 2025, 5:23 AM Max Edwards ***@***.***> wrote:
> *m3dwards* left a comment (bitcoin/bitcoin#32513)
> <https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2929886599>
>
> While touching this code, it seems reasonable to also address the other
> @davidgumberg <https://github.com/davidgumberg>'s comment
> <https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325>:
>
> Added
>
> —
> Reply to this email directly, view it o
...
🤔 janb84 reviewed a pull request: "blocks: force hash validations on disk read"
(https://github.com/bitcoin/bitcoin/pull/32638#pullrequestreview-2888353393)
ACK efbe0e86810ccbe828472935eb221c2ddf295bf3
Looks like a good followup PR to use the option to validate the hash as introduced in #32487
- code reviewed ✅
- compiled & run tests ✅
(https://github.com/bitcoin/bitcoin/pull/32638#pullrequestreview-2888353393)
ACK efbe0e86810ccbe828472935eb221c2ddf295bf3
Looks like a good followup PR to use the option to validate the hash as introduced in #32487
- code reviewed ✅
- compiled & run tests ✅
👍 willcl-ark approved a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463#pullrequestreview-2888344985)
ACK 2c5f26bc6ac47e3a6a8555e9a6c60832322a36e8
If you re-touch for any reason it would seem a little more logical to me to slightly restructure so we test without new `minrelaytxfee` first (`check_estimates()` on L260), then add the high `minrelaytxfee` before doing the rest of the tests with it added. But zero practical difference and not a blocker for me.
(https://github.com/bitcoin/bitcoin/pull/32463#pullrequestreview-2888344985)
ACK 2c5f26bc6ac47e3a6a8555e9a6c60832322a36e8
If you re-touch for any reason it would seem a little more logical to me to slightly restructure so we test without new `minrelaytxfee` first (`check_estimates()` on L260), then add the high `minrelaytxfee` before doing the rest of the tests with it added. But zero practical difference and not a blocker for me.
💬 willcl-ark commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2121112548)
Would it be marginally clearer here to use `2` directly here?
```suggestion
assert_greater_than_or_equal(self.nodes[1].estimatesmartfee(2)["feerate"], high_val)
```
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2121112548)
Would it be marginally clearer here to use `2` directly here?
```suggestion
assert_greater_than_or_equal(self.nodes[1].estimatesmartfee(2)["feerate"], high_val)
```
💬 maflcko commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2930666106)
Are you still working on this or can it be closed?
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2930666106)
Are you still working on this or can it be closed?
💬 maflcko commented on pull request "contrib: Add external RPC code generator with unit tests":
(https://github.com/bitcoin/bitcoin/pull/32140#issuecomment-2930668902)
Are you still working on this, or can it be closed?
(https://github.com/bitcoin/bitcoin/pull/32140#issuecomment-2930668902)
Are you still working on this, or can it be closed?
💬 ismaelsadeeq commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2121136080)
Would update when retouching,
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2121136080)
Would update when retouching,
💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2930694178)
OK I reworked this a little so that it works as described in top comment.
The overview is that the provider must be manually specified with `-DCMAKE_PROJECT_TOP_LEVEL_INCLUDES`, thus keeping full control for the user.
The toolchain remains usable (as today) without the provider being specified.
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2930694178)
OK I reworked this a little so that it works as described in top comment.
The overview is that the provider must be manually specified with `-DCMAKE_PROJECT_TOP_LEVEL_INCLUDES`, thus keeping full control for the user.
The toolchain remains usable (as today) without the provider being specified.
💬 ismaelsadeeq commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-2930709631)
> If you re-touch for any reason it would seem a little more logical to me to slightly restructure so we test without new minrelaytxfee first (check_estimates() on L260), then add the high minrelaytxfee before doing the rest of the tests with it added. But zero practical difference and not a blocker for me.
Makes sense, I will add it in the follow-up to this which is retaining the original `feature_fee_estimations.py` tests behaviour by incrementing the custom`blockmaxweight` by 4000. see ht
...
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-2930709631)
> If you re-touch for any reason it would seem a little more logical to me to slightly restructure so we test without new minrelaytxfee first (check_estimates() on L260), then add the high minrelaytxfee before doing the rest of the tests with it added. But zero practical difference and not a blocker for me.
Makes sense, I will add it in the follow-up to this which is retaining the original `feature_fee_estimations.py` tests behaviour by incrementing the custom`blockmaxweight` by 4000. see ht
...
🤔 TheCharlatan reviewed a pull request: "index: Check all necessary block data is available before starting to sync"
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2888284022)
Approach ACK
Can you run `clang-format-diff` on the hunks where it makes sense? I think requiring the indexes to declare if they will use undo data is sensible, but I'd also not be opposed to just making all of them require undo data.
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2888284022)
Approach ACK
Can you run `clang-format-diff` on the hunks where it makes sense? I think requiring the indexes to declare if they will use undo data is sensible, but I'd also not be opposed to just making all of them require undo data.