💬 fanquake commented on pull request "windows: Use predefined `RC_INVOKED` macro instead of custom one":
(https://github.com/bitcoin/bitcoin/pull/32633#issuecomment-2918929592)
This needs rebasing for #32634; otherwise, I'm not sure how a release build of this was tested, given the Windows Guix build is broken in this branch.

💬 fanquake commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2918940425)
https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223
```bash
[12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
[12:12:21.305] 23 | AssertLockNotHeld(mutex);
[12:12:21.305] | ^
[12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
[12:12:21.305] 141 | #define AssertLockNotHeld(cs
...

💬 hebasto commented on pull request "windows: Use predefined `RC_INVOKED` macro instead of custom one":
(https://github.com/bitcoin/bitcoin/pull/32633#issuecomment-2918956308)
> This needs rebasing for #32634; otherwise, I'm not sure how a release build of this was tested, given the Windows Guix build is broken in this branch.

Sure! Rebased.

💬 dergoegge commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-2918966148)
* Looking at `MaybeSetPeerAsAnnouncingHeaderAndIDs`, becoming a node's hb peer is pretty easy? I.e. be the first to supply the latest valid block at the tip and you'll be selected as high bandwidth (we process unsolicited block messages, so this should be trivial).
* Even if you're low-bandwidth can't you just send a headers announcement immediately followed by the compact block? As long as your headers message is the first announcement of the block, the compact block will be requested and sinc
...

🤔 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?