💬 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
💬 jnewbery commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532970517)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532970517)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183644247)
What about the db `require_format` that is above? Is that used anywhere?
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183644247)
What about the db `require_format` that is above? Is that used anywhere?
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183652859)
Thank you for the context, I see that I am conceptually going backwards here.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183652859)
Thank you for the context, I see that I am conceptually going backwards here.
💬 fanquake commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1533030631)
> I thought this branch might be preferable...
If we are going to add a script like this to the repository, it should be incorporated into the release process (guix build). AM thinking about how best to integrate it into that process, if we want to inspect the just-compiled .o files, and if there is another way to perform this test.
cc @laanwj you might have some thoughts here?
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1533030631)
> I thought this branch might be preferable...
If we are going to add a script like this to the repository, it should be incorporated into the release process (guix build). AM thinking about how best to integrate it into that process, if we want to inspect the just-compiled .o files, and if there is another way to perform this test.
cc @laanwj you might have some thoughts here?
💬 furszy commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183690806)
Performance wise, create a new thread per lookup is quite bad. Should use a worker pool instead.
I have implemented a generic one for #26966 (https://github.com/bitcoin/bitcoin/pull/26966/commits/f448668f473675915fcebf0e1bd3d73dcb1881af) that could also use here. It also support callback futures so you can remove the costly active wait as well.
Give it a look and if you like it, could decouple it into a standalone PR so we could start using it in both PRs.
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183690806)
Performance wise, create a new thread per lookup is quite bad. Should use a worker pool instead.
I have implemented a generic one for #26966 (https://github.com/bitcoin/bitcoin/pull/26966/commits/f448668f473675915fcebf0e1bd3d73dcb1881af) that could also use here. It also support callback futures so you can remove the costly active wait as well.
Give it a look and if you like it, could decouple it into a standalone PR so we could start using it in both PRs.
📝 MarcoFalke opened a pull request: "ci: Use arm_container.dockerfile"
(https://github.com/bitcoin/bitcoin/pull/27562)
This allows to cache the image and thus speed up the CI task
(https://github.com/bitcoin/bitcoin/pull/27562)
This allows to cache the image and thus speed up the CI task
👍 Xekyo approved a pull request: "wallet: Refactor and document CoinControl"
(https://github.com/bitcoin/bitcoin/pull/26066#pullrequestreview-1410939913)
ACK daba95700b0b77a2e898299f218c47a69ed2c7d0
(https://github.com/bitcoin/bitcoin/pull/26066#pullrequestreview-1410939913)
ACK daba95700b0b77a2e898299f218c47a69ed2c7d0
💬 Xekyo commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183713583)
Nit: If it’s not too much of a bother, and if you have to touch the code again, I would consider it an improvement to change `UnSelect` to `Unselect`. The first time I scanned that passage, the capitalized “S” in the middle of the word made me think that it was actually just “Select”. Even more nitty, I’d use the adjective “unselected” to refer to things that were not picked in the first place, I would prefer “to deselect” to refer to removing things from a selection.
```suggestion
void CCo
...
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183713583)
Nit: If it’s not too much of a bother, and if you have to touch the code again, I would consider it an improvement to change `UnSelect` to `Unselect`. The first time I scanned that passage, the capitalized “S” in the middle of the word made me think that it was actually just “Select”. Even more nitty, I’d use the adjective “unselected” to refer to things that were not picked in the first place, I would prefer “to deselect” to refer to removing things from a selection.
```suggestion
void CCo
...
💬 Xekyo commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183714494)
Nit with the same reasoning as above:
```suggestion
void CCoinControl::DeselectAll()
```
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183714494)
Nit with the same reasoning as above:
```suggestion
void CCoinControl::DeselectAll()
```
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183738245)
> 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
That does sound more sane than continuously re-indexing forever, or waiting for the user to interrupt.
The change I pushed will be reverted. How about instead of moving the previously `static` functions of `blockstorage.cpp` into its `BlockManager` class, moving them to a stateless sub-class of the `BlockManager`? Then we can have the
...
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183738245)
> 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
That does sound more sane than continuously re-indexing forever, or waiting for the user to interrupt.
The change I pushed will be reverted. How about instead of moving the previously `static` functions of `blockstorage.cpp` into its `BlockManager` class, moving them to a stateless sub-class of the `BlockManager`? Then we can have the
...
💬 MarcoFalke commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183754343)
> They're necessary for https://github.com/bitcoin/bitcoin/commit/88b63a6e163045e63a7540427834a57799234407 "tests: Automatically run both wallet types in parallel for wallet tests".
I looked at that commit and it looks like the test list for the tests to run in parallel is created before passing it to `TestHandler`. `TestHandler` should never create conflicting/duplicate datadirs, because each test has its own directory based on the global test_runner directore, the name of the test, and its
...
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183754343)
> They're necessary for https://github.com/bitcoin/bitcoin/commit/88b63a6e163045e63a7540427834a57799234407 "tests: Automatically run both wallet types in parallel for wallet tests".
I looked at that commit and it looks like the test list for the tests to run in parallel is created before passing it to `TestHandler`. `TestHandler` should never create conflicting/duplicate datadirs, because each test has its own directory based on the global test_runner directore, the name of the test, and its
...
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183761759)
I very much like them to be stateful, see also https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059 . This is also needed for some other, unrelated, stuff that someone should be working on (cc @josibake )
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183761759)
I very much like them to be stateful, see also https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059 . This is also needed for some other, unrelated, stuff that someone should be working on (cc @josibake )
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183762778)
This cleanup applies to other functions as well, so I am happy to have this split up, to avoid this pull from growing too large?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183762778)
This cleanup applies to other functions as well, so I am happy to have this split up, to avoid this pull from growing too large?
💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1183773763)
In f76830e5:
This db cursor changes need to be in the first commit as it's not compiling currently.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1183773763)
In f76830e5:
This db cursor changes need to be in the first commit as it's not compiling currently.