💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770548378)
same, I think we can preallocate these as well:
```C++
vGetData.reserve(vToDownload.size());
for (const CBlockIndex *pindex : vToDownload) {
```
followed by
```C++
const auto& tx_requests = m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time);
vGetData.reserve(vGetData.size() + tx_requests.size());
for (const GenTxid& gtxid : tx_requests) {
```
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770548378)
same, I think we can preallocate these as well:
```C++
vGetData.reserve(vToDownload.size());
for (const CBlockIndex *pindex : vToDownload) {
```
followed by
```C++
const auto& tx_requests = m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time);
vGetData.reserve(vGetData.size() + tx_requests.size());
for (const GenTxid& gtxid : tx_requests) {
```
💬 Sjors commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2367873049)
> Dropped the first refactoring commit
Do you think that was the cause of the indeterminism, or are you still investigating that?
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2367873049)
> Dropped the first refactoring commit
Do you think that was the cause of the indeterminism, or are you still investigating that?
💬 Sjors commented on pull request "doc: macOS 15 ships llvm 16":
(https://github.com/bitcoin/bitcoin/pull/30949#issuecomment-2367877024)
Just tested that after manually installing Xcode 15.2 on macOS 13.7 (the last supported version on the most recent 27" iMac), picking it with `xcode-select`, I can still build the project. No need to install llvm via Homebrew.
That's the most recent version you can use. As @maflcko pointed out, Xcode 15.0 should work too, since the Github CI macOS 14 jobs picks it.
(https://github.com/bitcoin/bitcoin/pull/30949#issuecomment-2367877024)
Just tested that after manually installing Xcode 15.2 on macOS 13.7 (the last supported version on the most recent 27" iMac), picking it with `xcode-select`, I can still build the project. No need to install llvm via Homebrew.
That's the most recent version you can use. As @maflcko pointed out, Xcode 15.0 should work too, since the Github CI macOS 14 jobs picks it.
💬 ajtowns commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2367878874)
> an attacker cannot have an unbounded number of low-latency connections.
You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2367878874)
> an attacker cannot have an unbounded number of low-latency connections.
You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)
💬 andrewtoth commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2367890416)
In light of this, I think a better approach is to track the available memory in the freelist in the PoolAllocator and expose it in a public getter. Then, discount that available memory in `CCoinsViewCache::DynamicMemoryUsage`. And finally, revert 5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 and achieve the speedup.
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2367890416)
In light of this, I think a better approach is to track the available memory in the freelist in the PoolAllocator and expose it in a public getter. Then, discount that available memory in `CCoinsViewCache::DynamicMemoryUsage`. And finally, revert 5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 and achieve the speedup.
🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2321884746)
ACK f20fe33
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2321884746)
ACK f20fe33
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771218843)
I thought of that, but since it has to be different from `base_value`, seems weird to assume it that in a different method instead of the call-site.
```
Base Write Cache Expected Coinbase
CheckAddCoin(base_value, VALUE3, SPENT_DIRTY, VALUE3_DIRTY, false); // VALUE3 + VALUE3_DIRTY -> VALUE3_DIRTY
```
makes more sense than
```
Base Cache Expected Coinbase
CheckAddCoin(base_value,
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771218843)
I thought of that, but since it has to be different from `base_value`, seems weird to assume it that in a different method instead of the call-site.
```
Base Write Cache Expected Coinbase
CheckAddCoin(base_value, VALUE3, SPENT_DIRTY, VALUE3_DIRTY, false); // VALUE3 + VALUE3_DIRTY -> VALUE3_DIRTY
```
makes more sense than
```
Base Cache Expected Coinbase
CheckAddCoin(base_value,
...
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1771221285)
Added some unit tests yesterday btw, should definitely catch this in I think 2 cases.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1771221285)
Added some unit tests yesterday btw, should definitely catch this in I think 2 cases.
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771249650)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771249650)
Done, thanks
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2367971650)
> set `CCACHE_COMPILERCHECK` to `content` mode, which passes the compiler check when the hash of the compiler is the same (the default is `mdate` which will change with each container we create)
Are you sure? At least on Ubuntu Noble `mdate` is set correctly for `clang` (and does not change with each container). I presume the same is true for Debian and GCC. So, if the `modify` is different, the content would be different as well (and vice-versa). See:
```
# stat /usr/bin/clang++
File:
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2367971650)
> set `CCACHE_COMPILERCHECK` to `content` mode, which passes the compiler check when the hash of the compiler is the same (the default is `mdate` which will change with each container we create)
Are you sure? At least on Ubuntu Noble `mdate` is set correctly for `clang` (and does not change with each container). I presume the same is true for Debian and GCC. So, if the `modify` is different, the content would be different as well (and vice-versa). See:
```
# stat /usr/bin/clang++
File:
...
💬 l0rinc commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368014082)
> Didn’t #28280 resolve this?
That's why I was checking it, I've rebased it on [9cb9651d92ddb5d92724f6a52440601c7a0bbcf8](https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8) before running it with `-prune`.
@andrewtoth is this what you expected to happen, or was I measuring something wrong here?
<details>
<summary>Details</summary>
```bash
# git rev-parse HEAD
6d7f24595b08b8d1eba53e648533bcf87c30b48f
# git log --grep="bitcoin/bitcoin#28280"
commit
...
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368014082)
> Didn’t #28280 resolve this?
That's why I was checking it, I've rebased it on [9cb9651d92ddb5d92724f6a52440601c7a0bbcf8](https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8) before running it with `-prune`.
@andrewtoth is this what you expected to happen, or was I measuring something wrong here?
<details>
<summary>Details</summary>
```bash
# git rev-parse HEAD
6d7f24595b08b8d1eba53e648533bcf87c30b48f
# git log --grep="bitcoin/bitcoin#28280"
commit
...
📝 stratospher opened a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951)
resolves #29618.
This PR adds a config flag option `-v2onlyclearnet` which disallows v1 connections on ipv4 and ipv6 networks since they're unencrypted. v1 connections are still possible from tor/i2p/cjdns peers since they're encrypted networks.
- v1 outbound connections to peers are not attempted
- v1 reconnections are not attempted (if peer is falsely advertised as v2 peer and we attempt a v2 connection but it fails, we do not attempt a v1 reconnection when `-v2onlyclearnet` is switched
...
(https://github.com/bitcoin/bitcoin/pull/30951)
resolves #29618.
This PR adds a config flag option `-v2onlyclearnet` which disallows v1 connections on ipv4 and ipv6 networks since they're unencrypted. v1 connections are still possible from tor/i2p/cjdns peers since they're encrypted networks.
- v1 outbound connections to peers are not attempted
- v1 reconnections are not attempted (if peer is falsely advertised as v2 peer and we attempt a v2 connection but it fails, we do not attempt a v1 reconnection when `-v2onlyclearnet` is switched
...
💬 stratospher commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2368065103)
opened a possible solution in https://github.com/bitcoin/bitcoin/pull/30951
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2368065103)
opened a possible solution in https://github.com/bitcoin/bitcoin/pull/30951
💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368086019)
> > Dropped the first refactoring commit
>
> Do you think that was the cause of the indeterminism, or are you still investigating that? (might be a followup)
Another commit with a fix for indeterminism has been pushed.
The PR description has been updated accordingly.
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368086019)
> > Dropped the first refactoring commit
>
> Do you think that was the cause of the indeterminism, or are you still investigating that? (might be a followup)
Another commit with a fix for indeterminism has been pushed.
The PR description has been updated accordingly.
👋 stratospher's pull request is ready for review: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988)
(https://github.com/bitcoin/bitcoin/pull/26988)
🤔 furszy reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2321929633)
Code review ACK
Commit 19bc71caf6a43702946f1bc15696fe14afa24fda description says "delay" when it should say "check_interval".
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2321929633)
Code review ACK
Commit 19bc71caf6a43702946f1bc15696fe14afa24fda description says "delay" when it should say "check_interval".
💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771246533)
nit:
`PEP 8: E302 expected 2 blank lines, found 1`
Should add another blank line above and below this function.
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771246533)
nit:
`PEP 8: E302 expected 2 blank lines, found 1`
Should add another blank line above and below this function.
💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771251127)
ultra tiny nit: same code, a bit shorter.
```python3
predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
while True:
if not predicate():
raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
if time.time() > time_end:
return
time.sleep(check_interval)
```
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771251127)
ultra tiny nit: same code, a bit shorter.
```python3
predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
while True:
if not predicate():
raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
if time.time() > time_end:
return
time.sleep(check_interval)
```
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2368089819)
Rebased. thanks @jonatack! reaching out.
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2368089819)
Rebased. thanks @jonatack! reaching out.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2368093192)
@glozow @instagibbs It occurs to me that with the change in this PR, the `Cluster` will really only be used in tests anymore, as `DepGraph` objects can and will be built incrementally using `AddTransaction`, `AddDependencies`, and `RemoveTransactions`.
Because of that, I think it would be simpler to get rid of `Cluster` entirely, and reformulate the tests to all operate on `DepGraph` directly. I can do this in this PR, or as a follow-up. What do you think?
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2368093192)
@glozow @instagibbs It occurs to me that with the change in this PR, the `Cluster` will really only be used in tests anymore, as `DepGraph` objects can and will be built incrementally using `AddTransaction`, `AddDependencies`, and `RemoveTransactions`.
Because of that, I think it would be simpler to get rid of `Cluster` entirely, and reformulate the tests to all operate on `DepGraph` directly. I can do this in this PR, or as a follow-up. What do you think?