💬 theStack commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1789334417)
I think this line makes the test flaky, as the orphan tx to drop in `TxOrphanage::LimitOrphans` is chosen randomly, i.e. it could also be the last one added (`tx_child_1` in this case). Didn't test locally yet, but I'd assume that about every 100th test run fails.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1789361727)
(yeah, marking pretty much all the moved stuff resolved)

👋 tdb3's pull request is ready for review: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043)

💬 pilab-gwon commented on issue "[Testnet] Insufficient data or no feerate found":
(https://github.com/bitcoin/bitcoin/issues/31032#issuecomment-2395841624)
> Fee estimation needs data, which consists of seeing transactions arrive, and seeing them be mined. Testnet just doesn't have much data.

I'd like to think that this is a problem caused by not having enough data, but that doesn't seem to be the case in my case.

I have two Bitcoin testnet nodes running (let's call them A and B).

Sometimes node A throws the error `Insufficient data or no feerate found`, while node B returns feerate without any problem.

Other times, both nodes A and B
...

💬 maflcko commented on issue "[Testnet] Insufficient data or no feerate found":
(https://github.com/bitcoin/bitcoin/issues/31032#issuecomment-2395992525)
> Sometimes node A throws the error `Insufficient data or no feerate found`, while node B returns feerate without any problem.

This is expected, because the two nodes may not have seen the same transactions at the same times.

💬 maflcko commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1789564241)
I wonder if it is possible to use mocktime and an exact assert?

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