🤔 stickies-v reviewed a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-2877628308)
Concept ACK, the new `GenTxid` allows us to better benefit from the type safety of `Txid` and `Wtxid` and this PR takes a clear and structured approach that seems straightforward to review.

Approach-wise, I think I have a slightly different view. I find code generally easier to understand when we use the underlying types {`Txid`, `Wtxid`} directly and as such would prefer functions to be overloaded with those types, pushing conversion to the edge as much as possible. This also has the side be
...

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2113608840)
In 811d2f34a0309a62a69897a15db47214bbfec776:

Isn't it more straightforward to just to have two `bool exists(const Txid& txid)` and `bool exists(const Wtxid& wtxid)` overloads?

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2113548178)
nit: can also be implemented as a single three-way operator:

```suggestion
friend auto operator<=>(const GenTxid& a, const GenTxid& b) {
return std::tuple(a.index(), a.ToUint256()) <=> std::tuple(b.index(), b.ToUint256());
}
```

(needs to include `<compare>` and `<tuple>`)

💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2113659770)
Similarly to my comment about `TxMempool::exists()`, I think it would be better here to just overload `info`:

<details>
<summary>git diff on ad6d02720c</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c0f6ac3f03..c34511d27d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5821,7 +5821,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
continue;
}

...

🤔 i-am-yuvi reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2877825924)
Post-Merge ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9

💬 fanquake commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2919014673)
> A fallback provider: This will use system packages when they are found and fall back to compiling vendored depends. Using this provider, find_package will always succeed, but it will not compile any "unnecessary" code. This approach may be preferred by developers or for quick experiments.

I can't tell from this if you are saying to use depends to "fill in" any missing system packages, or, if all the required system packages aren't available, we would switch to using depends (unclear if that
...

💬 hebasto commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2919073897)
I believe the manifest could be further simplified:
```diff
--- a/contrib/guix/manifest.scm
+++ b/contrib/guix/manifest.scm
@@ -18,7 +18,7 @@
((gnu packages python-build) #:select (python-poetry-core))
((gnu packages python-crypto) #:select (python-asn1crypto))
((gnu packages python-science) #:select (python-scikit-build-core))
- ((gnu packages python-xyz) #:select (python-pydantic-2 python-pydantic-core))
+ ((gnu package
...

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2113740976)
Could we defer such amendments to follow-ups until feedback from real-world CI usage has been collected?

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2919095363)
@hebasto @davidgumberg I believe all comments have been addressed?

💬 marcofleon commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2113749842)
I think the comment makes sense here, even if it is a bit verbose. Happy to change it once a fix such as https://github.com/bitcoin/bitcoin/pull/32313 gets merged.

💬 marcofleon commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2113750187)
Done

💬 marcofleon commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2113750716)
Done.

💬 marcofleon commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2113750909)
Done.

👍 hebasto approved a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-2877956479)
ACK ff2e35f6315cc6b912f68fe85103575d751bd2bc.

While touching this code, it seems reasonable to also address the other @davidgumberg's [comment](https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325):
> Might be nice to have a CI check like [davidgumberg@1b98160](https://github.com/davidgumberg/bitcoin/commit/1b9816052b1d8ad1d9c17945530e14ead79fce33) to prevent future regressions, but OK for a separate PR if out of scope.

💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2919114726)
> While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the iwyu_tool.py script or CMake's built-in functionality.

Reading the PR description, it's not really clear why we'd need to retain using both tools? Either the CMake built-in should be better in some way, or at least equivalent in functionality, in which case we should just switch to using it?

💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2113759162)
Is there an issue open for this upstream?

💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2113759342)
Same here?

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2919117368)
> While touching this code, it seems reasonable to also address the other @davidgumberg's [comment](https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325):

I don't mind adding that.

👍 hodlinator approved a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633#pullrequestreview-2877972468)
re-ACK 55f1c2ac8beb5ebebcaef0707d8cc3003e41d80e

Built x86_64-w64-mingw32 Guix build locally.

💬 hodlinator commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2919144295)
I suspect checking for manifests will currently fail for bench_bitcoin.exe as it doesn't yet have a manifest. Not sure about other binaries. Wouldn't be against adding missing manifests unless it slows down the builds noticeably.