💬 vasild 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_r1954339722)
TIL https://flylib.com/books/en/2.887.1.53/1/

Possible solutions I see:
* drop the default altogether and have all callers use 2 arguments
* use the "non-virtual interface idiom" as described in the chapter above (it is from the book "Effective C++" by Scott Meyers)
* use overload instead of default arguments. Have two methods `waitTipChanged(hash)` and `waitTipChanged(hash, timeout)` (the first could call the second internally).

💬 fanquake commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2656330751)
Close this given #31844 (which also solves #31745)? Or, it'd be good if you could comment either here or there on the alternative approach.

💬 vasild commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#issuecomment-2656335347)
`74f42a5920...cd4bfaee10`: address https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954320762

💬 vasild commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954343810)
Should be "CService", fixed, thanks!

💬 InnDe commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656343492)
I think 14 or 15 that was before 12 years
On Thu, 13 Feb 2025 at 14:20 Will Clark ***@***.***> wrote:

> but now it doesn’t work as usual
>
> Which previous version(s) of bitcoin core did this particular wallet and
> password work with?
>
> Does it still work with that version (thereby implying a regression in
> newer versions)?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656290307>,
> or unsubscribe
> <h
...

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