💬 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

💬 dergoegge commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2694732533)
I think this is a good addition to the unit test framework but I'd like to see some more tapscript specific test cases added in this PR as well (e.g. OP_SUCCESSx opcodes, schnorr sigs, no check multi sig, ...).

💬 laanwj commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2694757885)
Code review ACK fab51b26d68b37f5914314ad29eeb930538ea7ae
- Concept ACK: Prefer equivalent built-in functionality where possible
- Manually verified the scripted diffs
- Checked that there are no remaining unnecessary capitalizations of Span in comments.

📝 eval-exec opened a pull request: "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy"
(https://github.com/bitcoin/bitcoin/pull/31973)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

When I reviewing bitcoin's torcontrol.cpp's source code, the `true` in:
https://github.com/bitcoin/bitcoin/blob/79bbb381a1fd13ad456fde736b3c195a791d4e58/src/torcontrol.cpp#L400
is not easy to understand, so I add a comment to
...

💬 laanwj commented on pull request "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy":
(https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977739770)
i'd use a named parameter:
```
Proxy addrOnion = Proxy(resolved, /*m_randomize_credentials=*/ true);
```

💬 eval-exec commented on pull request "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy":
(https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977740942)
Thank you.

💬 eval-exec commented on pull request "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy":
(https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977744398)
Hello, what format tool do you use to format bitcoin's source code?
I tried `clang-format -i src/torcontrol.cpp`, it format too much.

💬 laanwj commented on pull request "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy":
(https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977748174)
Yes-FWIW, the main way in which this increases privacy is that every new connection is likely to choose a different exit node, so will appear to come from a different host.

There's something to be said for adding this documentation for the Proxy constructor, but as this specific behavior is specific to Tor (and not SOCKS5 in general) i have no objection to adding it here.

💬 laanwj commented on pull request "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy":
(https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977757935)
As you only add comments, i don't think there's a reason to run the formatter.

However, there's the [clang-format-diff](https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy) script that can be used to only re-format lines that have been changed, and not the whole file.

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1977762419)
Also, as discussed offline, this seems like an easy scenario to write a fuzz test for as described above ^

💬 laanwj commented on pull request "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy":
(https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977773135)
Sorry, it's failing lint now:
```
/ci_container_base/src/torcontrol.cpp:404:39: error: argument name 'm_randomize_credentials' in comment does not match parameter name '_randomize_credentials' [bugprone-argument-comment,-warnings-as-errors]
[10:56:52.844] 404 | Proxy addrOnion = Proxy(resolved, /*m_randomize_credentials=*/ true);
[10:56:52.844] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
[10:56:52.844] | /*_ra
...