💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183527429)
Unrelated in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: Is there a reason you are sometimes using `gArgs` and then `m_args`? It looks like `m_args` ignores `extra_args` and `gArgs`/`m_node.args` doesn't?
So my preference would be to figure out whether to use `m_node.args` or `m_args`, and then use it consistently?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183527429)
Unrelated in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: Is there a reason you are sometimes using `gArgs` and then `m_args`? It looks like `m_args` ignores `extra_args` and `gArgs`/`m_node.args` doesn't?
So my preference would be to figure out whether to use `m_node.args` or `m_args`, and then use it consistently?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059)
unrelated in 512592cf41ae8fb307bd6eb651e3319e5053851e: Might be good to store a reference to the chain params in m_opts in a previous commit and use it inside the function. Then, while touching all lines here, the now unused params argument could be dropped.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059)
unrelated in 512592cf41ae8fb307bd6eb651e3319e5053851e: Might be good to store a reference to the chain params in m_opts in a previous commit and use it inside the function. Then, while touching all lines here, the now unused params argument could be dropped.
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183561359)
nit (same commit): You can bind blockman to a lambda named `CheckFilterLookups` that binds the arg to avoid touching those lines
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183561359)
nit (same commit): You can bind blockman to a lambda named `CheckFilterLookups` that binds the arg to avoid touching those lines
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183537074)
87999089e1b7794cd595ca19893a383149546087: Can you explain why it is necessary and safe to move this out of the for loop?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183537074)
87999089e1b7794cd595ca19893a383149546087: Can you explain why it is necessary and safe to move this out of the for loop?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183519088)
unrelated nit in a48fd5f3c17a0a6030d0ba47fa42be41f7e4aad9: I wonder what `memory` is being used for, but iwyu seems happy, so this is fine.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183519088)
unrelated nit in a48fd5f3c17a0a6030d0ba47fa42be41f7e4aad9: I wonder what `memory` is being used for, but iwyu seems happy, so this is fine.
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183542168)
nit in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: What is the point of adding 5 LOC that will be removed in the next commit anyway? If you want to keep that, it would be good to also remove the now unused include as well, which you forgot?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183542168)
nit in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: What is the point of adding 5 LOC that will be removed in the next commit anyway? If you want to keep that, it would be good to also remove the now unused include as well, which you forgot?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183565479)
f665024be7e5cc0a91ac7ff5779209313453a043: Wrong commit? How is this not missing from 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183565479)
f665024be7e5cc0a91ac7ff5779209313453a043: Wrong commit? How is this not missing from 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183550132)
3b9d6b06407f79d87b3318ecaaaca696b0dd1e68: Why?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183550132)
3b9d6b06407f79d87b3318ecaaaca696b0dd1e68: Why?
💬 MarcoFalke commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532881584)
My previous reply should be valid for that one as well
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532881584)
My previous reply should be valid for that one as well
💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183583641)
It wasn't possible to do that in https://github.com/bitcoin/bitcoin/commit/7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 because `BlockFileSeq` wasn't part of `BlockManager` then. One could rearrange the commits though and have 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 after the commit that moves stuff into BlockManager.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183583641)
It wasn't possible to do that in https://github.com/bitcoin/bitcoin/commit/7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 because `BlockFileSeq` wasn't part of `BlockManager` then. One could rearrange the commits though and have 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 after the commit that moves stuff into BlockManager.
💬 fanquake commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532892309)
Yea. I don't really have an intention of fixing these ones, just keeping tabs so that if they do get reported, we can point to a thread.
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532892309)
Yea. I don't really have an intention of fixing these ones, just keeping tabs so that if they do get reported, we can point to a thread.
🤔 glozow reviewed a pull request: "doc: clarify processing of mempool-msgs when NODE_BLOOM"
(https://github.com/bitcoin/bitcoin/pull/27559#pullrequestreview-1410732256)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
(https://github.com/bitcoin/bitcoin/pull/27559#pullrequestreview-1410732256)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
🚀 glozow merged a pull request: "doc: clarify processing of mempool-msgs when NODE_BLOOM"
(https://github.com/bitcoin/bitcoin/pull/27559)
(https://github.com/bitcoin/bitcoin/pull/27559)
💬 mzumsande commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532929566)
Post-merge ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532929566)
Post-merge ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183625464)
if found, `std::search` returns the Iterator to the beginning of the first occurrence of the sequence, so this should be `!= key.begin` or `== key.end()`.
Crafted a quick test to check it https://github.com/furszy/bitcoin-core/commit/f8a7c0670f95b317c46b01855c4c0f0db7ba7714 (no need to add it, I was just double checking it)
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183625464)
if found, `std::search` returns the Iterator to the beginning of the first occurrence of the sequence, so this should be `!= key.begin` or `== key.end()`.
Crafted a quick test to check it https://github.com/furszy/bitcoin-core/commit/f8a7c0670f95b317c46b01855c4c0f0db7ba7714 (no need to add it, I was just double checking it)
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183628544)
In the context of this commit it is necessary to move the assignment out of the for loop, so we don't destruct the `node.blockman` on each iteration and end up with a dangling reference to the original object in zmq. What I did not think about though is that we are also no longer resetting the internal state of the `BlockManager`when the `ChainstateManager` is destructed. So no, I'm no longer sure that this is safe.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183628544)
In the context of this commit it is necessary to move the assignment out of the for loop, so we don't destruct the `node.blockman` on each iteration and end up with a dangling reference to the original object in zmq. What I did not think about though is that we are also no longer resetting the internal state of the `BlockManager`when the `ChainstateManager` is destructed. So no, I'm no longer sure that this is safe.
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183633068)
ok, it is probably needed for zmq, but I still don't trivially see how it is safe. I wonder if the for loop can be removed, and replaced by the shutdown workflow from https://github.com/bitcoin/bitcoin/pull/26674#issuecomment-1345237799? See also 5921b863e39e5c3997895ffee1c87159e37a5d6f
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183633068)
ok, it is probably needed for zmq, but I still don't trivially see how it is safe. I wonder if the for loop can be removed, and replaced by the shutdown workflow from https://github.com/bitcoin/bitcoin/pull/26674#issuecomment-1345237799? See also 5921b863e39e5c3997895ffee1c87159e37a5d6f
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183633786)
See also https://github.com/bitcoin/bitcoin/issues/25055
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183633786)
See also https://github.com/bitcoin/bitcoin/issues/25055
💬 willcl-ark commented on issue "Coinstats index corrupted after invalidateblock and clean shutdown":
(https://github.com/bitcoin/bitcoin/issues/27558#issuecomment-1532965770)
Thanks for this report @vostrnad.
I have reproduced this issue sucessfully and am looking into the root cause.
<details>
<summary>Details</summary>
```log
2023-05-03T12:40:07Z InvalidChainFound: invalid block=0000002217824449bf6b7d8f9bc3cdb3407927712114a462376a991fea9f74af height=141000 log2_work=40.653563 date=2023-05-01T05:44:57Z
2023-05-03T12:40:07Z InvalidChainFound: current best=0000010052eca20e8afb2fd62cb0b07ef3cbbaffe43068c825f2e09f80b2ab35 height=140999 log2_work=40.653
...
(https://github.com/bitcoin/bitcoin/issues/27558#issuecomment-1532965770)
Thanks for this report @vostrnad.
I have reproduced this issue sucessfully and am looking into the root cause.
<details>
<summary>Details</summary>
```log
2023-05-03T12:40:07Z InvalidChainFound: invalid block=0000002217824449bf6b7d8f9bc3cdb3407927712114a462376a991fea9f74af height=141000 log2_work=40.653563 date=2023-05-01T05:44:57Z
2023-05-03T12:40:07Z InvalidChainFound: current best=0000010052eca20e8afb2fd62cb0b07ef3cbbaffe43068c825f2e09f80b2ab35 height=140999 log2_work=40.653
...
💬 fanquake commented on issue "Coinstats index corrupted after invalidateblock and clean shutdown":
(https://github.com/bitcoin/bitcoin/issues/27558#issuecomment-1532968619)
cc @mzumsande
(https://github.com/bitcoin/bitcoin/issues/27558#issuecomment-1532968619)
cc @mzumsande