💬 laanwj commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977456821)
The regexp doesn't match the entire string until the end, so "0.19.0.1" and such already match. This change will only make it that "0.19.1" no longer matches.

💬 laanwj commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977459441)
We shouldn't have `|` at the end of the last clause, as this will make it match the empty string too (so effectively everything starting with `Satoshi:` matches).

💬 hebasto commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2694290202)
Concept ACK.

💬 dergoegge commented on pull request "Add mainnet assumeutxo param at height 880,000":
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2694345636)
Discussions to host the actual chainstate as the project itself (i.e. on bitcoincore.org) should not be had on some unrelated pull request. I think this deserves a full discussion in a separate issue or at the irc meeting.

Iirc there were discussions in the past about this and the sentiment (as I remember it) was not to host the actual snapshot.

👍 rkrux approved a pull request: "test: Fix authproxy named args debug logging"
(https://github.com/bitcoin/bitcoin/pull/31955#pullrequestreview-2654107325)
tACK fac1dd9dffba1033245c283bc0468e801c14e910

I ran the `wallet_timelock` test by passing both positional and named arguments in the first `listunspent` RPC.

```
coin_before = node.listunspent(1, maxconf=1, include_unsafe=True)
```

I can see both kinds of arguments being logged.

```
-28-> listunspent [1] {"maxconf": 1, "include_unsafe": true}
<-- [0.000553] {"jsonrpc":"2.0","result":[{"txid":"c0bbea0efd29a1c72e264efeb4eb53096741ebf3d309cabcec343b96e21a59ed","vout":0,"address":"bc
...

💬 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