👍 ryanofsky approved a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2629670115)
Code review ACK 587ad67bf3bee03a7088e1dd5be2828b7a4f4fd4. Only change since last review is removing another dummy vTxSigOpsCost assignment.

IMO, it would be probably be more ideal to drop the dummy values from the `CBlockTemplate::vTxFees` and `CBlockTemplate::vTxSigOpsCost` vectors. I don't think it matters if they have different indexing than `CBlock::vtx` because that is a different vector in a different class, and it would be confusing for that vector *not* to contain a placeholder transa
...

👍 ryanofsky approved a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2629683943)
Code review ACK 9725cd6a5c20302da3dcae8fc1156458537ce1df. Just minor suggestions implemented since last review: changing comments and swapping current_block / block in waitfornewblock.

💬 BrandonOdiwuor commented on pull request "test: fix TestShell initialization and reset()":
(https://github.com/bitcoin/bitcoin/pull/31415#discussion_r1963477222)
fixed the remaining instances

💬 BrandonOdiwuor commented on pull request "test: fix TestShell initialization and reset()":
(https://github.com/bitcoin/bitcoin/pull/31415#discussion_r1963480997)
reverted the extraction of `dummy_testshell_file` to avoid global variables

💬 BrandonOdiwuor commented on pull request "test: fix TestShell initialization and reset()":
(https://github.com/bitcoin/bitcoin/pull/31415#discussion_r1963484163)
The reason for the extraction was to share the variable with the `reset` function, I have however reverted the extraction to avoid using global variables

💬 BrandonOdiwuor commented on pull request "test: fix TestShell initialization and reset()":
(https://github.com/bitcoin/bitcoin/pull/31415#discussion_r1963486075)
I have reverted the extraction, is this still worth removing?

💬 maflcko commented on issue "ci: Intermittent failure "Could not resolve host: github.com"":
(https://github.com/bitcoin/bitcoin/issues/31889#issuecomment-2671378631)
As a workaround, reverting 4b527fa93b9763a33842069bc07446313cbf5e0f could be investigated. See also https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632994981

💬 BrandonOdiwuor commented on pull request "test: fix TestShell initialization and reset()":
(https://github.com/bitcoin/bitcoin/pull/31415#issuecomment-2671379166)
@l0rinc I have added instructions on how to test on the PR description

The reason for the extraction of the `dummy_testshell_file` initially was to share it with the `reset()` function to fix https://github.com/bitcoin/bitcoin/issues/31131, I have however reverted the change to avoid using global variable

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1963498435)
Unrelated changes have been dropped.

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1963499301)
Thanks! Reworked.

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2671390521)
Addressed the latest @TheCharlatan's feedback.

👍 rkrux approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2629756653)
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02

💬 maflcko commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2671429607)
Given that the commit id is hardcoded into the `git archive`, it could make sense to hardcode everything else as well and then just prefer that over any external environment influences. This should allow to drop `BITCOIN_GENBUILD_NO_GIT`, because whatever result it will calculate will be ignored.

In theory, it could be implemented with a small helper script to inject the hardcoded values into the source code (along with the *current* commit id), then create a new "dummy" commit, then call `git
...

💬 laanwj commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671469502)
> @laanwj did you get a chance to look into this? Else i guess it falls on me to take care of this before deadline..

Whoops. No i didn't, i lost track of the deadline (which, assuming this is considered a bugfix is 2025-03-06, according to #31029), sorry.
i will have a look at it today and tomorrow, if i don't manage to figure out this week, it would be great if you can take it over.

💬 darosior commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671477287)
Sounds good, thanks for looking into it.

-------- Original Message --------
On 2/20/25 8:17 AM, laanwj wrote:

>> ***@***.***(https://github.com/laanwj) did you get a chance to look into this? Else i guess it falls on me to take care of this before deadline..
>
> Whoops. No i didn't, i lost track of the deadline (which, assuming this is considered a bugfix is 2025-03-06, according to #31029), sorry.
> i will have a look at it today and tomorrow, if i don't manage to figure out this week, it wo
...

💬 maflcko commented on pull request "ci: Switch to gcr.io mirror to avoid rate limits":
(https://github.com/bitcoin/bitcoin/pull/31906#issuecomment-2671483472)
> > An alternative would probably be to configure the docker deamons of the self-hosted runners to use the mirror?
>
> I haven't tested this

I tested it and your suggestion failed locally for me:

```
# podman pull docker.io/ubuntu/24.04
Error: loading drop-in registries configuration "/etc/containers/registries.conf.d/gcr.conf": invalid location 'https://mirror.gcr.io': URI schemes are not supported
# cat /etc/containers/registries.conf.d/gcr.conf
[[registry]]
location = "https://m
...

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1963559614)
> What makes the secp subtree special enough to get its own header?

For other subtrees, it has not yet been decided whether their own build scripts will be used.

🤔 marcofleon reviewed a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676#pullrequestreview-2629915256)
ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4

I'm not seeing any slow units with the corpuses I have and when I fuzz with afl++ stability looks very good (98-99%). It could be that I have to fuzz longer for it to appear. I think the new fuzz tests are good to go for now. If issues (timeout, etc.) come up, we can address them in a followup?

Also, left one comment below to improve `FuzzedSock` a bit but it's not a blocker.

💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1963601055)
To make this more accurate, we could have it advance by the full timeout only when the event fails to occur. And then consume from 0 to `timeout` for when the event succeeds. Same for `WaitMany` below.

💬 ryanofsky commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671595439)
I'm not sure there is a strong justification for the `-upnp` option to trigger an interactive warning now, if it was always just a best-effort setting that wouldn't previously fail if it couldn't register with the upnp server., or the server wouldn't provide a port or external ip. Since anyone relying on this would need external monitoring or testing to know if it was working anyway, just using LogWarning here seems like it should be ok.

But if I'm underestimating the importance of having an in
...

💬 rkrux commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2671597288)
> Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment

On master, I am not able to successfully import a descriptor or get the descriptor info in case a space is passed in the desc. Is this statement incorrect or is there any other flow I don't know about?

<details>
<summary>
`getdescriptorinfo` and `importdescriptors` with both public key and private keys on master
</summary>

```
➜ src git:(master)
...