✅ techy2 closed an issue: "depends build fails - libevent not in CMakeLists"
(https://github.com/bitcoin/bitcoin/issues/31299)
(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
(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
...
(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
(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
...
(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.
(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...
(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)
(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.
(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.
(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,
...
(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.
(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.
💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1845288556)
Fixed bug in this and added it, along with tests
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1845288556)
Fixed bug in this and added it, along with tests
💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480923042)
Rebased to workaround CI bugs for now (is someone going to fix them?)
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480923042)
Rebased to workaround CI bugs for now (is someone going to fix them?)
💬 achow101 commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480926302)
> Rebased to workaround CI bugs for now (is someone going to fix them?)
Considering that CI is failing immediately after a `walletprocesspsbt` call, and that RPC is being changed in this PR, I'm inclined to think that it's probably not a CI issue.
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480926302)
> Rebased to workaround CI bugs for now (is someone going to fix them?)
Considering that CI is failing immediately after a `walletprocesspsbt` call, and that RPC is being changed in this PR, I'm inclined to think that it's probably not a CI issue.
📝 PastaPastaPasta opened a pull request: "refactor: covert ContainsNoNUL to ContainsNUL"
(https://github.com/bitcoin/bitcoin/pull/31301)
This allows us to convert all the double negative calls such as `!ContainsNoNUL()` -> `ContainsNUL()`.
There are no usages which aren't double negatives. This makes the code more understandable
Also, use std::any_of instead of manually looping
(https://github.com/bitcoin/bitcoin/pull/31301)
This allows us to convert all the double negative calls such as `!ContainsNoNUL()` -> `ContainsNUL()`.
There are no usages which aren't double negatives. This makes the code more understandable
Also, use std::any_of instead of manually looping
💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480937997)
>Considering that CI is failing immediately after a walletprocesspsbt call, and that RPC is being changed in this PR, I'm inclined to think that it's probably not a CI issue.
Yes, there was a new issue introduced by the added commits too. But there was also a CI issue: it failed to merge into master before building.
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480937997)
>Considering that CI is failing immediately after a walletprocesspsbt call, and that RPC is being changed in this PR, I'm inclined to think that it's probably not a CI issue.
Yes, there was a new issue introduced by the added commits too. But there was also a CI issue: it failed to merge into master before building.
💬 Eunovo commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2480940195)
> > I believe you can get more fine-grained benchmarks for this using the `rpc_blockchain` benchmarks.
>
> Yes, I can, but I’ll need to add verbosity levels 1 and 2, where we can see some performance improvements.
>
> Show diff
> Benchmark Results
>
> On **master**:
>
> ns/op op/s err% total benchmark
> 190,342 5,254 2.3% 0.01 `BlockToJsonVerbose1`
> 34,812,292 28.73 1.0% 0.38 `BlockToJsonVerbose2`
> 34,457,167 29.02 1.0% 0.38 `BlockToJsonVerbose3`
> On **this PR**:
>
> ns/op
...
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2480940195)
> > I believe you can get more fine-grained benchmarks for this using the `rpc_blockchain` benchmarks.
>
> Yes, I can, but I’ll need to add verbosity levels 1 and 2, where we can see some performance improvements.
>
> Show diff
> Benchmark Results
>
> On **master**:
>
> ns/op op/s err% total benchmark
> 190,342 5,254 2.3% 0.01 `BlockToJsonVerbose1`
> 34,812,292 28.73 1.0% 0.38 `BlockToJsonVerbose2`
> 34,457,167 29.02 1.0% 0.38 `BlockToJsonVerbose3`
> On **this PR**:
>
> ns/op
...
📝 PastaPastaPasta opened a pull request: "refactor: spanify DecodeBits, use constexpr std::array instead of vector"
(https://github.com/bitcoin/bitcoin/pull/31302)
Should generally prefer using std::span over const std::vector&, and generally should use constexpr std::array instead of the const std::vector as used here. This allows the interface to be more flexible (thought the value is low here) and allows it to be more constexpr.
Brief investigation was made into spanifing other interfaces in this file; however, the usage of the specialization std::vector<bool> made that incompatible with trivial spanification
(https://github.com/bitcoin/bitcoin/pull/31302)
Should generally prefer using std::span over const std::vector&, and generally should use constexpr std::array instead of the const std::vector as used here. This allows the interface to be more flexible (thought the value is low here) and allows it to be more constexpr.
Brief investigation was made into spanifing other interfaces in this file; however, the usage of the specialization std::vector<bool> made that incompatible with trivial spanification
💬 TheCharlatan commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1845340819)
We already have the cmake presets for making it a bit easier to get started and the user can bring their own `CmakeUserPresets.json`. It is seldom that I am still typing out a full cmake command.
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1845340819)
We already have the cmake presets for making it a bit easier to get started and the user can bring their own `CmakeUserPresets.json`. It is seldom that I am still typing out a full cmake command.