💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2079035706)
Tested the Guix build on Windows 11.

💬 fanquake commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2079037412)
> Surely this isn't the only place env vars like this would cause issues? Feels like a cat-and-mouse game.

Yea, as-is this doesn't seem like a great fix, and may just break other things?

A better diff might be something like:
```diff
diff --git a/depends/Makefile b/depends/Makefile
index 005d9696fb..091511758d 100644
--- a/depends/Makefile
+++ b/depends/Makefile
@@ -51,7 +51,7 @@ FALLBACK_DOWNLOAD_PATH ?= https://bitcoincore.org/depends-sources
C_STANDARD ?= c11
CXX_STANDARD ?= c
...

👍 hebasto approved a pull request: "refactor: Avoid unused-variable warning in init.cpp"
(https://github.com/bitcoin/bitcoin/pull/29968#pullrequestreview-2024633825)
ACK fa5ba429a71eb81848560b4a547d1717efd33593, tested on Ubuntu 24.04 with `-Wall`.

💬 Sjors commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2079052784)
@laanwj did you mean `'-b', BRANCH` instead of `'-b', branch`?

I also get a difference on qrencode when running this on macOS 13.6.6 with Python 3.12.1.

💬 laanwj commented on pull request "guix: remove bzip2 from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2079056842)
Contents are the same, there is a slight difference in the metadata:
```
--- qrencode-4.1.1.tar_gz.hex 2024-04-26 11:58:29.335356741 +0200
+++ qrencode-4.1.1.tar_bz2.hex 2024-04-26 11:58:16.387499575 +0200
@@ -4,7 +4,7 @@
00000060 00 00 00 00 30 30 30 30 37 35 35 00 30 30 30 31 |....0000755.0001|
00000070 37 35 30 00 30 30 30 31 37 35 30 00 30 30 30 30 |750.0001750.0000|
00000080 30 30 30 30 30 30 30 00 31 33 37 33 34 33 30 33 |0000000.13734303|
-00000090 36 36 36 00 30 3
...

💬 hebasto commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2079059322)
> A better diff might be something like:
> ...
> which would at least be using the option that is meant to be used for this.

I agree. Implemented.

Thanks!

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580742501)
Thanks, I've moved it to the outer loop.
Kept bool, seems harmless and I imagine we can find some more interesting code paths by sometimes not setting it immediately.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580739332)
As discussed offline, I agree the fuzz test has a few quirks that could be fixed up, but I think they are out of scope for this PR so I'm going to mark this resolved.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580743469)
added

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580756146)
haha oops, deleted

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580755767)
Ok I've done a bit of refactoring to split this into 2 functions for finding the package and validating/processing the package, and made a `PackageToValidate` struct that's difficult to misuse. Got rid of these "set me if you find something" variables.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580737130)
fixed

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580750294)
Packages aren't ever reconsiderable since we never submit anything beyond 1p1c. I've clarified this in the docs now. I definitely think we should cache a rejection when the error is `PCKG_POLICY` or `PCKG_MEMPOOL_ERROR`.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580736833)
added

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580698811)
done

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1580693314)
removed

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079095313)
@Sjors I've updated #28122 to sort outpoints lexicographically and added @setavenger 's test case. This should fix the discrepancy between your two indexes. Thanks again for testing this! Really glad we caught this one.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580842378)
7a27533332d14cf98f7e165f464142132e9f9d85: if you're going to change this here instead of adding a custom sort for BIP352, then you should add a note that it's critical for BIP352.

However I'm not sure if we should sort this way by default. If I wanted to list transaction outputs, I'd want to sorted by their integer version.

That said, I don't know if we currently expose any RPC where this sort order is exposed.

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580846838)
From what I could tell, we don’t expose this anywhere. I’m in favor of making this the default since we only ever really talking about outpoints in terms of their serialised encoding. Gonna give it some more thought, but I’m considering opening this commit as its own PR. It seems preferable to me to have one way of sorting outpoints vs creating a special case for silent payments, especially when there doesn’t seem to be any other “sort outpoints” use case (e.g. none of the tests failed when I ma
...

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580850619)
But still need to verify re: displayed in RPCs. At that point tho, we could just sort by `vout` and ignore the txid in the sort