🤔 theuni reviewed a pull request: "crypto, refactor: add method for applying the taptweak"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2050167463)
It's weird to see this hooked up but unused. It could also use some simple test vectors.

Mind killing both birds by adding some tests, at least 1 for each `merkle_root`?

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596806833)
This could use a comment.

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1596813747)
Yeah, the mock time seems to reduce the runtime by 5-10 secs. I'm guessing with 10 secs of wait it could be the case that the connection is still being tried, so the timeout hasn't been considered. I'll add 5 more.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2104694897)
> Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596815685)
That looks much better, thanks. Exposing the vector-like interface is a shame, but I don't see any way around it for `secp256k1_silentpayments_sender_create_outputs`.

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596841762)
Hmm, looking at the docs and playing around, `CMAKE_AR` seems more correct to me too. It seems like it should work correctly, what was the problem?

There's also `CMAKE_C_COMPILER_AR` which may be more correct?

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596844125)
There's also no CMAKE_NM, so that isn't going to work if we want to pass that through as well.

I see `CMAKE_NM` in my `CMakeCache.txt`, maybe it's just not documented for some reason?

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2104736197)
Comparison at height 831,370:

* without cut through: GB @
* with cut through:

Next step is to (occasionally) prune the cut-through index.

I suspect the easiest way to is just to build a new index in the background, shut both the new and old indexes down, delete the old index and move the new one in place, and then turn it on again.

Another approach would be to override its contents block by block. If LevelDB handles resulting fragmentation in a sane way, this approach seems better
...

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596871427)
> Hmm, looking at the docs and playing around, `CMAKE_AR` seems more correct to me too. It seems like it should work correctly, what was the problem?

It seems `CMAKE_AR` is interpreted as relative to the build dir for some reason. What works for me is:
```
-DCMAKE_AR=`which ar`
```

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596873482)
Bug here: https://gitlab.kitware.com/cmake/cmake/-/issues/18087

💬 willcl-ark commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2104779556)
OK, I think I have pretty much identified the root cause here.

Calling [`blockToJSON`](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/rest.cpp#L340) on a maximum-sized block allocates 70-80MB on the heap during runtime.

There is then an additional 15-20MB allocated for `strJSON`.

After these allocations are made they then seem to be re-used when possible in the future, so long as subsequent blocks are small enough to fit. This totals up to the ~100M
...

📝 instagibbs opened a pull request: "test: expand LimitOrphan and EraseForPeer coverage"
(https://github.com/bitcoin/bitcoin/pull/30082)
Inspired by refactorings in #30000 as the coverage appeared a bit sparse.

Added some minimal border value testing, timeouts, and tightened existing assertions.

✅ nimrare closed an issue: "libxcb-xinerama0 Library required by bitcoin-qt"
(https://github.com/bitcoin/bitcoin/issues/30061)

💬 nimrare commented on issue "libxcb-xinerama0 Library required by bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2104847558)
@sipa @laanwj Okay, thank you for your response on this. I see your points and also agree that btc core can't and shouldn't deal with os related graphics peculiarities. Just from a UX/security perspective, I was a bit surprised about it when first encountered. Maybe it could be made optional at some point or a more verbose error message can be printed. In any case, no big issue. I'll close this!

💬 stickies-v commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2104860540)
> but it is then copied into a [second UniValue](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/rpc/blockchain.cpp#L196), doubling the required heap size.

And if I understand correctly, it is then copied _again_ [here](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/univalue/lib/univalue.cpp#L134) since `UniValue` doesn't have a move constructor. So improving UniValue move semantics seems to make sense here indeed.

👍 hebasto approved a pull request: "build: swap cctools otool for llvm-objdump"
(https://github.com/bitcoin/bitcoin/pull/29739#pullrequestreview-2050387339)
ACK 7f5ac4520d1553170b1053a9ffcd58179386a6d2.

After migration to CMake, there is a possibility that CMake will expect to find `otool` for the "Darwin" systems. But we can deal with this in due time.

💬 achow101 commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#issuecomment-2104876107)
ACK e912717ff63f111d8f1cd7ed1fcf054e28f36409

🤔 jonatack reviewed a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994#pullrequestreview-2050435690)
@vasild thank you, rectifying my comment.

ACK 95897ff181c0757e445f0e066a2a590a0a0120d2

New help:
```
$ ./src/bitcoind -h | grep -A2 "\-port\="
-port=<port>
Listen for connections on <port> (default: 8333, testnet: 18333, signet:
38333, regtest: 18444). Not relevant for I2P (see doc/i2p.md).
```

💬 libreisaac commented on issue "libxcb-xinerama0 Library required by bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2104915543)
> Okay, thank you for your response on this. I see your points and also agree that btc core can't and shouldn't deal with os related graphics peculiarities. Just from a UX/security perspective, I was a bit surprised about it when first encountered. Maybe it could be made optional at some point or a more verbose error message can be printed. In any case, no big issue. I'll close this!

If you're concerned about trusting binaries installed by your OS' package manager, consider Gentoo. But fundamen
...

💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596979700)
Done.