💬 furszy commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1956639524)
doesn't hurt to add it.

💬 furszy commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1956655963)
"External wallets" are wallets that were created on a different node instance. You can either load them by path, using the dump/import functionality or just by moving them manually into the wallets/ directory.

💬 sipa commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#issuecomment-2660161679)
Eagerly awaiting C++26's explicit `[[indeterminate]]` [initialization](https://en.cppreference.com/w/cpp/language/attributes/indeterminate).

💬 furszy commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1956660785)
We haven't started following this pattern in the wallet sources yet. Might be better to do it all at once in a single PR and verify that nothing breaks (which should be the case because we don't use the witness id).

💬 furszy commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1956668649)
> Is the following scenario handled by the usual handling or via this new one?
>
> > User has their node running and receive a coinbase transaction, and then the node is stopped. User restarts the node after a long time and meanwhile that transaction was invalidated (block invalidated), and at the time of loading the wallet the correct balance is shown.

This new one.

💬 tnndbtc commented on issue "contrib: Autogenerate bash completion":
(https://github.com/bitcoin/bitcoin/issues/17289#issuecomment-2660181474)
One improvement to the slowness due to RPC on auto completion is to add those first level (cword==1) commands in contrib/completions/bash/bitcoin-cli.bash, then it will not rely on RPC to bitcoind. And, for any give release, run functional test with --overwrite to generate bitcoin-cli.bash then it should have a good coverage on supported commands.

I put more details in https://github.com/bitcoin/bitcoin/pull/30860/files#r1956655374 with sample code, which is validated in my mac Apple M1 chip.

...

💬 fanquake commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1956673569)
What does this mean? I'm not sure it's correct.

💬 tnndbtc commented on pull request "test: autogenerate bash completion":
(https://github.com/bitcoin/bitcoin/pull/30860#issuecomment-2660193274)
I tested the code in mac(Apple M1 chip) and it worked fine. Steps are:
# run test and generate contrib/completions/bash/bitcoin-cli.bash:
% build/test/functional/test_runner.py test/functional/tool_cli_bash_completion.py --tmpdir /tmp --nocleanup --overwrite

# validate bash-completion on mac:
% brew info bash-completion

# switch to bash shell
% bash

# source the bash_completion script
% . /opt/homebrew/etc/profile.d/bash_completion.sh

# source bitcoin-cli bash
% source contrib
...

💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2660211682)
Rebased to fix merge conflict.

💬 davidgumberg commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1956687815)
It's also fair if you just think #31665 is a bad approach and this is more evidence of that.

💬 l0rinc commented on pull request "optimization: speed up block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r1956688357)
Absolutely, I already have other optimization ideas in mind after that's merged.

The reviewers can decide the preferred merge order, I don't mind rebasing or doing it in multiple PRs - there's a lot of work left with serialization anyway.

💬 davidgumberg commented on pull request "doc: build: Fix instructions for msvc gui builds":
(https://github.com/bitcoin/bitcoin/pull/31851#issuecomment-2660223577)
> ACK [c3fa043](https://github.com/bitcoin/bitcoin/commit/c3fa043ae560289759b6f836ac62736d97ba91bf)
>
> Am I misremembering or was this, or a similar workaround, part of the previous build instructions at some point?

Yep, #29048 added a similar note to the old `build_msvc/README.md`, which had instructions for doing a static build of Qt, but I guess during the cmake transition, there was a switch from manually building Qt to using vcpkg, but sadly, the same problem still exists.

💬 theuni commented on pull request "cmake: add optional source files to bitcoin_crypto and crc32c directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2660224380)
Agree with @hebasto that it'd be more straightforward to use `target_compile_options()`, if only for the sake of avoiding future footguns.

utACK otherwise.

💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1956695414)
Suggested by me here: https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1912232236
Maybe stating the obvious a bit too much for your taste, or how would it be incorrect?

💬 theuni commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#discussion_r1956695948)
Thanks @maflcko, I also agree.

💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1956696518)
> What does this mean?

My point is that it's not required on other operating systems.

👍 theuni approved a pull request: "random: Initialize variables in hardware RNG functions"
(https://github.com/bitcoin/bitcoin/pull/31863#pullrequestreview-2618764775)
utACk 99755e04ffadd5daad53c686f4f0feee2232eeb2

💬 theuni commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2660231472)
> > gcc (since v10) and clang (since v13) have intrinsics for __rndr and __rndrrs that would imo be preferable.
>
> Yes. But that's imo out of scope for this workaround PR.

For sure out of scope, yes, I didn't mean to imply that should be done here.

💬 sipa commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#issuecomment-2660238149)
utACK 99755e04ffadd5daad53c686f4f0feee2232eeb2

👍 theuni approved a pull request: "cmake: Add `libbitcoinkernel` target"
(https://github.com/bitcoin/bitcoin/pull/31869#pullrequestreview-2618778587)
ACK 3a914ab96bdeb70cdf848dd74deda5a80d0a7f78

Tested working as intended. Building/installing both targets works identically, no unnecessary rebuilds or installs either way.

Thanks hebasto!