💬 maflcko commented on pull request "build: Rename `PACKAGE_*` variables to `BITCOIN_PACKAGE_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1789575428)
nit: When renaming this, wouldn't it make more sense to use the `CLIENT_VERSION_` prefix here as well? (Maybe the two other variables others could use the `CLIENT_` prefix as well?)

💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1789657200)
Thanks! Updated per your feedback.

💬 hebasto commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2396158697)
> In order to get `bitcoin-mine` in the guix build (once it supports multiprocess) I think you need to move `set(installable_targets)` up in the file, and also add `bitcoin-mine` to the list in `foreach(target IN ITEMS bitcoind` in `Maintenance.cmake`.

I don't think `set(installable_targets)` needs to be moved up because it already precedes all subsequent use cases.

> Though I'm not sure why that doesn't use `installable_targets`

There are two local `installable_targets` variables: one
...

💬 maflcko commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#issuecomment-2396167856)
lgtm ACK deacf3c7cd68dd4ee973526740e45c7015b6433a

👍 Sjors approved a pull request: "test: Remove 0.16.3 test from wallet_backwards_compatibility.py"
(https://github.com/bitcoin/bitcoin/pull/30920#pullrequestreview-2351187932)
utACK fae44c83da982095661b034bdd01afe8ac2fb0a6

💬 maflcko commented on pull request "qt6, test: Handle deprecated code":
(https://github.com/bitcoin-core/gui/pull/839#discussion_r1789716060)
Is the `c_str()` still needed?

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2396197303)
@hebasto I forgot that I moved `target_link_libraries(bitcoin-mine` up myself in https://github.com/Sjors/bitcoin/pull/48/commits/254cf216b5e312297ec34a4ee58256d21ab8571e. So need to change `set(installable_targets)` here.

💬 maflcko commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2396202777)
lgtm ACK 80761afced12c774a1768fb27a3975d456981ae0

💬 fanquake commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2396206978)
> I think small changes to make code compatible with both versions are fine

I don't think that is the case here? Qt 6 isn't supported for windows until the later PR, at which point Qt 5 isn't supported?

💬 maflcko commented on issue "Crash upon RPC v1 connection in v28.0.0":
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2396224686)
> ### Steps to reproduce
>
> Run Bitcoin Core

What are the exact steps to reproduce? What is the config? Which RPCs were called, in what order, and in what timing (parallel, sequential), ...?

Does it still happen after https://github.com/romanz/electrs/pull/1091 ?

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396235386)
Getting fresh error when trying to `make` depends on Intel macOS 15.0.1, without Xcode.coc

```
ERROR: detected a std::atomic implementation that fails for function pointers. Please apply the patch corresponding to your Standard Library vendor, found in qtbase/config.tests/atomicfptr

CMake Error at qtbase/cmake/QtBuildInformation.cmake:208 (message):
Check the configuration messages for an error that has occurred.
Call Stack (most recent call first):
qtbase/cmake/QtBuildInformation
...

💬 maflcko commented on issue "RPC breakage with v28.0":
(https://github.com/bitcoin/bitcoin/issues/31039#issuecomment-2396238757)
> I'm not observing any RPC issues yet with [mempool/electrs@055aba1](https://github.com/mempool/electrs/commit/055aba1e8d0d8f4cf7080014c89e3b3055e3d0b7), except for it not handling xor'd blocks, but that's a different problem.

Just for reference: https://github.com/mempool/electrs/pull/101

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1789755698)
Heh, we currently always inform the user with "Error opening block database." even if there is an error with the chainstate. I think I am going to leave this as is here, so not to scope creep this PR and will address this in another PR.

💬 promag commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2396253665)
@fanquake oh I see your point, we won't need to support the old signature of qt5 so the diff could be just the arg type change.

> at which point Qt 5 isn't supported?

Unless this is not the case? @hebasto?

I'm keeping the ACK, but what @fanquake suggests is fine too, with less maintenance efforts.

💬 fanquake commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2396262864)
@promag sed this commit in the switchover PR: https://github.com/bitcoin/bitcoin/commit/812f3e7727211a60989e613bd5edf0a4ec3f146f. Qt 6.7.0 or later will be required for Windows (although as far as I'm aware, Qt 5 won't be supported generally in any case).

💬 maflcko commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2396270745)
Looks good to me either way as well. Merge now and remove later, or change later. Not sure how hard the overall changes are, but "spreading-out" review could be useful. Though, no strong opinion, either way is fine.

💬 vasild commented on pull request "chainparams: Remove seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/30720#issuecomment-2396293474)
> Great, I'm just testing it as we speak, and I'll open a new PR adding it, once its shown to work, if that's ok.

wen pr?

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1789788111)
Ugh, I should have drafted this PR once I opened #30968 , since I opened it after being fed up with the init logic in this change. The error handling introduced there forces you to do the right thing in this change now. Sorry for the churn!

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2396295598)
Rebased 63dfc6067bd7fe06e5a440c229cca030bba6d53a -> 63dfc6067bd7fe06e5a440c229cca030bba6d53a ([blockmanDB](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB) -> [blockmanDB](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB..blockmanDB))

* Fixed conflict with #30968

👍 maflcko approved a pull request: "test: Fix copy-paste in wallet/test/db_tests ostream operator"
(https://github.com/bitcoin/bitcoin/pull/31038#pullrequestreview-2351318001)
lgtm ACK f50557f5d36f568b7fe65ff5e242303b16b9b258