💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475307568)
> Sorry for being unclear. I don't think there is any broken assumption either.
> However, if we made this a non-atomic as suggested, then _if_ something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
But as you said, the thread sanitizer would likely complain about that. We’re accessing the same variable from different threads (main and worker). The main issue, however, will probably be r
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475307568)
> Sorry for being unclear. I don't think there is any broken assumption either.
> However, if we made this a non-atomic as suggested, then _if_ something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
But as you said, the thread sanitizer would likely complain about that. We’re accessing the same variable from different threads (main and worker). The main issue, however, will probably be r
...
💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475344730)
+1 for separate test cases for solicited / unsolicited. I agree the test can be improved in a dedicated PR.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475344730)
+1 for separate test cases for solicited / unsolicited. I agree the test can be improved in a dedicated PR.
💬 Psifour commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463882411)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that speci
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463882411)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that speci
...
💬 ajtowns commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765)
> After [#27675](https://github.com/bitcoin/bitcoin/pull/27675) (but also before, with a few more restrictions), we relay any tx from our mempool (except for those that arrived after the last INV sent to the peer), so someone wanting to fingerprint our mempool could just ask for those transactions directly instead I think and then check whether we send it to them or answer with `NOTFOUND`.
I don't think this issue makes sense as an attack -- we aim to avoid giving away tight timing information
...
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765)
> After [#27675](https://github.com/bitcoin/bitcoin/pull/27675) (but also before, with a few more restrictions), we relay any tx from our mempool (except for those that arrived after the last INV sent to the peer), so someone wanting to fingerprint our mempool could just ask for those transactions directly instead I think and then check whether we send it to them or answer with `NOTFOUND`.
I don't think this issue makes sense as an attack -- we aim to avoid giving away tight timing information
...
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3463894000)
I'd say it's time to update the PR description:
<details>
<summary>24% faster reindex-chainstate | 921129 blocks | dbcache 450 | rpi5-16-2 | aarch64 | Cortex-A76 | 4 cores | 15Gi RAM | ext4 | SSD</summary>
```
COMMITS="cb0fdfdf3704d5ffe6ccc634de6fdba6b7b57a85 2aa510348143521a14146e41b5cf87cb3e60b29e"; \
STOP=921129; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q ori
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3463894000)
I'd say it's time to update the PR description:
<details>
<summary>24% faster reindex-chainstate | 921129 blocks | dbcache 450 | rpi5-16-2 | aarch64 | Cortex-A76 | 4 cores | 15Gi RAM | ext4 | SSD</summary>
```
COMMITS="cb0fdfdf3704d5ffe6ccc634de6fdba6b7b57a85 2aa510348143521a14146e41b5cf87cb3e60b29e"; \
STOP=921129; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q ori
...
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2475412779)
I agree that mentioning a consensus error makes sense, so I added it to the warning message. Note that I didn't change the existing `CheckForkWarningConditions()` message as well, but could do that too if reviewers suggest it.
In principle, I don't see a difference whether this scenario happens during IBD or close to the tip. In both cases, it is most likely corruption but a consensus error is possible too. Current behaviour is that in the IBD case, we will cycle through header-syncs with n
...
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2475412779)
I agree that mentioning a consensus error makes sense, so I added it to the warning message. Note that I didn't change the existing `CheckForkWarningConditions()` message as well, but could do that too if reviewers suggest it.
In principle, I don't see a difference whether this scenario happens during IBD or close to the tip. In both cases, it is most likely corruption but a consensus error is possible too. Current behaviour is that in the IBD case, we will cycle through header-syncs with n
...
💬 TheCharlatan commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2475436253)
In commit fa128d1488c08d9816f425c73d01db4c1597ee68:
Would the return type of `GetTotalSize` fit into the criteria for this refactor?
```diff
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index cf6da55aa9..9d50dfe949 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -195 +195 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
- unsigned int tx_missing_size = 0;
+ uint32_t tx_missing_size = 0;
diff --git a/src/ne
...
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2475436253)
In commit fa128d1488c08d9816f425c73d01db4c1597ee68:
Would the return type of `GetTotalSize` fit into the criteria for this refactor?
```diff
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index cf6da55aa9..9d50dfe949 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -195 +195 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
- unsigned int tx_missing_size = 0;
+ uint32_t tx_missing_size = 0;
diff --git a/src/ne
...
💬 l0rinc commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3463979308)
I have also documented some workarounds for mac in https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681 - they kinda' work, without sanitizers at least, which cannot handle exceptions for some reason.
But I'm not sure why we want to give up fuzzing on Macs, everything we do is a whack-a-mole game, if we want people to write fuzz tests, we can't expect them to run them on a different machine and not even try them locally. We'll just end up with 100 PR pushes because people will
...
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3463979308)
I have also documented some workarounds for mac in https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681 - they kinda' work, without sanitizers at least, which cannot handle exceptions for some reason.
But I'm not sure why we want to give up fuzzing on Macs, everything we do is a whack-a-mole game, if we want people to write fuzz tests, we can't expect them to run them on a different machine and not even try them locally. We'll just end up with 100 PR pushes because people will
...
💬 ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3463997000)
I'm not sure this makes sense, per https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765 . Perhaps it would be better to:
* treat a CMPCTBLOCK announcement as being fine at any time (eg, so that miners who get a block via RPC can just send CMPCTBLOCK messages directly), and the "high-bandwidth" as just a way for nodes to avoid wasting their bandwidth sending those messages unnecessarily
* change our behaviour when sending GETBLOCKTXN to also request any transactions that
...
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3463997000)
I'm not sure this makes sense, per https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765 . Perhaps it would be better to:
* treat a CMPCTBLOCK announcement as being fine at any time (eg, so that miners who get a block via RPC can just send CMPCTBLOCK messages directly), and the "high-bandwidth" as just a way for nodes to avoid wasting their bandwidth sending those messages unnecessarily
* change our behaviour when sending GETBLOCKTXN to also request any transactions that
...
💬 l0rinc commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3464012836)
> -L/opt/homebrew/opt/llvm@14/lib
this is too old, you need at least version 16. Do a general `brew upgrade` on the system and retry, I have the same OSx, it should work.
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3464012836)
> -L/opt/homebrew/opt/llvm@14/lib
this is too old, you need at least version 16. Do a general `brew upgrade` on the system and retry, I have the same OSx, it should work.
✅ mzumsande closed a pull request: "validation: Make ReplayBlocks interruptible"
(https://github.com/bitcoin/bitcoin/pull/30155)
(https://github.com/bitcoin/bitcoin/pull/30155)
💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#issuecomment-3464018442)
Closing as "up for grabs" - https://github.com/bitcoin/bitcoin/issues/11600 is already closed, and because we now flush the chainstate every hour replays in general shouldn't take as much time anymore, so there is less need to interrupt/continue them.
That said, it could still be a nice-to-have if someone wants to pick it up (note https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1633721111 in this case, maybe there is a more elegant way I didn't see).
(https://github.com/bitcoin/bitcoin/pull/30155#issuecomment-3464018442)
Closing as "up for grabs" - https://github.com/bitcoin/bitcoin/issues/11600 is already closed, and because we now flush the chainstate every hour replays in general shouldn't take as much time anymore, so there is less need to interrupt/continue them.
That said, it could still be a nice-to-have if someone wants to pick it up (note https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1633721111 in this case, maybe there is a more elegant way I didn't see).
👍 TheCharlatan approved a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3396350580)
ACK f5eca8d5252a68f0fbc8b5efec021c75744555b6
I have fuzzed the thread pool for some time again, and did not find any new issues. Also the previously observed memory leak during fuzzing was not observed anymore. The changes to the http workers look sane to me.
Can you run the commits through `clang-format-diff.py`? There are a bunch of formatting issues in the first two commits.
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3396350580)
ACK f5eca8d5252a68f0fbc8b5efec021c75744555b6
I have fuzzed the thread pool for some time again, and did not find any new issues. Also the previously observed memory leak during fuzzing was not observed anymore. The changes to the http workers look sane to me.
Can you run the commits through `clang-format-diff.py`? There are a bunch of formatting issues in the first two commits.
💬 trevarj commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2475562342)
> Would it work with the recursion in make-mingw-w64/implementation?
Forgot about that, so no🤦🏻♂️. Will be much better when that PR lands.
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2475562342)
> Would it work with the recursion in make-mingw-w64/implementation?
Forgot about that, so no🤦🏻♂️. Will be much better when that PR lands.
💬 ajtowns commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2475563895)
Perhaps document how we prevent concurrent access to this while you're at it? (We can't use `GUARDED_BY(m_nodes_mutex)` since that results in lock order issues with cs_main)
AIUI, accesses are:
* `DisconnectNodes()` via `ThreadSocketHandler()` which adds nodes and removes them in normal operation
* `StopNodes()` via `Stop()` which runs it after `StopThreads()`, which ensures `threadSocketHandler` is joined before returning. `Stop()` is invoked in `~CConnman()` and `init.cpp:Shutdown()`
...
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2475563895)
Perhaps document how we prevent concurrent access to this while you're at it? (We can't use `GUARDED_BY(m_nodes_mutex)` since that results in lock order issues with cs_main)
AIUI, accesses are:
* `DisconnectNodes()` via `ThreadSocketHandler()` which adds nodes and removes them in normal operation
* `StopNodes()` via `Stop()` which runs it after `StopThreads()`, which ensures `threadSocketHandler` is joined before returning. `Stop()` is invoked in `~CConnman()` and `init.cpp:Shutdown()`
...
💬 TheCharlatan commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2475582408)
Ah, I see. I think I would change the order then, since typically named optional args precede positional arguments / operands in command line applications: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_01 .
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2475582408)
Ah, I see. I think I would change the order then, since typically named optional args precede positional arguments / operands in command line applications: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_01 .
🤔 fjahr reviewed a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-3378943773)
Concept ACK
I agree with @ajtowns that coupling this with nolisten makes sense and also means that this should not be too dangerous. Rather than turning that on by default and potentially risking the user don't fully understand the implications of this, the option could also error if nolisten isn't set.
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-3378943773)
Concept ACK
I agree with @ajtowns that coupling this with nolisten makes sense and also means that this should not be too dangerous. Rather than turning that on by default and potentially risking the user don't fully understand the implications of this, the option could also error if nolisten isn't set.
💬 fjahr commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2462055701)
nit: m_ prefix?
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2462055701)
nit: m_ prefix?
💬 fjahr commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2462049949)
Could the naming be kept consistent between the function names and the init arg? One is "disable v1" and the other is "only v2" and I don't see a good reason for that.
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2462049949)
Could the naming be kept consistent between the function names and the init arg? One is "disable v1" and the other is "only v2" and I don't see a good reason for that.
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475596211)
Thanks for catching this, I'll remove the line, but like I said above, ideally this test wouldn't be stateful at all.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475596211)
Thanks for catching this, I'll remove the line, but like I said above, ideally this test wouldn't be stateful at all.