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

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549880323)
I think the same applies here as in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549873306, the suggested version focuses on the happy path (which is the purpose of the method - and it's also easer to say out loud), which I found helps with comprehension.

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

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563180629)
> > > Not sure I understand the dependence on #33775?
> > > CI is using it's own build, not the guix build right? Also MinGW brings it's own GCC doesn't it?
> >
> >
> > The CI jobs are more useful when using toolchains with versions similar to those in the Guix script.
>
> I think this is just a doc question. I think the `trixie/g++-mingw-w64-ucrt64` task can be left as-is, without adding a dependency here. In the future, #33593 could simply update the comment to clarify which one is ru
...

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

"ignored" clarifies the value is not read at all, while "shouldn't matter" could mean read-but-no-effect. Very minor preference though, I think either can works just fine

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549896888)
Need to pick the right name here.

👍 maflcko approved a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3492941007)
lgtm

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549906416)
Thanks, I prefer the previous versions since it's higher level, it's expressing the intent, not the implementation detail

🤔 hodlinator reviewed a pull request: "build: Embedded ASMap [3/3]: Build binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3492585142)
### Concept ACK 1dcd2e8a983406d12009e41bac5dbaeb12f330be

I do understand the distaste at adding 1MB+ binary blobs into a reproducible build. From that perspective, it would be cleaner to include the ASMap blob as a separate file included in our Bitcoin Core releases.

But having a separate file opens up the can of worms of where to install it on various operating systems and distributions, how to encourage package maintainers to include and ensure it ends up in the right locations, and making B
...

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2549691949)
nit: Convention seems to be

```suggestion
if(WITH_EMBEDDED_ASMAP)
```

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2549632572)
PR description nit:
```diff
- -DWITH_EMBEDDED_ASMAP=NO
+ -DWITH_EMBEDDED_ASMAP=OFF
```

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549938061)
Thanks! Fixed.