Bitcoin Core Github
44 subscribers
121K links
Download Telegram
🤔 maflcko reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2439353625)
Just some non-blocking nits. Feel free to ignore.

review ACK cc6d3ea623933cc7f0db0daaabcbdda86272c8f7 🐱

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNL
...
💬 Sjors commented on issue "Proposed Timeline for Legacy Wallet and BDB removal":
(https://github.com/bitcoin/bitcoin/issues/20160#issuecomment-2480556327)
I wrote two years ago:

> However, not too soon.

Ok, now is fine :-)

It seems #30328 and #31248 is next on the review list and then actual removal in #28710?
💬 hebasto commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2480631513)
> cmake version 3.10.2

This CMake version does not understand the `-S` and `-B` command line options, which were introduced in the version 3.13.

@techy2

You can download CMake 3.22 or newer, which is required for the main build system, from https://cmake.org/download/.
💬 hebasto commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2480658902)
Concept ACK.
👍 hebasto approved a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2440774187)
re-ACK ee1128ead846698db5e5633f193883837f2fbc64, only rebased, a commit message and a comment have been amended since my recent [review](https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160).
💬 hebasto commented on pull request "guix: scope pkg-config to Linux only":
(https://github.com/bitcoin/bitcoin/pull/31276#issuecomment-2480674078)
Concept ACK.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2480677422)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31285.
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2480690102)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/26593.
techy2 closed an issue: "depends build fails - libevent not in CMakeLists"
(https://github.com/bitcoin/bitcoin/issues/31299)
💬 techy2 commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2480721823)
installed 3.22, works now
Thank you
💬 hebasto commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2480760881)
> Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly. Set the necessary compile options at the source file level, rather than the target level.

While working on the CMake staging branch, this was done on purpose (see https://github.com/hebasto/bitcoin/pull/172 and similar https://github.com/hebasto/bitcoin/pull/171). However, the goal [PR](https://github.com/hebasto/bitcoin/pull/157) has since been replaced.

Additionally, from the [Pro
...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1845238389)
Also added fuzz harness
📝 hebasto opened a pull request: "depends: Update minimum required CMake version"
(https://github.com/bitcoin/bitcoin/pull/31300)
From the CMake 3.31 [Release Notes](https://cmake.org/cmake/help/v3.31/release/3.31.html):

> Compatibility with versions of CMake older than 3.10 is now deprecated and will be removed from a future version.

This PR updates the `cmake_minimum_required` command across all packages in depends to ensure the minimum version is not set below 3.10.

Without this change, CMake 3.31 emits a warning:
```
$ make -C depends freetype
make: Entering directory '/home/hebasto/git/bitcoin/depends'
Ex
...
🤔 hebasto reviewed a pull request: "refactor: Drop deprecated space in operator""_mst"
(https://github.com/bitcoin/bitcoin/pull/31267#pullrequestreview-2440818843)
Post-merge ACK faf21625652fd0d4bbf9b86fd9ebedb5857505ea.
💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480841064)
CI failure appears to be a CI bug unrelated to this PR. I don't see a way to un-draft...
👋 luke-jr's pull request is ready for review: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963)
💬 casey commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2480857899)
Something else that's useful to note is that even a short rescan, on a weak or otherwise loaded machine, can cause RPC calls to start timing out. So avoiding it when unnecessary is useful.
💬 casey commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1845264268)
More advanced commands and configuration would definitely require departing from the commands in the `justfile`. However, for someone just getting started, for simple commands, and using the given commands as a starting point to write your own, it's still very useful.
💬 casey commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2480878062)
I started working on a branch which uses JSON schema, instead of an ad-hoc format. So far it seems fine! I started with command arguments first, since they're much simpler than command results.

I tried to keep everything self-contained in `src/rpc/schema.{h,cpp}` files. If someone winds up using this to codegen support for current versions of core, they might wish to back-port it to older versions of core, and having it mostly be in a single place helps with that.

To easily see the output,
...
👍 tdb3 approved a pull request: "net, init: derive default onion port if a user specified a -port"
(https://github.com/bitcoin/bitcoin/pull/31223#pullrequestreview-2440835235)
Code review ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4

> if reviewers think it's better not to make another change now and tell affected users to use -bind instead of -port

I don't feel very strongly either way, but I lean toward implementing the port+1 approach in this PR.

> still add a functional test for -port so that future changes in this area are more likely to be caught by tests.

I support this. It's descriptive and helps catch issues later.