👍 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)

🤔 rkrux reviewed a pull request: "test: add missing comparison of node1's mempool in MempoolPackagesTest"
(https://github.com/bitcoin/bitcoin/pull/29948#pullrequestreview-2049811220)
tACK [e912717](https://github.com/bitcoin/bitcoin/pull/29948/commits/e912717ff63f111d8f1cd7ed1fcf054e28f36409)

Make successful, all functional tests pass. Left couple suggestions.
Also for some reason checking out this PR using this method doesn't work, I had to checkout this particular commit instead. Is it because the forked repo's branch is also called `master`?
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-
...

💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596588179)
With this, isn't the check on line 201 redundant now?

💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596588632)
Where is this done? Can you share link?