Bitcoin Core Github
42 subscribers
126K links
Download Telegram
⚠️ 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
💬 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.