💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2862902153)
My Guix build:
```
aarch64
be6aa004944a606e9bbef9129b971960186a533f6572c5fb2e522fabc544c611 guix-build-477b69ad19d7/output/dist-archive/bitcoin-477b69ad19d7.tar.gz
e75f1ed645abf5d086f5e328107ce804e3ebec9ac5b2d0d8a9d037744e2e4859 guix-build-477b69ad19d7/output/x86_64-w64-mingw32/SHA256SUMS.part
526d76c4402086b1f8382d5b461162485efcaaf1ebf98a23dc9fd8b8f7bd62d8 guix-build-477b69ad19d7/output/x86_64-w64-mingw32/bitcoin-477b69ad19d7-win64-codesigning.tar.gz
11029fcb9a8f11c7df44b2dfbda58392d22
...

💬 fanquake commented on pull request "dbwrapper: Bump LevelDB max file size to 32 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2862903991)
Anyone that reviewed here, might be interested in https://github.com/bitcoin-core/leveldb-subtree/pull/52.

👍 laanwj approved a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445#pullrequestreview-2824986604)
Code review ACK dfeac76b951fc2d28e0832b176f3c8ad595697f1

For example:
```sh
$ export SOURCE_DATE_EPOCH=1
$ cmake -B build
$ grep YEAR build/src/*.h
build/src/bitcoin-build-config.h:#define COPYRIGHT_YEAR 1970
```
In the guix build it sets `SOURCE_DATE_EPOCH` based on the last commit
```
SOURCE_DATE_EPOCH="${SOURCE_DATE_EPOCH:-$(git -c log.showSignature=false log --format=%at -1)}"
```
So ostensibly this works out the correct date deterministically.

Should test an actual guix bui
...

👍 theStack approved a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2824981042)
re-ACK f09e1f943dbd1e5865dece3c35f342e4f0b31b9e :writing_hand: :hash:

💬 theStack commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2079628809)
nitty nit: now that the checks are done via `testmempool` above, these two lines could be deleted

💬 fanquake commented on pull request "doc: swap "Docker image" for "container image"":
(https://github.com/bitcoin/bitcoin/pull/32444#issuecomment-2862942029)
Pushed a commit up here: https://github.com/fanquake/core-review/commit/1656c4891dc1ac83b66765ba5a2607815b91d6eb.

🚀 fanquake merged a pull request: "doc: swap "Docker image" for "container image""
(https://github.com/bitcoin/bitcoin/pull/32444)

💬 hebasto commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079675289)
"Define to 1 if '*ifaddrs' are available" implies that "it remains undefined otherwise". To achieve this, I suggest:
```suggestion
/* Define to 1 if '*ifaddrs' are available. */
#cmakedefine HAVE_IFADDRS 1
```

This also allows to replace the following `#if HAVE_IFADDRS` with `#ifdef HAVE_IFADDRS`.

💬 hebasto commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2863028605)
> No movement in #31507, but could rebase & update this with the one new commit?

Sure! Rebased and updated.

💬 laanwj commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#issuecomment-2863033532)
ACK
i think this would make it slightly more clear what is going on with regard to ranges and validity:
```diff
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index 33a4555dd595c9e4886f194cb113b905a55bddb3..6f251b094c5b68373f30d1d578088e65d4595a09 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -148,11 +148,12 @@ void static NegateSignatureS(std::vector<unsigned char>& vchSig) {
while (s.size() < 33) {
s.insert(s.begin(), 0x00)
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2863039003)
`1c16944a4a...32488cfd6c`: rebase due to conflicts

🤔 rkrux reviewed a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2825019929)
Concept ACK bfb7b8876035774eddb200d741aa6a86859069b3

Conceptually, I'm in agreement with the replacement of custom negation code by using the one from secp256k1. Though I was a little surprised to note that git grep of this function doesn't lead to any actual usages of the function besides the tests & `secp256k1_ec_privkey_negate` function that too doesn't seem to be used elsewhere.

```bash
git grep -n "secp256k1_ec_seckey_negate"

src/secp256k1/include/secp256k1.h:699:SECP256K1_API SEC
...

💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079651302)
> Note that as the name suggests, secp256k1_ec_seckey_negate is meant to be used for secret keys, but it obviously works in general for scalars modulo the group order.

I can see a call to `secp256k1_scalar_negate` inside `secp256k1_ec_seckey_negate`.

https://github.com/bitcoin/bitcoin/blob/64697529523705f5a411ca893060429afcd5cd47/src/secp256k1/src/secp256k1.c#L622-L635

Nit: However, the current comment makes one wonder why it is (ab)using libsecp256k1, which is answered in the commit m
...

💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079666382)
I can see the earlier implementation also ended the iteration at second index (1), which is ok.

However, the `+ 1` is quite apparent now & I'm not entirely sure why this is the case.
It would be helpful to know the reason and document it in code if possible.

💬 fanquake commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2863061380)
Guix Build:
```bash
cab0e4d3d2602d2eaa8893ae5294bd736b514fea6deb72df03c4d693971282dd guix-build-7343a1846ceb/output/aarch64-linux-gnu/SHA256SUMS.part
21163373fc74e834f3d81309888fc4cd99719462be8fef447e535fb56aa5ce1e guix-build-7343a1846ceb/output/aarch64-linux-gnu/bitcoin-7343a1846ceb-aarch64-linux-gnu-debug.tar.gz
b09ca7a33bf16d3e97bce8800bf57a06402a9cf4860d327be3d6571a56be598c guix-build-7343a1846ceb/output/aarch64-linux-gnu/bitcoin-7343a1846ceb-aarch64-linux-gnu.tar.gz
f1b2aa3edd0ce6b4
...

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2079717534)
I feel it's fine to keep them - imho the ultimate test is whether the transaction can be mined, gives me more confidence.

💬 laanwj commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079719628)
Would it be possible to work around this in the build script somehow?

i guess not, i mean except by changing a global git setting there, which would be awful behavior...

💬 laanwj commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079728574)
The reason is that the first byte cannot have the upper bit set, as this would mean a negative number, which would be illegal here. So the number can be 33 bytes long (with a 0x00 prefixed) even though it fits in 32 bytes.

💬 vasild commented on issue "rfc: only relay transactions to v2 encrypted peers":
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2863104253)
I agree with @mzumsande above, also:

If a node has even a single v2 connection that already makes it nearly impossible for a passive spying ISP to conclude it is the origin of a transaction, no?

Lets assume a node has only v1 connections, then the ISP can observe all transactions going to it and all transactions going out of it. As soon as a transaction is seen going out that did not come in, a conclusion can be made that it originated from the node.

However, if there are v2 connections, even
...

💬 theuni commented on pull request "crypto: disable ASan for sha256_sse4 with Clang":
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2863107504)
Post-merge ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8. Looks like the right fix to me.