Bitcoin Core Github
42 subscribers
126K links
Download Telegram
💬 ryanofsky commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920355534)
In commit "build: depends makes libmultiprocess by default" (https://github.com/bitcoin/bitcoin/commit/48268facc9bb4f3e4f98cff5c21e1fe490b232e2)

Would suggest reverting this change to simplify the PR.
💬 ryanofsky commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920385465)
In commit "build: depends makes libmultiprocess by default" (48268facc9bb4f3e4f98cff5c21e1fe490b232e2)

This isn't actually doing anything because RUN_FUNCTIONAL_TESTS is false above
💬 ryanofsky commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920342619)
In commit "build: depends makes libmultiprocess by default" (48268facc9bb4f3e4f98cff5c21e1fe490b232e2)

It doesn't seem right to include host_os in these variables anymore. I think this is is causing multiprocess packages not to be built right now. If I run `make print-multiprocess_packages_` the variable is empty and the toolchain file does not seem to have multiprocess enabled. But if I drop host_os these things are fixed:

```diff
-multiprocess_packages_$(NO_MULTIPROCESS) = $(multiproces
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920426923)
Indeed, I overlooked that.
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920431102)
I'm confused. What should depends/Makefile look like?

I'll push a new version with just the above _host_ bugfix for now.
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2598732399)
> There's no depends build using multiprocess binaries in functional tests.

I added `BITCOIND=bitcoin-node` to "CentOS, dash, gui". So after the _host_ bug fix (https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920342619), the it should use depends, should include multiprocess and run the functional tests.
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920450203)
Moved to "CentOS, dash, gui".
💬 ryanofsky commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920454143)
> I'm confused. What should depends/Makefile look like?

I think the only changes that need to be made to this file in this PR are:

```diff
--- a/depends/Makefile
+++ b/depends/Makefile
@@ -41,7 +41,10 @@ NO_SQLITE ?=
NO_WALLET ?=
NO_ZMQ ?=
NO_USDT ?=
-MULTIPROCESS ?=
+# Default NO_MULTIPROCESS value is 1 on unsupported host platforms:
+# - Windows
+# - OpenBSB: https://github.com/capnproto/capnproto/pull/1907
+NO_MULTIPROCESS ?= $(if $(findstring mingw32,$(HOST))$(findstring
...
💬 mzumsande commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1920457435)
Right, with`ProcessOrphanTx()` doing at most one actual validation, it would probably not be good to call it in a loop until all outstanding work is done. Maybe calling it just once (as if it was this peer's next turn) would already help in most cases, but to find out it'd probably be best to get some empirical data from mainnet.
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2598764171)
@fanquake IMO, monitoring CPU usage is useful on its own, even if we don't start treating peers differently based on that metric (https://github.com/bitcoin/bitcoin/issues/31033). In other words, this metric is useful at least as much as a bunch of other metrics in the `getpeerinfo` output.

Yes, I have been running this locally - there is like 65x difference between the least and most demanding peers. I am curious to correlate this to the messages being sent/received to/from those peers and t
...
📝 maflcko converted_to_draft a pull request: "ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions"
(https://github.com/bitcoin/bitcoin/pull/31522)
It is possible that someone accidentally removes the workaround in fa9e0489f57968945d54ef56b275f51540f3e5e4, or more likely that someone accidentally adds new code without the workaround.

Avoid this by adding a temporary CI check.

This can be tested by reverting the workaround and observing a failure.
👍 TheCharlatan approved a pull request: "ci: optionally use local docker build cache"
(https://github.com/bitcoin/bitcoin/pull/31545#pullrequestreview-2559492845)
tACK e87429a2d0f23eb59526d335844fa5ff5b50b21f
💬 maflcko commented on pull request "ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions":
(https://github.com/bitcoin/bitcoin/pull/31522#issuecomment-2598785767)
Ok, put into draft for now. I'll rebase after centos 10.
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920481732)
Done
📝 pablomartin4btc converted_to_draft a pull request: "Add new "address type" column to the "receiving tab" address book page"
(https://github.com/bitcoin-core/gui/pull/753)
This PR fixes #646 and introduces some enhancements to the Address Book functionality.

A new "Address Type" column has been added to the address book table page, only visible for the "Receiving address" tab. Users can employ an also new added combo box at the bottom of the page to filter address by their type, this filtering can be combined with the current search line text box.
When the export feature is used, this new field will be included.

![Peek 2023-09-10 19-25](https://github.com/b
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2598790040)
I applied the simplification suggested here: https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920454143
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920485524)
(it snuck back in, but it's now also dropped from `00_setup_env_arm.sh`)
📝 ryanofsky opened a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679)
Currently when "make install" or "cmake --install" are run, various internal binaries that are confusing and not typically useful and installed to `${CMAKE_INSTALL_PREFIX}/bin` and can wind up on the system PATH. This PR moves internal binaries out of `bin/` into `libexec/` where they will still be accessible but will not be automatically placed on the PATH or be confused with more useful binaries. The PR also adds an install rule for the bitcoin-chainstate binary. After this PR binaries install
...
💬 pablomartin4btc commented on pull request "Add new "address type" column to the "receiving tab" address book page":
(https://github.com/bitcoin-core/gui/pull/753#issuecomment-2598800192)
<details>
<summary><B>Drafted</B> while solving the nested filter logic that immediately after rebasing <code>clang-tidy-19</code> was complaining about.</summary>

```
[14:05:17.184] [623/666][21.4s] clang-tidy-19 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/qt/addressbookpage.cpp
[14:05:17.184] /ci_container_base/src/qt/addressbookpage.cpp:30:5: error: function 'AddressBookSortFilterProxyModel' is within a re
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2598801543)
So now the macOS native job, without depends, as well as the CentOS job, with depends, run the functional tests using `bitcoin-node`.