💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1596384279)
I fully agree what you've said. I'm leaving as is for now. If #26593 is merged and I retouch this, I'll reconsider changing this. I'll leave this as "unresolved" for now.

💬 Sjors commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2104114821)
tACK 57a06646952fed98c1c281f02fe58a0758a8ed5a

It seems that BIP341 introduced H as just an example of a NUMS point:

> One example of such a point is H

The BIP got it from libsecp256k1 which IIUC only uses it for tests. As does Bitcoin Core so far.

[BIP352](https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki) gives it special status:

> The one exception is script path spends that use NUMS point H as their internal key

💬 S3RK commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2104126580)
Code Review ACK 57a06646952fed98c1c281f02fe58a0758a8ed5a

The constant definition is consistent with BIP-341. PR removes code duplication and provides a single place to refer to `NUMS_H` either as vector of bytes or x-only pubkey.

👍 laanwj approved a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2049515955)
Agree these don't need to be logged at high severity, this matches our general idea that network problems on the open internet are normal and there's no reason to scream at the user for them.
ACK 7524aaf58f5dbe077d4997b3a5dcf635a027a659

💬 willcl-ark commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/29888#issuecomment-2104165366)
reACK bd5860bc7a892c6bcffe313246dd6b81b973b9c6

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1596441523)
dropped in 8109319e102c41d3aeed0ecfbc3a0e23b7fea807

👍 kristapsk approved a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2049587375)
cr utACK 7524aaf58f5dbe077d4997b3a5dcf635a027a659

👍 stickies-v approved a pull request: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/29888#pullrequestreview-2049591642)
re-ACK bd5860bc7a892c6bcffe313246dd6b81b973b9c6

💬 rkrux commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1596471709)
I see, the part I'm a little confused about is that how can we test for something that's not done yet. Is the intention to have this test written before the topology is relaxed so that we have more robustness in the tests?

Also, am I correct to assume that this error message would need to be updated when the topology is relaxed later? `'package_msg': 'package-not-child-with-unconfirmed-parents'`

💬 fanquake commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#issuecomment-2104234716)
Guix Build (aarch64):
```bash
cf21ea004d1f8d6d7b53d4ef725083229b18469948439f79872a2b019e8bc966 guix-build-7efd6bf03755/output/aarch64-linux-gnu/SHA256SUMS.part
1fa79040aba310b34075b11eefbdb570dd1f053d25ea432faf9bca596603cabb guix-build-7efd6bf03755/output/aarch64-linux-gnu/bitcoin-7efd6bf03755-aarch64-linux-gnu-debug.tar.gz
a97ee58be3ea463c565ec8098ed3f98a04f70b7a897c2b1961df083f5142e07b guix-build-7efd6bf03755/output/aarch64-linux-gnu/bitcoin-7efd6bf03755-aarch64-linux-gnu.tar.gz
1f4f55
...

📝 ismaelsadeeq opened a pull request: "Fee Estimation: Ignore all transactions with an in-block child"
(https://github.com/bitcoin/bitcoin/pull/30079)
Another attempt at #25380 with an alternate approach

This PR updates `CBlockPolicyEstimator` to ignore all transactions with an in-block child when a new block is received.
This fixes the assumption that all transactions are confirmed solely because of their fee rate.
As some transactions are confirmed due to a CFPP by some child.


The downside of this approach is that transactions that are not CPFP’d will aslo be ignored.

The correct approach is to linearize all transactions remov
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2104258773)
Rebased and renamed to `-bip352index` (stored in `indexes/bip352`). Presumably a future version will have its own BIP number, and by default get a fresh index. By that time we can figure out if it's better to expand the existing index or have two separate non-overlapping indexes.

The index functions `GetSilentPaymentKeys` and `FindSilentPayment` are kept generic, so we have the option of subclassing the index.

I kept the `getsilentpaymentblockdata` RPC name generic, but renamed to `tweaks`
...

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2104281397)
> Rebased (on https://github.com/bitcoin/bitcoin/pull/28122) and renamed to -bip352index (stored in indexes/bip352).

Nice! Worth mentioning that now the indexes can be built without needing the wallet to be compiled.

💬 hebasto commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596566961)
In contrary to the `CC` and `CXX` environment variables, CMake [docs](https://cmake.org/cmake/help/latest/manual/cmake-env-variables.7.html) do not mention that `AR` and `RANLIB` are honored as well.

I suggest to use [`CMAKE_AR`](https://cmake.org/cmake/help/latest/variable/CMAKE_AR.html) and [`CMAKE_RANLIB`](https://cmake.org/cmake/help/latest/variable/CMAKE_RANLIB.html) instead.

💬 fanquake commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596571855)
I tried both of those before landing on this change, and they didn't seem to work properly. Happy to have a look if you can show a patch that works. There's also no CMAKE_NM, so that isn't going to work if we want to pass that through as well.

👍 AngusP approved a pull request: "test: switch from curl to urllib for HTTP requests"
(https://github.com/bitcoin/bitcoin/pull/29970#pullrequestreview-2049781449)
crACK 7fe94f79c90b689dce89b287d1016a97218021f6

💬 AngusP commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1596570893)
AFAIK there's a minor behaviour change here, where `urlopen` *will* follow redirects whereas `curl` won't usually

```shell
$ curl -I https://httpbin.org/absolute-redirect/3
HTTP/2 302
# ...
```

```python
>>> from urllib.request import urlopen
>>> response = urlopen("https://httpbin.org/absolute-redirect/3")
>>> response.code
200 # Not 302 because redirects were followed
```

This should be fine, but worth a mention.

💬 TheCharlatan commented on pull request "build: swap cctools otool for llvm-objdump":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2104354645)
> I've rebuilt, and see matching now (you and Sjors)

Also refired and got the same hashes again. Might have been something else unrelated to this PR?

ACK 7f5ac4520d1553170b1053a9ffcd58179386a6d2

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2104363831)
`5543990321...1fa61d9edc`: rebase and log a message from `~AutoFile()` if closing fails and terminate the program.

👋 vasild's pull request is ready for review: "util: check for errors after close and read in AutoFile"
(https://github.com/bitcoin/bitcoin/pull/29307)