💬 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`.

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2598808316)
re: https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2556956641

> It seems better to either not install these binaries or to put them out of the way in libexec/ to avoid confusion. [...] Would like to see more discussion about this and find out where the disagreement is so binaries don't have to move twice, but this doesn't need to block the PR.

I opened #31679 to move internal binaries to libexec, since using libexec isn't directly related to what this PR is trying to do. W
...

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920217085)
It's `extern const` rather than `constexpr` because its defined in the cpp file, rather than the header -- for constexpr stuff you need to include its value in the header.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920262478)
This period is probably too low for testnet3 which would have ~20k periods analysed and cached rather than the ~1400 that it should have with the BIP9 recommended period size of 2016.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920266744)
Changed this, should be more obvious now.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920433539)
Ah, a later commit had added `using enum ThresholdState` to avoid the need for this in new code, guess I didn't move it quite early enough. Moved it earlier and changed the following line for consistency.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920447244)
They're implementation details that aren't required to use the interface, but also can't just go in the .cpp file (because of tests). `addrman_impl.h` is similar (though I see it's been included in rpc/net.h because `AddrInfo` has been added to addrman.h's interface without the class being moved into that file...) It's just one module, split into three files, so the "circular dependency" is just a false positive.

👍 ryanofsky approved a pull request: "Add multiprocess binaries to release build (except Windows, OpenBSD)"
(https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2559568739)
Code review ACK b924ee0b85f17f684c215ac164d685cfa747556c. Latest version looks good now, assuming CI passed. Includes host_os variable bugfix and some suggested simplifications since last review.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920515277)
Changed it to set `period = chainparams.GetConsensus().DifficultyAdjustmentInterval();` which is much the same, but doesn't rely on the TESTDUMMY deployment.

💬 sipa commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920531697)
Any RPC **in** which one of the parameters...

💬 1440000bytes commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2598860948)
Concept ACK

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920535577)
Done

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920535704)
Done.