💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408023767)
> If we `wait()` or `get()` on the returned future, does that imply a release/acquire memory ordering or a relaxed memory ordering? I can't seem to find out what this means for other non-atomic memory that was written on the worker thread before the task is completed.
For the returned value, `wait()`/`get()` provide release/acquire semantics. All updates performed by the task should be visible after `get()` returns.
Now, if the task modifies other objects that might be accessed concurrently
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408023767)
> If we `wait()` or `get()` on the returned future, does that imply a release/acquire memory ordering or a relaxed memory ordering? I can't seem to find out what this means for other non-atomic memory that was written on the worker thread before the task is completed.
For the returned value, `wait()`/`get()` provide release/acquire semantics. All updates performed by the task should be visible after `get()` returns.
Now, if the task modifies other objects that might be accessed concurrently
...
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408056506)
Sure, so I tried updating #31132 to use the thread pool here. It makes the change simpler if we were to already have this thread pool merged. However, the threads write to non-synchronized shared vectors, which the main thread will read. We need to make sure the write happens before the read. I initially stored the futures and wait on them like this https://github.com/andrewtoth/bitcoin/commit/c256f1b457cbd5b900aa34703eb5853d2449bcde#diff-3eff97d24109f473292b38a6495c5b51e8bda9f4bff9b691cf255968b
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408056506)
Sure, so I tried updating #31132 to use the thread pool here. It makes the change simpler if we were to already have this thread pool merged. However, the threads write to non-synchronized shared vectors, which the main thread will read. We need to make sure the write happens before the read. I initially stored the futures and wait on them like this https://github.com/andrewtoth/bitcoin/commit/c256f1b457cbd5b900aa34703eb5853d2449bcde#diff-3eff97d24109f473292b38a6495c5b51e8bda9f4bff9b691cf255968b
...
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408106215)
> All updates performed by the task should be visible after get() returns.
Hmm so in that case, then the diff I linked above is correct? The main thread is the one waiting on the future, and the non-synchronized vector is modified only by the worker thread. So the change to the vector should be visible on the main thread.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408106215)
> All updates performed by the task should be visible after get() returns.
Hmm so in that case, then the diff I linked above is correct? The main thread is the one waiting on the future, and the non-synchronized vector is modified only by the worker thread. So the change to the vector should be visible on the main thread.
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408379260)
> Sure, so I tried updating #31132 to use the thread pool here. It makes the change simpler if we were to already have this thread pool merged. However, the threads write to non-synchronized shared vectors, which the main thread will read. We need to make sure the write happens before the read. I initially stored the futures and wait on them like this [andrewtoth@c256f1b#diff-3eff97d24109f473292b38a6495c5b51e8bda9f4bff9b691cf255968ba80d85dR151-R164](https://github.com/andrewtoth/bitcoin/commit/c
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408379260)
> Sure, so I tried updating #31132 to use the thread pool here. It makes the change simpler if we were to already have this thread pool merged. However, the threads write to non-synchronized shared vectors, which the main thread will read. We need to make sure the write happens before the read. I initially stored the futures and wait on them like this [andrewtoth@c256f1b#diff-3eff97d24109f473292b38a6495c5b51e8bda9f4bff9b691cf255968ba80d85dR151-R164](https://github.com/andrewtoth/bitcoin/commit/c
...
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408391339)
Nice!
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408391339)
Nice!
🤔 hebasto reviewed a pull request: "Fix windows libc++ `fs::path` `fstream` compile errors"
(https://github.com/bitcoin/bitcoin/pull/33550#pullrequestreview-3307332538)
> ... even though there is not currently a CI job testing Windows libc++ builds.
Add to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.
(https://github.com/bitcoin/bitcoin/pull/33550#pullrequestreview-3307332538)
> ... even though there is not currently a CI job testing Windows libc++ builds.
Add to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.
👍 hodlinator approved a pull request: "Clear out space on GHA jobs"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3307376880)
ACK c01214e72b9876c59924c98e96f43906f5df3353
I'm okay with applying broadly to ease maintenance, with a very slight preference for changing it back to more surgically apply to CentOS.
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3307376880)
ACK c01214e72b9876c59924c98e96f43906f5df3353
I'm okay with applying broadly to ease maintenance, with a very slight preference for changing it back to more surgically apply to CentOS.
👍 hodlinator approved a pull request: "Modernize use of UTF-8 in Windows code"
(https://github.com/bitcoin/bitcoin/pull/32380#pullrequestreview-3307401839)
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
(https://github.com/bitcoin/bitcoin/pull/32380#pullrequestreview-3307401839)
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
📝 mzumsande opened a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553)
In case of corruption that leads to a block marked as invalid that is actually valid for the rest of the network, the user currently doesn't receive good error messages, but will get be stuck in an endless headers-sync loop with no explanation (#26931).
This PR improves warnings in two ways:
- When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see https://github.com/bitcoin/bitcoin/issues/26391#issu
...
(https://github.com/bitcoin/bitcoin/pull/33553)
In case of corruption that leads to a block marked as invalid that is actually valid for the rest of the network, the user currently doesn't receive good error messages, but will get be stuck in an endless headers-sync loop with no explanation (#26931).
This PR improves warnings in two ways:
- When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see https://github.com/bitcoin/bitcoin/issues/26391#issu
...
💬 mzumsande commented on issue "HeadersSync tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33547#issuecomment-3374358704)
I opened #33553 to address the problem from https://github.com/bitcoin/bitcoin/issues/26391 .
(https://github.com/bitcoin/bitcoin/issues/33547#issuecomment-3374358704)
I opened #33553 to address the problem from https://github.com/bitcoin/bitcoin/issues/26391 .
💬 mzumsande commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-3374369293)
> I'll work on a PR suggesting to use this check in the pre-sync header loop situation
Forgot about this completely, but opened #33553 now to improve the logging (we'll still search for peers with headers we can use, but will now repeatedly log warning suggesting to reindex if the problem persists)
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-3374369293)
> I'll work on a PR suggesting to use this check in the pre-sync header loop situation
Forgot about this completely, but opened #33553 now to improve the logging (we'll still search for peers with headers we can use, but will now repeatedly log warning suggesting to reindex if the problem persists)
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408782353)
Let me know how it goes :)
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2408782353)
Let me know how it goes :)
⚠️ plebhash opened an issue: "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage"
(https://github.com/bitcoin/bitcoin/issues/33554)
this is similar to #33463 cc @ryanofsky
however there's no usage of `waitTipChanged`, only `waitNext`
steps to reproduce:
- build `bitcoin-node` from `v30.0rc3` tag
- clone https://github.com/plebhash/sv2-bitcoin-core
- check out `2025-10-06-abort-proxy-io` branch
- launch `bitcoin-node` with `-ipc-bind=unix`
- launch `sv2-bitcoin-core` with `cargo run --example logger "/path/to/node.sock"`
here's a stack trace of `bitcoin-node`:
https://pastebin.com/uugSA6cD
(exceeds 65536 characters so g
...
(https://github.com/bitcoin/bitcoin/issues/33554)
this is similar to #33463 cc @ryanofsky
however there's no usage of `waitTipChanged`, only `waitNext`
steps to reproduce:
- build `bitcoin-node` from `v30.0rc3` tag
- clone https://github.com/plebhash/sv2-bitcoin-core
- check out `2025-10-06-abort-proxy-io` branch
- launch `bitcoin-node` with `-ipc-bind=unix`
- launch `sv2-bitcoin-core` with `cargo run --example logger "/path/to/node.sock"`
here's a stack trace of `bitcoin-node`:
https://pastebin.com/uugSA6cD
(exceeds 65536 characters so g
...
💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374530494)
also reproducible on `v30-0rc1` tag
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374530494)
also reproducible on `v30-0rc1` tag
💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374565734)
one relevant detail here is that our rust code has evolved from what it looked like when I first reported https://github.com/bitcoin/bitcoin/issues/33463, and there I did not witness this behavior
now, every 10s, we receive a Sv2 [`CoinbaseOutputConstraints`](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputconstraints-client---server) message, which is meant to set the coinbase parameters with regards to blockspace consumption (weight a
...
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374565734)
one relevant detail here is that our rust code has evolved from what it looked like when I first reported https://github.com/bitcoin/bitcoin/issues/33463, and there I did not witness this behavior
now, every 10s, we receive a Sv2 [`CoinbaseOutputConstraints`](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputconstraints-client---server) message, which is meant to set the coinbase parameters with regards to blockspace consumption (weight a
...
💬 ryanofsky commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374751529)
(So far I'm not able to reproduce a crash with v30.0rc3, but that might just be because I was testing with regtest.)
The assert you see happening on [line 289](https://github.com/bitcoin-core/libmultiprocess/blob/13424cf2ecc1e5eadc85556cf1f4c65e915f702a/include/mp/proxy-io.h#L289) just happens when the IPC client asks the server to execute multiple IPC calls simultaneously on the same thread. The IPC server does not expect this and currently aborts when it happens.
It is possible for the serve
...
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3374751529)
(So far I'm not able to reproduce a crash with v30.0rc3, but that might just be because I was testing with regtest.)
The assert you see happening on [line 289](https://github.com/bitcoin-core/libmultiprocess/blob/13424cf2ecc1e5eadc85556cf1f4c65e915f702a/include/mp/proxy-io.h#L289) just happens when the IPC client asks the server to execute multiple IPC calls simultaneously on the same thread. The IPC server does not expect this and currently aborts when it happens.
It is possible for the serve
...
🤔 l0rinc reviewed a pull request: "cluster mempool: control/optimize TxGraph memory usage"
(https://github.com/bitcoin/bitcoin/pull/33157#pullrequestreview-3299619754)
It's my first time diving into cluster mempool, and it seems like a solid implementation.
I like the polymorphic split between `SingletonClusterImpl` and `GenericClusterImpl`.
I've documented my review journey and left many comments. Some are tiny nits, but others appear to be leftovers from previous refactorings that should be corrected - I'm fine with doing them in a follow-up PR as well.
If you decide to apply any suggestions here, I've included a few commit reorganization tips that could ma
...
(https://github.com/bitcoin/bitcoin/pull/33157#pullrequestreview-3299619754)
It's my first time diving into cluster mempool, and it seems like a solid implementation.
I like the polymorphic split between `SingletonClusterImpl` and `GenericClusterImpl`.
I've documented my review journey and left many comments. Some are tiny nits, but others appear to be leftovers from previous refactorings that should be corrected - I'm fine with doing them in a follow-up PR as well.
If you decide to apply any suggestions here, I've included a few commit reorganization tips that could ma
...
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403014140)
nit: I noticed `m_locator` is a raw C-style array, was `std::array<Locator, MAX_LEVELS>` considered as a slightly safer alternative that may avoid pointer decay - given its tiny size we want to make sure we're not misindexing and it might make the iterations slightly more descriptive:
```C++
for (int level{0}; level < entry.m_locator.size(); ++level) {
```
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2403014140)
nit: I noticed `m_locator` is a raw C-style array, was `std::array<Locator, MAX_LEVELS>` considered as a slightly safer alternative that may avoid pointer decay - given its tiny size we want to make sure we're not misindexing and it might make the iterations slightly more descriptive:
```C++
for (int level{0}; level < entry.m_locator.size(); ++level) {
```
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2402767616)
I wonder if the new vtable pointer that was introduced by polymorphism is worth it or we could make a fake-inheritance by templates or macros or code generation instead or `std::variant<SingletonCluste, GenericCluster>` parameter types? It would likely be a big refactor and not sure it would be smaller or more readable, just thinking out loud.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2402767616)
I wonder if the new vtable pointer that was introduced by polymorphism is worth it or we could make a fake-inheritance by templates or macros or code generation instead or `std::variant<SingletonCluste, GenericCluster>` parameter types? It would likely be a big refactor and not sure it would be smaller or more readable, just thinking out loud.
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2402810313)
Given that we'd already be failing in the parent call's `Assume(level <= to_level)` (and that `std::unreachable()` is not yet available in C++20) could we maybe use our own `NONFATAL_UNREACHABLE` here?
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2402810313)
Given that we'd already be failing in the parent call's `Assume(level <= to_level)` (and that `std::unreachable()` is not yet available in C++20) could we maybe use our own `NONFATAL_UNREACHABLE` here?