💬 achow101 commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3198197406)
Thinking on this further, I think I agree that there isn't a good reason to be making this change. When building from source, we already don't enable several optional features, including the GUI, that we ship in the release binaries. Since the switch to cmake, having the dependencies installed doesn't enable those features automatically, they still need to be enabled explicitly during configuration. Developers should probably be using the dev-mode preset, or similar, which enables a bunch of the
...
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3198197406)
Thinking on this further, I think I agree that there isn't a good reason to be making this change. When building from source, we already don't enable several optional features, including the GUI, that we ship in the release binaries. Since the switch to cmake, having the dependencies installed doesn't enable those features automatically, they still need to be enabled explicitly during configuration. Developers should probably be using the dev-mode preset, or similar, which enables a bunch of the
...
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283314830)
Thanks, when I saw this I wasn't aware what a ["bundled"](https://github.com/capnproto/pycapnp/blob/a89eb0dee6c67049bf2f6b875e51dd3f71a6abd1/setup.py#L146-L151) capnproto was but agree it makes sense to use it if the python extension includes it.
I guess I'd be curious what would happen if the docs were changed to use `force-system-libcapnp=True` instead of `force-bundled-libcapnp=True`? It seems like since the c++ code already requires capnproto to be built, maybe it's wasteful for the instr
...
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283314830)
Thanks, when I saw this I wasn't aware what a ["bundled"](https://github.com/capnproto/pycapnp/blob/a89eb0dee6c67049bf2f6b875e51dd3f71a6abd1/setup.py#L146-L151) capnproto was but agree it makes sense to use it if the python extension includes it.
I guess I'd be curious what would happen if the docs were changed to use `force-system-libcapnp=True` instead of `force-bundled-libcapnp=True`? It seems like since the c++ code already requires capnproto to be built, maybe it's wasteful for the instr
...
🤔 achow101 reviewed a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3129705803)
I disagree with defaulting `ENABLE_IPC=ON` since that makes capnproto almost a required dependency. Instead of it being an optional feature that you can turn on, it now becomes and optional feature that you can turn off, and if you just do a build with the defaults, capnproto will need to be installed.
I think it's ok to have the default source build not match the release builds or the depends builds since we already do that by excluding optional features like the GUI, or ZMQ, etc.
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3129705803)
I disagree with defaulting `ENABLE_IPC=ON` since that makes capnproto almost a required dependency. Instead of it being an optional feature that you can turn on, it now becomes and optional feature that you can turn off, and if you just do a build with the defaults, capnproto will need to be installed.
I think it's ok to have the default source build not match the release builds or the depends builds since we already do that by excluding optional features like the GUI, or ZMQ, etc.
💬 achow101 commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2283319242)
In 16bce9ac4cd02b2c8308f370bf39d70f9421e69b "build: depends makes libmultiprocess by default"
For the other packages that depends may build, there is a similar if-else block right about this, with a comment that says something about needing to use `MATCHES` rather than `STREQUAL`. That seems like something that should be done here?
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2283319242)
In 16bce9ac4cd02b2c8308f370bf39d70f9421e69b "build: depends makes libmultiprocess by default"
For the other packages that depends may build, there is a similar if-else block right about this, with a comment that says something about needing to use `MATCHES` rather than `STREQUAL`. That seems like something that should be done here?
👋 Crypt-iQ's pull request is ready for review: "test: modify logging_filesize_rate_limit params"
(https://github.com/bitcoin/bitcoin/pull/33211)
(https://github.com/bitcoin/bitcoin/pull/33211)
💬 Crypt-iQ commented on pull request "test: modify logging_filesize_rate_limit params":
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3198335063)
cc @stickies-v
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3198335063)
cc @stickies-v
🤔 achow101 reviewed a pull request: "index: Fix coinstats overflow"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3129757943)
053ac55eb569be6b49c5249a7d8c0eeaa149a18b seems like it could cause incorrect total statistics if somehow we ever have the situation where we are appending a block that doesn't build on the index's current block hash.
This behavior does match what we do today with the running total members, but it feels
Although that sounds like it should really be a fatal error.
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3129757943)
053ac55eb569be6b49c5249a7d8c0eeaa149a18b seems like it could cause incorrect total statistics if somehow we ever have the situation where we are appending a block that doesn't build on the index's current block hash.
This behavior does match what we do today with the running total members, but it feels
Although that sounds like it should really be a fatal error.
💬 achow101 commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2283353869)
In b8c5bf71a4be4756740fb7b8a6a580153b007475 "index, refactor: DRY coinbase check"
This seems like a bug fix that should be in it's own commit.
Should this be decrementing `m_total_unspendable_amount` and `m_total_unspendables_bip30` to reverse what the similar code in `CustomAppend` does? This doesn't really matter once those members are removed.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2283353869)
In b8c5bf71a4be4756740fb7b8a6a580153b007475 "index, refactor: DRY coinbase check"
This seems like a bug fix that should be in it's own commit.
Should this be decrementing `m_total_unspendable_amount` and `m_total_unspendables_bip30` to reverse what the similar code in `CustomAppend` does? This doesn't really matter once those members are removed.
💬 Crypt-iQ commented on issue "ci: failure in `logging_tests`":
(https://github.com/bitcoin/bitcoin/issues/33195#issuecomment-3198343891)
I've opened https://github.com/bitcoin/bitcoin/pull/33211 to fix this. It changes the time window to 1h which should be more than enough. Also, it reduces `num_lines` quite a bit as suggested by @stickies-v above.
> It seems the 835th iteration got corrupted (scroll down a bit)
Interesting... now I'm curious why it was corrupted? But I'll let that be for now.
(https://github.com/bitcoin/bitcoin/issues/33195#issuecomment-3198343891)
I've opened https://github.com/bitcoin/bitcoin/pull/33211 to fix this. It changes the time window to 1h which should be more than enough. Also, it reduces `num_lines` quite a bit as suggested by @stickies-v above.
> It seems the 835th iteration got corrupted (scroll down a bit)
Interesting... now I'm curious why it was corrupted? But I'll let that be for now.
💬 achow101 commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2277678459)
In 232485d2486f08221203224212d5451a47eb4fc8 "refactor(test): Break up headers_sync_chainwork_tests"
Our preferred pattern for test cases within unit tests is to define them with `BOOST_AUTO_TEST_CASE` rather than functions like these.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2277678459)
In 232485d2486f08221203224212d5451a47eb4fc8 "refactor(test): Break up headers_sync_chainwork_tests"
Our preferred pattern for test cases within unit tests is to define them with `BOOST_AUTO_TEST_CASE` rather than functions like these.
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3198354810)
> I disagree with defaulting `ENABLE_IPC=ON` since that makes capnproto almost a required dependency. Instead of it being an optional feature that you can turn on, it now becomes and optional feature that you can turn off, and if you just do a build with the defaults, capnproto will need to be installed.
Yes, with the current version of this PR, ENABLE_IPC acts like ENABLE_WALLET: it's an optional feature that is default-on in both depends and non-depends builds.
I didn't realize that othe
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3198354810)
> I disagree with defaulting `ENABLE_IPC=ON` since that makes capnproto almost a required dependency. Instead of it being an optional feature that you can turn on, it now becomes and optional feature that you can turn off, and if you just do a build with the defaults, capnproto will need to be installed.
Yes, with the current version of this PR, ENABLE_IPC acts like ENABLE_WALLET: it's an optional feature that is default-on in both depends and non-depends builds.
I didn't realize that othe
...
📝 mzumsande opened a pull request: "index: Don't commit state in BaseIndex::Rewind "
(https://github.com/bitcoin/bitcoin/pull/33212)
The committed state of an index should never be ahead of the flushed chainstate.
Otherwise, in the case of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next ChainStateFlushed notification.
Fixes #33208
(https://github.com/bitcoin/bitcoin/pull/33212)
The committed state of an index should never be ahead of the flushed chainstate.
Otherwise, in the case of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next ChainStateFlushed notification.
Fixes #33208
💬 mzumsande commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3198387530)
See #33212 for a fix and a test.
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3198387530)
See #33212 for a fix and a test.
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2283445412)
re: https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2283319242
I think this looks ok, although it would be nice to make the style consistent. The MATCHES checks are treating values like "@zmq_packages@" "@wallet_packages@" as false if they are empty strings or if they are "strings containing only spaces" since apparently such values are treated as false by make & depends.
The check here is doing the same thing but it's just implemented in a different way. If `@ipc_packages@` cont
...
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2283445412)
re: https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2283319242
I think this looks ok, although it would be nice to make the style consistent. The MATCHES checks are treating values like "@zmq_packages@" "@wallet_packages@" as false if they are empty strings or if they are "strings containing only spaces" since apparently such values are treated as false by make & depends.
The check here is doing the same thing but it's just implemented in a different way. If `@ipc_packages@` cont
...
💬 achow101 commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3198425748)
ACK 4c531782569b42fc47478a468f4a79e0e2d93946
Verified against iproute2 and strace
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3198425748)
ACK 4c531782569b42fc47478a468f4a79e0e2d93946
Verified against iproute2 and strace
💬 achow101 commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3198474435)
> That's the sha256 hash digest of the MuHash object, not the full `MuHash3072`. I think [@mzumsande](https://github.com/mzumsande) is correct that we can't roll `m_muhash` back if the blocks are not available.
Since the coinstatsindex is changing with #30469, maybe we should also add to that to store the full muhash for every block? This would be the best time to make such a breaking change.
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3198474435)
> That's the sha256 hash digest of the MuHash object, not the full `MuHash3072`. I think [@mzumsande](https://github.com/mzumsande) is correct that we can't roll `m_muhash` back if the blocks are not available.
Since the coinstatsindex is changing with #30469, maybe we should also add to that to store the full muhash for every block? This would be the best time to make such a breaking change.
💬 gmaxwell commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3198534472)
The timelocks could be relaxed analogous to increasing the size however. WithMTP we essentially know the lock criteria for one block out.
I don't really think the memory usage is a concern, -- because there is just not a need to constantly do this with all peers in the face of memory pressure.
You could add an additional message that lets peers say "I have X fees in Y weight in my template" -- if the max size they were targeting was standardized then this would be a pretty good indicator w
...
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3198534472)
The timelocks could be relaxed analogous to increasing the size however. WithMTP we essentially know the lock criteria for one block out.
I don't really think the memory usage is a concern, -- because there is just not a need to constantly do this with all peers in the face of memory pressure.
You could add an additional message that lets peers say "I have X fees in Y weight in my template" -- if the max size they were targeting was standardized then this would be a pretty good indicator w
...
💬 achow101 commented on pull request "test: modify logging_filesize_rate_limit params":
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3198536278)
If we believe it is due to the reset window, then we could set set the reset window to 5 hours, which is the longest CI timeout, so if that were to ever be reached, CI would be failing anyways. But 1 hour is also fine.
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3198536278)
If we believe it is due to the reset window, then we could set set the reset window to 5 hours, which is the longest CI timeout, so if that were to ever be reached, CI would be failing anyways. But 1 hour is also fine.
💬 fjahr commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3198583247)
> I think @mzumsande is correct that we can't roll m_muhash back if the blocks are not available.
Actually there is a way to recover `m_muhash` but it's way too complicated to be considered as a fix IMO. Maybe in the future when we have made progress on a number of fronts. But here it goes: we could roll back the chain to the index fork point, calculate the muhash for the utxo set from scratch, compare the finalized out to the sha256 digest and then move on from there. Also, it would be possibl
...
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3198583247)
> I think @mzumsande is correct that we can't roll m_muhash back if the blocks are not available.
Actually there is a way to recover `m_muhash` but it's way too complicated to be considered as a fix IMO. Maybe in the future when we have made progress on a number of fronts. But here it goes: we could roll back the chain to the index fork point, calculate the muhash for the utxo set from scratch, compare the finalized out to the sha256 digest and then move on from there. Also, it would be possibl
...
🤔 achow101 reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3130148017)
ACK d395abac85a2acf0717859f05ccfccf8a3c71a46
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3130148017)
ACK d395abac85a2acf0717859f05ccfccf8a3c71a46