💬 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
...

👍 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

🤔 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
```

📝 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
@
...

👍 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

🤔 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, ...)

💬 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
...

💬 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)

💬 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
...

💬 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.

💬 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
...

💬 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.

💬 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.


...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225041208)
> the framework is nonblocking, so there is no problem with the client making multiple calls at the same time, and the client can also decide what threads in the server the calls run on. So any interface you would like to implement should be possible to implement.

I'm not sure if that helps. I was also a bit confused, so let me rephrase it:

The current interface `createNewBlock` returns a template, which contains 4 MB of serialized transaction data. Ideally I would _not_ include those tran
...

💬 maflcko commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1675479974)
I guess it would still be good to leave a comment about the race in the source code (or in the docs on how to avoid the race). I guess the user would simply have to avoid calling other `*block` RPCs for the duration of `dumptxoutset`? (The p2p is already stopped, so shouldn't be the cause of a race)

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1675498531)
Great, thank you!

👍 alfonsoromanz approved a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2174193471)
Re ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953

🚀 fanquake merged a pull request: "depends: update doc in Qt pwd patch"
(https://github.com/bitcoin/bitcoin/pull/30336)

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2225104347)
See https://github.com/bitcoin/bitcoin/issues/29359

📝 hodlinator opened a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436)
Prompted by comment in https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200 (referring to https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378).