💬 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"`

💬 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?

💬 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
...

🤔 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 ✅

👍 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.

💬 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)
```

💬 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?

💬 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?

💬 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,

💬 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.

💬 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
...

🤔 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.

💬 TheCharlatan commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2121073979)
I think it should be ok to scope the lock to just the necessary call to `LookupBlockIndex`, but I am also having a hard time saying for certain, that there might not be some edge case race condition here with the later call to `Contains` and `FindFork`.

💬 TheCharlatan commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2121131527)
I think you are just comparing pointers here, but actually want to compare their height. This block of code is also kind of hard to test against from a functional test. Maybe we can add a unit test for `StartIndexBackgroundSync` that manipulates some of the internal block index state?

💬 TheCharlatan commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2121167855)
Nit: These include fixes seem a bit random. Why not apply all suggested fixes from the CI?

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2930801207)
> if I (perhaps accidentally) `sendrawtransaction` the same tx a second time, we open private connections, send our INV -- but those nodes already have the tx.

This PR will not queue new private broadcast connections if the same tx is submitted again. However, it does keep trying to broadcast with the 3 connections queued from the first time the tx is submitted. If those peers already have the tx, then the connection will wait for 3 minutes until disconnecting. See discussion [here](https://g
...

👍 TheCharlatan approved a pull request: "doc: Remove build instruction for running `clang-tidy`"
(https://github.com/bitcoin/bitcoin/pull/32662#pullrequestreview-2888488249)
ACK 4b1b36acb48fab133ca4a3241148fa9683915874

🤔 janb84 reviewed a pull request: "doc: Remove build instruction for running `clang-tidy`"
(https://github.com/bitcoin/bitcoin/pull/32662#pullrequestreview-2888493704)
ACK 4b1b36acb48fab133ca4a3241148fa9683915874

Checked both outcomes, the "old" way and the new way, both outcomes are the equal.(the diff was harder to check due to running it in multithreaded mode, therefor the files where out of order)

🚀 fanquake merged a pull request: "doc: Remove build instruction for running `clang-tidy`"
(https://github.com/bitcoin/bitcoin/pull/32662)

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2930824497)
It seems that if a (miniscript, not yet spendable) script path is present in the descriptor, then `walletprocesspsbt` won't provide a `pubnonce`.