💬 rkrux commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1977508527)
The RPC request seems to be logged with the `AuthServiceProxy.__id_count` but the corresponding non erroneous response doesn't seem to be. @maflcko Do you know the reason for it or can we improve it later?

```
-30-> getreceivedbyaddress
<-- [0.000553] {"jsonrpc":"2.0"...
```

💬 rkrux commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1977514451)
Also it would be easier on the eyes to log the suffix `s` after the elapsed time but something maybe for another PR.

💬 rkrux commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1977504465)
Older code and doesn't need to be fixed in this PR but this line seems buggy to me, Idk how these 2 conditions can be true at the same time.

💬 Sjors commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2694402685)
re-tACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7

The range-diff shows that we persevere the `DEPENDS_EXPLICIT_OPT` check added in #30911.

Checked the same thing as last time: https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2598371523

> My cmake knowledge is also limited, but I like that this removes duplication.

It still is, and this PR still does.

💬 rkrux commented on pull request "Add mainnet assumeutxo param at height 880,000":
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2694412711)
Well, this pull request is not completely unrelated to this topic but in order to have a more concrete discussion, I will create a Github issue for it & we can move the discussion there.

💬 hebasto commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2694422067)
> I believe `MAIN_DEPENDENCY` is MSVC related, so it wouldn't show up in the `build.ninja`. Did it help with something there?

[It](https://cmake.org/cmake/help/latest/command/add_custom_command.html):
> is treated just like any value given to the `DEPENDS` option but also suggests to Visual Studio generators where to hang the custom command.

I'm not sure how useful it is.

⚠️ Sjors opened an issue: "RFC: hosting utxo snapshot on bitcoincore.org and linking to it"
(https://github.com/bitcoin/bitcoin/issues/31972)
#31969 adds a net mainnet snapshot for v29

I generated a torrent for it. There was some discussion about whether we should put a binary seed on bitcoincore.org.

But that's just a performance detail. The main question is: should we link to the snapshot torrent from the release notes, installation instructions and/or the website?

We already commit to the hash in code, so it seems fine to me to link to a torrent - as long as enough people verify that its content matches.

💬 Sjors commented on pull request "Add mainnet assumeutxo param at height 880,000":
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2694423245)
@rkrux already done in #31972

💬 laanwj commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2694547169)
Seems fine to add this, though it also takes up a line for information that is always the same for the same node, dynamic status information is the most interesting to show in `-*info` calls.

🤔 ismaelsadeeq reviewed a pull request: "wallet, rpc: deprecate settxfee and paytxfee"
(https://github.com/bitcoin/bitcoin/pull/31278#pullrequestreview-2654184017)
Looking good @polespinasa
I've reviewed the code and tested the RPC's and the startup option
Just a couple of comments

💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977554732)
In "wallet,rpc: paytxfee and settxfee is deprecated and hidden from help" ee080ce689bfc8b74438a02e1c62bbe644893ffa

The startup option is not hidden from startup help, IIUC it is not deprecated we just throw a warning when loading a wallet.

💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977549689)
In "wallet,rpc: paytxfee and settxfee is deprecated and hidden from help" ee080ce689bfc8b74438a02e1c62bbe644893ffa

I think you can consolidate this to be dry

```suggestion

RPC and Startup Option
---
The -paytxfee startup option and the settxfee RPC are now deprecated and will be removed in Bitcoin Core 31.0. They allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can lead to overpaying or underpaying. Users should in
...

💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977577720)
In "wallet,rpc: paytxfee and settxfee is deprecated and hidden from help" ee080ce689bfc8b74438a02e1c62bbe644893ffa

Shouldn't this be a failure instead of a warning since an error is being thrown in the RPC? I think we should be consistent and fail as well.

The user should explicitly provide something like `deprecatedstartupoption=paytxfee`. Do we support that?

💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977593279)
In "wallet,rpc: paytxfee and settxfee is deprecated and hidden from help" ee080ce689bfc8b74438a02e1c62bbe644893ffa

nit:
I think release notes is usually on it's own commit

💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977559809)
In "wallet,rpc: paytxfee and settxfee is deprecated and hidden from help" ee080ce689bfc8b74438a02e1c62bbe644893ffa

Note: when removing `paytxfee` in the future we should also update `txconfirmtarget` startup option help text

💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977597368)
Makes sense to me to be hidden but no strong opinion

💬 m3dwards commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-2694584064)
CC @willcl-ark

There are also some companies that offer drop in replacements to Github runners such as [BuildJet](https://buildjet.com/for-github-actions). They will also often provide their own caching options.

I quite like the idea of all of CI being defined as Github Actions which means all jobs should be runnable on forks (if a little slower as they wouldn't be on bigger runners).

Assuming we were happy to migrate the jobs to Github Actions, the next consideration is which runners to use:
...

🚀 fanquake merged a pull request: "cmake: Revamp handling of data files"
(https://github.com/bitcoin/bitcoin/pull/30901)

👍 ismaelsadeeq approved a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2654391185)
Code review ACK 6d7648795aa053f535510d11379fdd9d0399004c

This dummy values were introduced in #2115. I did not see the usage or rationale for adding the negative fees of all block transactions as the fee of the coinbase tx.
I think this dummy values should be deleted.

However I think having total_fees values can be useful.

🤔 ismaelsadeeq reviewed a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803#pullrequestreview-2654431647)
fwiw ACK 8c926243656b42479aa5e3cd13701450a43922a6