💬 fanquake commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1954349832)
Yes

💬 maflcko commented on pull request "qa debug: Add --debug_runs/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2656348436)
> > I think if you want to debug the start-up sequence specifically, it may be easier to do it outside the tests? Then anything else, if needed, can happen without patching Bitcoin Core source code itself.
>
> This is not about debugging the start-up sequence specifically, but rather debugging _from an early stage_. Maybe one has set 5 breakpoints, some of which are hit during _bitcoind_ init, and some which are hit by re-running a Python test which calls a set of RPCs.

I'd still say it is
...

💬 maflcko commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2656374222)
This still happens: https://cirrus-ci.com/task/4901485584056320

Though, it seems odd, because that run should not have pulled more than two images. Rate-limiting that seems that the new rate limits are stronger that they document themselves?

If that is true, maybe the easiest fix would be to run (or pick) a caching pass-through mirror instead?

💬 maflcko commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656376795)
lgtm ACK 033acdf03da4a77d69fb58f7cab97dd1073fb83e

💬 delta1 commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656383645)
ACK https://github.com/bitcoin/bitcoin/commit/033acdf03da4a77d69fb58f7cab97dd1073fb83e

🚀 fanquake merged a pull request: "chore: remove redundant word"
(https://github.com/bitcoin/bitcoin/pull/31855)

💬 delta1 commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656392782)
similar instance on line 94 now that i see it

https://github.com/bitcoin/bitcoin/pull/31855/files#diff-91d294c18d2f63ceb1c9aadff3b3ae046b252b763d69b13f930f7de7b399f440R94

⚠️ jirijakes opened an issue: "doc/zmq: Note about endianness does not match reality"
(https://github.com/bitcoin/bitcoin/issues/31856)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

PR #23471 added a note to ZMQ's documentation page saying that:

> […] 32-byte hashes are in Little Endian and not in the Big Endian format that the RPC interface and block explorers use to display transaction and block hashes.

Also:

> `| hashtx | <32-byte transaction hash in Little Endian> | <uint32 sequence number in Little Endian>`
> `| hashblock | <32-byte block hash in Little Endian
...

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954403094)
I'm not expert in this area to decide if this is to sterile or not (i.e. testing a too narrow slice that isn't representative, where the biggest speedup doesn't cause any measurable macro change), will let others decide if this is too `micro` or not.

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954418526)
is the` AddToBlockIndex` cheap enough to be done inside the benchmark

🤔 rkrux reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2614799077)
ACK af76664b12d8611b606a7e755a103a20542ee539

I reviewed range-diff:
```
git range-diff d5e28457a099cd546e757984043f28ba9f6689b1...af76664b12d8611b606a7e755a103a20542ee539
```

Newer changes are incorporating previous suggestions such as fixing comments, adding couple more assertions in the functional tests, getting rid of unused private key in test. Clarified my previous suggestion.

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954387143)
Not sure if I was clear earlier. I am suggesting this change:

```
some_keys_addr = self.old_node.deriveaddresses(some_keys_priv_desc)[0]
all_keys_addr = self.old_node.deriveaddresses(all_keys_priv_desc)[0]
```

Reason being that these are imported in the wallet down below, a wallet that has been created on old_node. Same reasoning for the suggestion in the next comment for the `test_taproot` test.

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954390887)
Aah right, thanks.

💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136)
1. Rebased due to the conflict with the merged bitcoin/bitcoin#30911.
2. Addressed the recent @theuni's feedback.
3. Fixed minor bugs, such as: https://github.com/bitcoin/bitcoin/blob/18cc0a55595f9dc1f2e561743201a05754996b64/cmake/module/TargetDataSources.cmake#L6

> Also, since you're messing with this, please consider the `CODEGEN` option for `add_custom_command`, which would need feature-gating same as `DEPENDS_EXPLICIT_OPT`.

Nice! Added.

Please note that `cmake_policy(SET CMP0171 N
...

💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1954428888)
Thanks! [Rebased](https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136).

💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1954429250)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2656487136).

💬 maflcko commented on pull request "fuzz: Use serial task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2656491879)
CI passed and coverage is as expected:

* This pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/faad87189015e559/733f70b21be22925/fuzz.coverage/index.html
* Before this pull: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/6a46be75c43e6489/733f70b21be22925/fuzz.coverage/index.html

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954433827)
I pushed a commit to explain the long poll behaviour. Also added an early return above this point, just for clarity.

💬 ryanofsky commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954434025)
re: https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954308137

> Nit in [a8fedb3](https://github.com/bitcoin/bitcoin/commit/a8fedb36a71ac193fc158284b14d4eb474e5ab62): Why is this needed? It seems like a no-op:

Right it is is a no-op, see #30529 which makes the change you suggested and removes many other misuses of IsArgSet

💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2656503405)
- Removed the nested loop in `GetAdaptersAddresses` (@sipa)
- Made doc comment for `FromSockAddr` more concise (@davidgumberg)