💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414391241)
good point - I restricted the log to outbound peers, which makes sense anyway, because it's consistent with the fact that we only disconnect those.
(Besides, I'm curious what the current way to think about unconditional logging should be - be more liberal / care less about spamming? Or keep on being careful about log levels just as before when the rate limiting didn't exist? A bit offtopic here, but maybe something to discuss at CoreDev...)
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414391241)
good point - I restricted the log to outbound peers, which makes sense anyway, because it's consistent with the fact that we only disconnect those.
(Besides, I'm curious what the current way to think about unconditional logging should be - be more liberal / care less about spamming? Or keep on being careful about log levels just as before when the rate limiting didn't exist? A bit offtopic here, but maybe something to discuss at CoreDev...)
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414395854)
Thanks for pointing this out, `GetStrongRandBytes` does not introduce non-determinism. I'll change the next time I push up by making `rand_path` accept a `bool strong` so that the original behavior when `-testdatadir` is set is unchanged.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414395854)
Thanks for pointing this out, `GetStrongRandBytes` does not introduce non-determinism. I'll change the next time I push up by making `rand_path` accept a `bool strong` so that the original behavior when `-testdatadir` is set is unchanged.
💬 theuni commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2414400956)
Agreed about avoiding crashing.
I pushed a change that does something different than what you suggested though: assume an unknown log level should only be seen at our most verbose level, which is Trace.
I think that's ok, as it should cause a compilation warning (missed switch value), but otherwise not much change for the user. As it would be an API break that requires downstream attention, I think that's reasonable.
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2414400956)
Agreed about avoiding crashing.
I pushed a change that does something different than what you suggested though: assume an unknown log level should only be seen at our most verbose level, which is Trace.
I think that's ok, as it should cause a compilation warning (missed switch value), but otherwise not much change for the user. As it would be an API break that requires downstream attention, I think that's reasonable.
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414403520)
Right, m_interrupt should be initialized to true.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414403520)
Right, m_interrupt should be initialized to true.
⚠️ ryanofsky opened an issue: "RFC: Cancelling waitNext calls in the IPC mining interface"
(https://github.com/bitcoin/bitcoin/issues/33575)
It seems that the IPC mining interface needs to provide some way to cancel [`BlockTemplate::waitNext`](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/interfaces/mining.h#L63-L74) calls. This should not be hard to implement, but there are a few approaches that could be taken and it would be useful to know which would work best for callers. Context:
---
_Originally posted by @plebhash in [#33554](https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-
...
(https://github.com/bitcoin/bitcoin/issues/33575)
It seems that the IPC mining interface needs to provide some way to cancel [`BlockTemplate::waitNext`](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/interfaces/mining.h#L63-L74) calls. This should not be hard to implement, but there are a few approaches that could be taken and it would be useful to know which would work best for callers. Context:
---
_Originally posted by @plebhash in [#33554](https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-
...
⚠️ 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?