💬 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`
```
(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
(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
...
(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.
(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)
(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!
(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.
(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.
(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
(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).
```
(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
...
(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.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596979700)
Done.
🚀 achow101 merged a pull request: "test: add missing comparison of node1's mempool in MempoolPackagesTest"
(https://github.com/bitcoin/bitcoin/pull/29948)
(https://github.com/bitcoin/bitcoin/pull/29948)
💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2104921312)
@theuni Updated with a comment and added `KeyPair` to the BIP340 test vectors. This does test all possible merkle_root states and ensures everything is 1-to-1 with `XOnlyPubKey::ComputeTapTweakHash` and `CKey::SignSchnorr`
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2104921312)
@theuni Updated with a comment and added `KeyPair` to the BIP340 test vectors. This does test all possible merkle_root states and ensures everything is 1-to-1 with `XOnlyPubKey::ComputeTapTweakHash` and `CKey::SignSchnorr`
🤔 instagibbs reviewed a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2050241117)
reviewed through 65f69bf533f66ecfb2695b03204bd437f1fe692d
> Given that a Bitcoin Core node doesn't ever do witness replacement, this seems like unnecessary complexity that won't ever get used between normally-operating nodes. So I did not add this.
Makes sense, seems unlikely to ever be used in a 20 minute window lifetime of an orphan given that we don't do these kinds of replacements.
also opened https://github.com/bitcoin/bitcoin/pull/30082 seeing we're touching `EraseForPeer` et al a
...
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2050241117)
reviewed through 65f69bf533f66ecfb2695b03204bd437f1fe692d
> Given that a Bitcoin Core node doesn't ever do witness replacement, this seems like unnecessary complexity that won't ever get used between normally-operating nodes. So I did not add this.
Makes sense, seems unlikely to ever be used in a 20 minute window lifetime of an orphan given that we don't do these kinds of replacements.
also opened https://github.com/bitcoin/bitcoin/pull/30082 seeing we're touching `EraseForPeer` et al a
...
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596969302)
```Suggestion
# exists in orphanage, but should be re-requested due to having witness data.
```
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596969302)
```Suggestion
# exists in orphanage, but should be re-requested due to having witness data.
```
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596930516)
but `wait_for_verack=False` isn't being set?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596930516)
but `wait_for_verack=False` isn't being set?
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596975768)
```Suggestion
assert_equal(node.getrawmempool(), [])
# 5. The parent is requested. Honest peer sends it.
```
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596975768)
```Suggestion
assert_equal(node.getrawmempool(), [])
# 5. The parent is requested. Honest peer sends it.
```
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596966608)
```Suggestion
# 2. Node requests tx_grandparent by txid.
```
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596966608)
```Suggestion
# 2. Node requests tx_grandparent by txid.
```
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596929262)
but you are sending verack just below?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596929262)
but you are sending verack just below?