⚠️ fanquake opened an issue: "29.x: using a local libmultiprocess install will no-longer work"
(https://github.com/bitcoin/bitcoin/issues/33576)
Given that the `29.x` branch doesn't have the changes to accomodate upstream changes in libmultiprocess, anyone who builds and installs libmultiprocess, and then tries to build Core with `-DWITH_MULTIPROCESS=ON`, will get a compile failure:
```bash
[ 34%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc.dir/capnp/protocol.cpp.o
/bitcoin/src/ipc/capnp/protocol.cpp: In destructor 'virtual ipc::capnp::{anonymous}::CapnpProtocol::~CapnpProtocol()':
/bitcoin/src/ipc/capnp/protocol.cpp:45:62: error:
...
(https://github.com/bitcoin/bitcoin/issues/33576)
Given that the `29.x` branch doesn't have the changes to accomodate upstream changes in libmultiprocess, anyone who builds and installs libmultiprocess, and then tries to build Core with `-DWITH_MULTIPROCESS=ON`, will get a compile failure:
```bash
[ 34%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc.dir/capnp/protocol.cpp.o
/bitcoin/src/ipc/capnp/protocol.cpp: In destructor 'virtual ipc::capnp::{anonymous}::CapnpProtocol::~CapnpProtocol()':
/bitcoin/src/ipc/capnp/protocol.cpp:45:62: error:
...
💬 fanquake commented on issue "29.x: using a local libmultiprocess install will no-longer work":
(https://github.com/bitcoin/bitcoin/issues/33576#issuecomment-3382318483)
cc @Sjors @ryanofsky
(https://github.com/bitcoin/bitcoin/issues/33576#issuecomment-3382318483)
cc @Sjors @ryanofsky
💬 l0rinc commented on pull request "doc: bump the template macOS version since 14 is now the minimum supported version":
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382325065)
ACK 1dcff2e335444d5518961cdf798d9d24c6e3e1d7
Originally reported in https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3294934753
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382325065)
ACK 1dcff2e335444d5518961cdf798d9d24c6e3e1d7
Originally reported in https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3294934753
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414422774)
> Actually, we must use worker count check. Since we can Start with 0 worker threads but still set interrupt to false, so this would deadlock with only the interrupt check.
> Maybe guard against starting with 0 threads and then pick either?
I'm tempted to leave it as is, mainly because we might want to add `Interrupt()` and `Resume()` methods in the future that don't kill the worker threads. We might want to clear the queue due to an early failure in one of our tasks without having to recrea
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414422774)
> Actually, we must use worker count check. Since we can Start with 0 worker threads but still set interrupt to false, so this would deadlock with only the interrupt check.
> Maybe guard against starting with 0 threads and then pick either?
I'm tempted to leave it as is, mainly because we might want to add `Interrupt()` and `Resume()` methods in the future that don't kill the worker threads. We might want to clear the queue due to an early failure in one of our tasks without having to recrea
...
💬 ryanofsky commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3382354366)
These are good questions. I opened #33575 to figure out how to cancel waitNext calls, and I think it should not be hard to provide that.
More broadly, I think I'd try just having a c++ "waiter" thread dedicated to running `waitNext` calls, and a c++ "worker" thread to run all other IPC operations besides `waitNext`. The "waiter" thread should be mostly blocked monitoring mempool and network, and the "worker" thread should be available to do anything else you might need like fetching and submitt
...
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3382354366)
These are good questions. I opened #33575 to figure out how to cancel waitNext calls, and I think it should not be hard to provide that.
More broadly, I think I'd try just having a c++ "waiter" thread dedicated to running `waitNext` calls, and a c++ "worker" thread to run all other IPC operations besides `waitNext`. The "waiter" thread should be mostly blocked monitoring mempool and network, and the "worker" thread should be available to do anything else you might need like fetching and submitt
...
💬 plebhash commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3382367189)
number 1 would be a bit prohibitive on the Sv2 implementations we're working on
we need to stop `waitNext` every time the coinbase output constratints change, but that doesn't mean that a solution associatated with past constraints couldn't still arrive
so we still need ot keep a reference to every "active" `BlockTemplate` in memory, at least until a chain tip update
if we're forced to call `destroy` just to stop `waitNext`, that would lose context and forbid us from submitting this potential
...
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3382367189)
number 1 would be a bit prohibitive on the Sv2 implementations we're working on
we need to stop `waitNext` every time the coinbase output constratints change, but that doesn't mean that a solution associatated with past constraints couldn't still arrive
so we still need ot keep a reference to every "active" `BlockTemplate` in memory, at least until a chain tip update
if we're forced to call `destroy` just to stop `waitNext`, that would lose context and forbid us from submitting this potential
...
💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3382421722)
rephrasing my previous question in a more compact way:
is there a way to "kill" server threads?
---
> because presumably block_reserved_weight and max_additional_sigops option values should not change very frequently
indeed, I think this will be relatively rare in practice... but it's still something that's not under our control, and on the current strategy it will lead to unbounded resource consumption on Bitcoin Core side
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3382421722)
rephrasing my previous question in a more compact way:
is there a way to "kill" server threads?
---
> because presumably block_reserved_weight and max_additional_sigops option values should not change very frequently
indeed, I think this will be relatively rare in practice... but it's still something that's not under our control, and on the current strategy it will lead to unbounded resource consumption on Bitcoin Core side
📝 hebasto opened a pull request: "Modernize custom filtering"
(https://github.com/bitcoin-core/gui/pull/899)
In [`QSortFilterProxyModel::invalidateFilter()`](https://doc.qt.io/qt-6/qsortfilterproxymodel.html#invalidateFilter) is scheduled for deprecation in Qt 6.13. and emits warnings in Qt 6.10
[`QSortFilterProxyModel::beginFilterChange()`](https://doc.qt.io/qt-6/qsortfilterproxymodel.html#beginFilterChange) was introduced in Qt 6.9.
[`QSortFilterProxyModel::endFilterChange()`](https://doc.qt.io/qt-6/qsortfilterproxymodel.html#endFilterChange) was introduced in Qt 6.10.
Fixes https://github.c
...
(https://github.com/bitcoin-core/gui/pull/899)
In [`QSortFilterProxyModel::invalidateFilter()`](https://doc.qt.io/qt-6/qsortfilterproxymodel.html#invalidateFilter) is scheduled for deprecation in Qt 6.13. and emits warnings in Qt 6.10
[`QSortFilterProxyModel::beginFilterChange()`](https://doc.qt.io/qt-6/qsortfilterproxymodel.html#beginFilterChange) was introduced in Qt 6.9.
[`QSortFilterProxyModel::endFilterChange()`](https://doc.qt.io/qt-6/qsortfilterproxymodel.html#endFilterChange) was introduced in Qt 6.10.
Fixes https://github.c
...
💬 hebasto commented on issue "build: Qt deprecated-declarations warnings":
(https://github.com/bitcoin/bitcoin/issues/33571#issuecomment-3382431711)
Fixed in https://github.com/bitcoin-core/gui/pull/899.
(https://github.com/bitcoin/bitcoin/issues/33571#issuecomment-3382431711)
Fixed in https://github.com/bitcoin-core/gui/pull/899.
💬 Sjors commented on issue "29.x: using a local libmultiprocess install will no-longer work":
(https://github.com/bitcoin/bitcoin/issues/33576#issuecomment-3382446543)
Maybe we should tag the last `libmultiprocess` commit that is expected to work with 29.x and recommend that in either the libmultiprocess README or backported documentation here?
(https://github.com/bitcoin/bitcoin/issues/33576#issuecomment-3382446543)
Maybe we should tag the last `libmultiprocess` commit that is expected to work with 29.x and recommend that in either the libmultiprocess README or backported documentation here?
💬 ryanofsky commented on issue "29.x: using a local libmultiprocess install will no-longer work":
(https://github.com/bitcoin/bitcoin/issues/33576#issuecomment-3382446988)
> Unless we backport a number of ipc related changes to this branch (unlikely), this will be broken for the rest of this branches lief. This seems like something we might want to document.
This does seem like something good to document. Maybe the 29.x [dependencies.md](https://github.com/bitcoin/bitcoin/blob/29.x/doc/dependencies.md) file could document the version of multiprocess known to work with v29, consistent with the version used in depends https://github.com/bitcoin/bitcoin/blob/8bcb90d
...
(https://github.com/bitcoin/bitcoin/issues/33576#issuecomment-3382446988)
> Unless we backport a number of ipc related changes to this branch (unlikely), this will be broken for the rest of this branches lief. This seems like something we might want to document.
This does seem like something good to document. Maybe the 29.x [dependencies.md](https://github.com/bitcoin/bitcoin/blob/29.x/doc/dependencies.md) file could document the version of multiprocess known to work with v29, consistent with the version used in depends https://github.com/bitcoin/bitcoin/blob/8bcb90d
...
🤔 marcofleon reviewed a pull request: "fuzz: compact block harness"
(https://github.com/bitcoin/bitcoin/pull/33300#pullrequestreview-3315685387)
Some possible discussion about ed813c48f826d083becf93c741b483774c850c86 for review club.
(https://github.com/bitcoin/bitcoin/pull/33300#pullrequestreview-3315685387)
Some possible discussion about ed813c48f826d083becf93c741b483774c850c86 for review club.
💬 marcofleon commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414451232)
As we discussed a bit offline, increasing the number of peers here to greater than 3 should be enough to hit some of the missing "eviction" logic in `MaybeSetPeerAsAnnouncingHeaderAndIDs`. Maybe increasing to 8 peers would be fine? Although not sure if that would test anything more (somewhere else) than if it were 4.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414451232)
As we discussed a bit offline, increasing the number of peers here to greater than 3 should be enough to hit some of the missing "eviction" logic in `MaybeSetPeerAsAnnouncingHeaderAndIDs`. Maybe increasing to 8 peers would be fine? Although not sure if that would test anything more (somewhere else) than if it were 4.
💬 marcofleon commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414480315)
I feel like I might be overlooking something here, but could we keep `m_dirty_blockindex` the same and then if `EnableFuzzDeterminism()` just sort `vBlocks` by hash before calling `WriteBatchSync()`?
I guess I'm asking is there a reason why `m_dirty_blockindex` needs to be sorted by hash from the beginning vs right before the write.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414480315)
I feel like I might be overlooking something here, but could we keep `m_dirty_blockindex` the same and then if `EnableFuzzDeterminism()` just sort `vBlocks` by hash before calling `WriteBatchSync()`?
I guess I'm asking is there a reason why `m_dirty_blockindex` needs to be sorted by hash from the beginning vs right before the write.
💬 ryanofsky commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3382453186)
> if we're forced to call `destroy` just to stop `waitNext`, that would lose context and forbid us from submitting this potentially valid solution
Makes sense. So it sounds like you would want a `BlockTemplate::interruptWait()` method?
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3382453186)
> if we're forced to call `destroy` just to stop `waitNext`, that would lose context and forbid us from submitting this potentially valid solution
Makes sense. So it sounds like you would want a `BlockTemplate::interruptWait()` method?
💬 ryanofsky commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3382493322)
> is there a way to "kill" server threads?
Indirectly, yes. The server threads will destroy themselves as soon as they are no longer referenced, either by the client or an ongoing IPC call that is using them. So as long as the client is not holding references to threads it is not using, they will be cleaned up.
There is some cost to creating threads, so it's good to reuse them between calls, and why I recommended having dedicated "waiter" and "worker" threads. But if you wanted to create more
...
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3382493322)
> is there a way to "kill" server threads?
Indirectly, yes. The server threads will destroy themselves as soon as they are no longer referenced, either by the client or an ongoing IPC call that is using them. So as long as the client is not holding references to threads it is not using, they will be cleaned up.
There is some cost to creating threads, so it's good to reuse them between calls, and why I recommended having dedicated "waiter" and "worker" threads. But if you wanted to create more
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3382525762)
Thank you for the review @ismaelsadeeq!
Updated 2f6d5637d8cd1dc1a1444e31fdfb2dfb13152500 -> 4fce466d1a3e345836434f8fb375980f626981f1 ([kernelApi_70](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_70) -> [kernelApi_71](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_71), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_70..kernelApi_71))
* Addressed @ismaelsadeeq's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406532719), added some r
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3382525762)
Thank you for the review @ismaelsadeeq!
Updated 2f6d5637d8cd1dc1a1444e31fdfb2dfb13152500 -> 4fce466d1a3e345836434f8fb375980f626981f1 ([kernelApi_70](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_70) -> [kernelApi_71](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_71), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_70..kernelApi_71))
* Addressed @ismaelsadeeq's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406532719), added some r
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414186905)
Just trying to cast to our internal type early on. Might be more confusing though, so just removed it.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414186905)
Just trying to cast to our internal type early on. Might be more confusing though, so just removed it.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414538874)
I could change these, but I think just copying here is preferable, since these are just wrapped pointer views. Ideally they don't depend on the instantiation of the view in the iterator, but can be be used out of the scope of it.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414538874)
I could change these, but I think just copying here is preferable, since these are just wrapped pointer views. Ideally they don't depend on the instantiation of the view in the iterator, but can be be used out of the scope of it.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414217181)
I think it would be good to be consistent with these range checks here. This would also be asserted in the checks for the transaction when we call `Init` on the precomputed data.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2414217181)
I think it would be good to be consistent with these range checks here. This would also be asserted in the checks for the transaction when we call `Init` on the precomputed data.