💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549734317)
In general I prefer to have less nesting. It was in some book and I agree that more nesting requires more cognitive load to process. Maybe not necessary in this case because here the nested code is very short.

I find `foo->first` and `foo->second` very unreadable and prefer to always give them a name by assigning them to a named variable. `Txid` in this case.

I prefer:
```cpp
int x = expression;
if (x == 5) {
```
instead of
```cpp
if (int x = expression; x == 5) {
```
it is subjec
...

💬 fanquake commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549737017)
> Only in rare cases

I don't think wanting to build without having a GCC installation should be considered rare? Other than the clang-only Nix example linked too above, we are actively trying to make that "normal" for some of our builds, i.e #30206.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549737708)
Same: https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549734317

💬 fanquake commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641)
> They should be exactly equivalent.

Unfortunately this isn't the case. `make build_CC=clang build_CXX=clang++ host_CC=clang host_CXX=clang++` doesn't work properly on a machine with no `g++`, because Boosts CMake will still try and find `g++`, and fail. Arguably this is also a CMake issue that we should fix/workaround in some way, because there's no reason for Boost to try and find a compiler, when we aren't compiling anything, and only need to copy headers. In this case, you do need to use
...

🤔 fanquake requested changes to a pull request: "doc: Document compiler configuration for native depends packages"
(https://github.com/bitcoin/bitcoin/pull/33902#pullrequestreview-3492766868)
See https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641.

✅ diegoviola closed a pull request: "Fix bitcoin-qt visual glitches on Wayland"
(https://github.com/bitcoin-core/gui/pull/904)

💬 diegoviola commented on pull request "Fix bitcoin-qt visual glitches on Wayland":
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3563050872)
Closing in favor of [#904.](https://github.com/bitcoin-core/gui/pull/914).

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3563051500)
I'm not sure what the comment about motivation means, but I appreciate your time.

💬 diegoviola commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3563080669)
@hebasto ACK. Thanks for looking into this and for the revert. :+1:

💬 frankomosh commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549829849)
```suggestion
bool mutated = fuzzed_data_provider.ConsumeBool(); // output param, initial value ignored
```

🤔 l0rinc reviewed a pull request: "coins: use number of dirty cache entries in flush warnings/logs"
(https://github.com/bitcoin/bitcoin/pull/33512#pullrequestreview-3492784897)
Thanks for the comments @optout21, since I wasn't invalidating any ACKs, I have applied your suggestions (with rebase) - let me know if this is what you meant.

> keeping a count like the dirty count is very efficient, but prone to going out of sync

yeah, we have a few of these - but the sanitizers and sanity checks and fuzzers usually reveal the problems (sanitizers break on underflow and the sanity checks recalculate everything during testing).

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2549809335)
I have grouped dirty setter and dirty count updates deliberately:
```
% git grep -A1 'CCoinsCacheEntry::SetDirty' src/coins.cpp
src/coins.cpp: CCoinsCacheEntry::SetDirty(*it, m_sentinel);
src/coins.cpp- ++m_dirty_count;
--
src/coins.cpp: CCoinsCacheEntry::SetDirty(*it, m_sentinel);
src/coins.cpp- ++m_dirty_count;
--
src/coins.cpp: CCoinsCacheEntry::SetDirty(*it, m_sentinel);
src/coins.cpp- ++m_dirty_count;
--
src/coins.cpp: CCoinsCache
...

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2549782860)
Fair, changed both:
```C++
//! Size of the cache (in number of transaction outputs)
unsigned int GetCacheSize() const;

//! Number of dirty cache entries (transaction outputs)
size_t GetDirtyCount() const noexcept { return m_dirty_count; }
```

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2549775651)
You mean like:
```C++
BOOST_CHECK_EQUAL(dirty_count, 0U);
```

💬 hebasto commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3563118321)
> I'm not sure what the comment about motivation means...

It means that the following only describes the feature itself, without providing the goals, use cases, or rationale behind it:
> GUI: Add coins tab and sets view-only mode
>
> Adds Coins (UTXOs) tab at the top and:
>
> * Removes selection checkboxes from the dialog
>
> * Disables coin selection functionality
>
> * Maintains UTXO viewing capabilities
>
> * Removes all the information from the top area of the
...

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549840441)
Why is that better?

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3563134954)
> > I'm not sure what the comment about motivation means...
>
>
>
> It means that the following only describes the feature itself, without providing the goals, use cases, or rationale behind it:
>
> > GUI: Add coins tab and sets view-only mode
>
> >
>
> > Adds Coins (UTXOs) tab at the top and:
>
> >
>
> > * Removes selection checkboxes from the dialog
>
> >
>
> > * Disables coin selection functionality
>
> >
>
> > * Maintains UTXO viewing capabilities
>
> >
>
> > *
...

💬 frankomosh commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549851020)
nit

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549873306)
My motivation for the if-init version was just to make the “happy path” a single block, to me that reads as “if we have a node for this id and a matching tx, return it, otherwise return nullopt”, which matches how I’d describe the code in English. The previous version is more verbose in English and focuses on the unlikely scenarios which I find distracting.

On the first/second bit: those can also be avoided with destructuring when they start to hurt readability, e.g.:
```C++
const auto [nod
...

👋 hebasto's pull request is ready for review: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764)