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

💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596591694)
Nit: This portion is copy-paste of the above block, can be extracted in a function `compareMempoolsEntry(entry0, entry1)`

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596601706)
[See this](https://github.com/bitcoin/bitcoin/pull/21800)

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596605908)
`mempool1` only stores string(txid), while `getmempoolentry` contains more information that we need to check, so no, it is not redundant. Also see: https://developer.bitcoin.org/reference/rpc/getmempoolentry.html

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596607978)
Same as above, imo definitely want to log txid if we have it.

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596610435)
Yes

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596607638)
Yes, I would definitely say to log both txid and wtxid when we have it. A lot of things use txid only, e.g. if you want to trace a tx through logs or look it up in mempool. Would be a pain to not have txid imo.

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596612470)
That's on purpose, just to align with the existing coding style within this file (and other testing code). Those unit tests don't actually introduce new functions while comparing two entries (and the entries' variables).

🤔 ismaelsadeeq reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2049842223)
Code Review ACK 6a39183b53a41bb282ebf8373d0e11691d97d365

💬 AngusP commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1596597207)
Nit:
```suggestion
from decimal import Decimal, ROUND_DOWN
```