💬 hebasto commented on issue "intermittent issue in feature_reindex.py: Reindex fails to continue after restart; `initload` thread hangs forever":
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2223971606)
> I think a simple fix would be to change the order in `Shutdown()`, stopping import block before the scheduler:
>
> ```diff
> diff --git a/src/init.cpp b/src/init.cpp
> index e4b65fbfa9..e0aae3699f 100644
> --- a/src/init.cpp
> +++ b/src/init.cpp
> @@ -298,8 +298,8 @@ void Shutdown(NodeContext& node)
>
> // After everything has been shut down, but before things get flushed, stop the
> // scheduler and load block thread.
> - if (node.scheduler) node.scheduler->stop();
...
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2223971606)
> I think a simple fix would be to change the order in `Shutdown()`, stopping import block before the scheduler:
>
> ```diff
> diff --git a/src/init.cpp b/src/init.cpp
> index e4b65fbfa9..e0aae3699f 100644
> --- a/src/init.cpp
> +++ b/src/init.cpp
> @@ -298,8 +298,8 @@ void Shutdown(NodeContext& node)
>
> // After everything has been shut down, but before things get flushed, stop the
> // scheduler and load block thread.
> - if (node.scheduler) node.scheduler->stop();
...
💬 ryanofsky commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2224087427)
re: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2223419766
> Unless there's way to halt processing of an IPC message mid-way, push out NewTemplate and then process the remainder that we need (much later) to reply to RequestTransactionData.Success.
I'm not sure if this is a question about the multiprocess IPC framework, or you're just deciding what the mining IPC interface should look like, but the framework is nonblocking, so there is no problem with the client making multip
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2224087427)
re: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2223419766
> Unless there's way to halt processing of an IPC message mid-way, push out NewTemplate and then process the remainder that we need (much later) to reply to RequestTransactionData.Success.
I'm not sure if this is a question about the multiprocess IPC framework, or you're just deciding what the mining IPC interface should look like, but the framework is nonblocking, so there is no problem with the client making multip
...
💬 mzumsande commented on issue "intermittent issue in feature_reindex.py: Reindex fails to continue after restart; `initload` thread hangs forever":
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2224098060)
Thanks!
Just saw that #23234 describes the same issue (and suggests the same fix).
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2224098060)
Thanks!
Just saw that #23234 describes the same issue (and suggests the same fix).
💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2224112679)
re-ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2224112679)
re-ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953
💬 tdb3 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1674830641)
Good thought. At least from an initial look, what makes sense to me would be checking if it's already null and if not call `freeaddrinfo()` (`getaddrinfo()` handles allocation and `freeaddrinfo()` handles cleanup). I suspect this could be somewhat overkill (or not happen if unsuccessful `getaddrinfo()` never sets ai_res to non-null), but we don't want a memory leak. I'm still looking for guarantees in documentation (and we'd need to potentially compare guarantees for different platforms).
...
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1674830641)
Good thought. At least from an initial look, what makes sense to me would be checking if it's already null and if not call `freeaddrinfo()` (`getaddrinfo()` handles allocation and `freeaddrinfo()` handles cleanup). I suspect this could be somewhat overkill (or not happen if unsuccessful `getaddrinfo()` never sets ai_res to non-null), but we don't want a memory leak. I'm still looking for guarantees in documentation (and we'd need to potentially compare guarantees for different platforms).
...
💬 kevkevinpal commented on pull request "log: LogError with FlatFilePos in UndoReadFromDisk":
(https://github.com/bitcoin/bitcoin/pull/30428#issuecomment-2224176117)
ACK [fa14e1d](https://github.com/bitcoin/bitcoin/pull/30428/commits/fa14e1d9d5c5dc44396a01583ae94480b7bc29ee)
I think this makes sense and mirroring `ReadBlockFromDisk` seems reasonable to me
(https://github.com/bitcoin/bitcoin/pull/30428#issuecomment-2224176117)
ACK [fa14e1d](https://github.com/bitcoin/bitcoin/pull/30428/commits/fa14e1d9d5c5dc44396a01583ae94480b7bc29ee)
I think this makes sense and mirroring `ReadBlockFromDisk` seems reasonable to me
💬 kevkevinpal commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2224232188)
> The coverage we do have comes from the process_messages harness, but the harness doesn't fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Concept ACK [75fc22f](https://github.com/bitcoin/bitcoin/pull/30413/commits/75fc22f7729c6df779162edb620f986b5c811d8b)
I think it makes sense to fuzz the handshake itself
Maybe I'm just not too familiar with fuzz testing but this seems very similar to `process_messages`.
Would it make sense
...
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2224232188)
> The coverage we do have comes from the process_messages harness, but the harness doesn't fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Concept ACK [75fc22f](https://github.com/bitcoin/bitcoin/pull/30413/commits/75fc22f7729c6df779162edb620f986b5c811d8b)
I think it makes sense to fuzz the handshake itself
Maybe I'm just not too familiar with fuzz testing but this seems very similar to `process_messages`.
Would it make sense
...
💬 kevkevinpal commented on pull request "test: [refactor] Pass TestOpts":
(https://github.com/bitcoin/bitcoin/pull/30407#issuecomment-2224255339)
utACK [fa690c8](https://github.com/bitcoin/bitcoin/pull/30407/commits/fa690c8e532672f7ab53be6f7a0bb3070858745e)
I did a grep onto `./src/test/` not sure if we want to use the `TestOpts` struct on any of these
```
grep -nri "std::vector<const char\*>" ./src/test/ -I
./src/test/argsman_tests.cpp:827: std::vector<const char*> argv = {"ignored"};
./src/test/argsman_tests.cpp:961: std::vector<const char*> argv = {"ignored"};
./src/test/fuzz/system.cpp:87: std::v
...
(https://github.com/bitcoin/bitcoin/pull/30407#issuecomment-2224255339)
utACK [fa690c8](https://github.com/bitcoin/bitcoin/pull/30407/commits/fa690c8e532672f7ab53be6f7a0bb3070858745e)
I did a grep onto `./src/test/` not sure if we want to use the `TestOpts` struct on any of these
```
grep -nri "std::vector<const char\*>" ./src/test/ -I
./src/test/argsman_tests.cpp:827: std::vector<const char*> argv = {"ignored"};
./src/test/argsman_tests.cpp:961: std::vector<const char*> argv = {"ignored"};
./src/test/fuzz/system.cpp:87: std::v
...
👍 tdb3 approved a pull request: "log: LogError with FlatFilePos in UndoReadFromDisk"
(https://github.com/bitcoin/bitcoin/pull/30428#pullrequestreview-2173580683)
cr and light test ACK fa14e1d9d5c5dc44396a01583ae94480b7bc29ee
(https://github.com/bitcoin/bitcoin/pull/30428#pullrequestreview-2173580683)
cr and light test ACK fa14e1d9d5c5dc44396a01583ae94480b7bc29ee
🤔 tdb3 reviewed a pull request: "rpc: Use CHECK_NONFATAL over Assert"
(https://github.com/bitcoin/bitcoin/pull/30429#pullrequestreview-2173621799)
Approach ACK
Ran `lint-assertions.py` against the old `rpc/blockchain.cpp` and similar to what @stickies-v observed the `Assert`s did not appear to be caught.
```
$ grep --version | grep GNU
grep (GNU grep) 3.8
```
(https://github.com/bitcoin/bitcoin/pull/30429#pullrequestreview-2173621799)
Approach ACK
Ran `lint-assertions.py` against the old `rpc/blockchain.cpp` and similar to what @stickies-v observed the `Assert`s did not appear to be caught.
```
$ grep --version | grep GNU
grep (GNU grep) 3.8
```
📝 mzumsande opened a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435)
This avoids situations during a reindex, in which the shutdown doesn't finish since `LimitValidationInterfaceQueue()` is called by the load block thread when the scheduler is already stopped, in which case it would block indefinitely. This can lead to intermittent failures in `feature_reindex.py` (#30424), which I could locally reproduce with
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 74f0e4975c..be1706fdaf 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@
...
(https://github.com/bitcoin/bitcoin/pull/30435)
This avoids situations during a reindex, in which the shutdown doesn't finish since `LimitValidationInterfaceQueue()` is called by the load block thread when the scheduler is already stopped, in which case it would block indefinitely. This can lead to intermittent failures in `feature_reindex.py` (#30424), which I could locally reproduce with
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 74f0e4975c..be1706fdaf 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@
...
👍 TheCharlatan approved a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2173996562)
ACK e427fed82f7931ae6f09a4939e0fcd6cb235ef0d
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2173996562)
ACK e427fed82f7931ae6f09a4939e0fcd6cb235ef0d
🤔 maflcko reviewed a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2174016816)
lgtm ACK e427fed82f7931ae6f09a4939e0fcd6cb235ef0d
But I think the comment can be improved to clarify that this is done in symmetry with the other threads (rpc, p2p, ...)
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2174016816)
lgtm ACK e427fed82f7931ae6f09a4939e0fcd6cb235ef0d
But I think the comment can be improved to clarify that this is done in symmetry with the other threads (rpc, p2p, ...)
💬 maflcko commented on pull request "init: change shutdown order of load block thread and scheduler":
(https://github.com/bitcoin/bitcoin/pull/30435#discussion_r1675406917)
I don't think this deadlock is limited to `LimitValidationInterfaceQueue`, but affects any thread that may call `SyncWithValidationInterfaceQueue`.
So before the scheduler is stopped, all threads in the context of rpc, p2p, indexes must have been stopped. Also chainmans' `restart_indexes` and `ABC` must not be called afterwards. Below chainman should only flush the chainstate, which should be fine, as it does not call `SyncWithValidationInterfaceQueue`.
(IPC must also be stopped, e.g. `src
...
(https://github.com/bitcoin/bitcoin/pull/30435#discussion_r1675406917)
I don't think this deadlock is limited to `LimitValidationInterfaceQueue`, but affects any thread that may call `SyncWithValidationInterfaceQueue`.
So before the scheduler is stopped, all threads in the context of rpc, p2p, indexes must have been stopped. Also chainmans' `restart_indexes` and `ABC` must not be called afterwards. Below chainman should only flush the chainstate, which should be fine, as it does not call `SyncWithValidationInterfaceQueue`.
(IPC must also be stopped, e.g. `src
...
💬 maflcko commented on pull request "init: change shutdown order of load block thread and scheduler":
(https://github.com/bitcoin/bitcoin/pull/30435#issuecomment-2224963680)
I guess it should be backported to 27.x? (My understanding is that this existed "forever", since 0.16.x, because `SyncWithValidationInterfaceQueue` never had a boost interruption point, or other interrupt check)
(https://github.com/bitcoin/bitcoin/pull/30435#issuecomment-2224963680)
I guess it should be backported to 27.x? (My understanding is that this existed "forever", since 0.16.x, because `SyncWithValidationInterfaceQueue` never had a boost interruption point, or other interrupt check)
💬 maflcko commented on pull request "rpc: Use CHECK_NONFATAL over Assert":
(https://github.com/bitcoin/bitcoin/pull/30429#issuecomment-2224984002)
Can you add exact steps to reproduce, starting from a fresh install of your operating system? Otherwise there is nothing that can be done. For a fresh install of Ubuntu 24.04 LTS (noble) or Debian GNU/Linux 12 (bookworm) it passes:
```
git switch --detach fa8b587b91dfa8df28fc13589c511b871902670b && ( git show src | git apply --reverse ) && ./test/lint/lint-assertions.py
HEAD is now at fa8b587 rpc: Use CHECK_NONFATAL over Assert
CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be u
...
(https://github.com/bitcoin/bitcoin/pull/30429#issuecomment-2224984002)
Can you add exact steps to reproduce, starting from a fresh install of your operating system? Otherwise there is nothing that can be done. For a fresh install of Ubuntu 24.04 LTS (noble) or Debian GNU/Linux 12 (bookworm) it passes:
```
git switch --detach fa8b587b91dfa8df28fc13589c511b871902670b && ( git show src | git apply --reverse ) && ./test/lint/lint-assertions.py
HEAD is now at fa8b587 rpc: Use CHECK_NONFATAL over Assert
CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be u
...
💬 maflcko commented on pull request "rpc: Use CHECK_NONFATAL over Assert":
(https://github.com/bitcoin/bitcoin/pull/30429#issuecomment-2224999042)
Same in the CI https://cirrus-ci.com/task/6075434675208192?logs=lint#L755 and when using `( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo run -- --lint=all_python_linters )` locally. Again, I'll need exact steps to reproduce, otherwise there is little that can be done.
(https://github.com/bitcoin/bitcoin/pull/30429#issuecomment-2224999042)
Same in the CI https://cirrus-ci.com/task/6075434675208192?logs=lint#L755 and when using `( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo run -- --lint=all_python_linters )` locally. Again, I'll need exact steps to reproduce, otherwise there is little that can be done.
💬 maflcko commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1675452625)
nit in the scripted-diff: Personally I prefer a non-scripted diff, if the script is larger than the diff. So I think you can split out the 8 `/d` delete statements and three script lines for a simple last commit to just manually delete those lines. If you want to keep the scripted-diff my recommendation would be to split the `/d` parts into a separate one. Otherwise review is harder, because one has to jump back and forth during review because replacements and deletions are mixed with each other
...
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1675452625)
nit in the scripted-diff: Personally I prefer a non-scripted diff, if the script is larger than the diff. So I think you can split out the 8 `/d` delete statements and three script lines for a simple last commit to just manually delete those lines. If you want to keep the scripted-diff my recommendation would be to split the `/d` parts into a separate one. Otherwise review is harder, because one has to jump back and forth during review because replacements and deletions are mixed with each other
...
💬 maflcko commented on pull request "test: [refactor] Pass TestOpts":
(https://github.com/bitcoin/bitcoin/pull/30407#issuecomment-2225015681)
> I did a grep onto `./src/test/` not sure if we want to use the `TestOpts` struct on any of these
Not sure what you mean, but I am happy to push any changes that make sense, if you provide a diff. Also, I am happy to review follow-ups, if you create one.
(https://github.com/bitcoin/bitcoin/pull/30407#issuecomment-2225015681)
> I did a grep onto `./src/test/` not sure if we want to use the `TestOpts` struct on any of these
Not sure what you mean, but I am happy to push any changes that make sense, if you provide a diff. Also, I am happy to review follow-ups, if you create one.
💬 maflcko commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2225030941)
> > Are you sure it is completely non-existent?
>
> The coverage we do have comes from the `process_messages` harness, but the harness doesn't fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Sorry for the wrong comments above. Not sure why I hallucinated that this was already fuzzed. I guess I remembered seeing the timedata warnings printed in the fuzz runner and somehow assumed this part of the code is already fully fuzzed.
...
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2225030941)
> > Are you sure it is completely non-existent?
>
> The coverage we do have comes from the `process_messages` harness, but the harness doesn't fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Sorry for the wrong comments above. Not sure why I hallucinated that this was already fuzzed. I guess I remembered seeing the timedata warnings printed in the fuzz runner and somehow assumed this part of the code is already fully fuzzed.
...