⚠️ MarcoFalke opened an issue: "test: RPC coverage check doesn't work?"
(https://github.com/bitcoin/bitcoin/issues/27593)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Currently CI reports that all RPCs are covered: https://cirrus-ci.com/task/6244871116161024?logs=ci#L4674

### Expected behaviour

However, some RPCs are clearly not covered because they are never called. For example:

```
$ git grep abortrescan ./test/ | grep -v '. Please call `abortrescan` before ' | wc -l
0
```

### Steps to reproduce

?

### Relevant log output

_No response_


...

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187190754)
nit in 1def580e80a1892e473324016db9a0f2ffec0c27: (For this method and others like `UndoReadFromDisk` ...) It is UB to pass a nullptr pindex, so it would be good, while touching the function signature in this pull request to switch the pointer to a reference.

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187174928)
nit in ea2a049b42f73499eaa1b69212af3514026440fd: Any reason to accept a nullptr here, which would be UB?

I think it would be better to accept a `const CBlockIndex& block`.

👍 MarcoFalke approved a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1416363907)
nice ACK 3b34ac7465919b968795063995f6610a73aa2d29 🗣

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 3b34ac7465919b96
...

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187180745)
Also, it would be good to include the diff in `src/zmq/zmqpublishnotifier.cpp` from the next commit. This makes review easier, because reviewers can see that the function-to-lambda is a simple 1-1 replacement that compiles and passes all tests on its own.

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187211843)
in 0400fb7b55415867d9ff0aa88898920c60b4b2df: The function parameter consensusParams is now unused and should be removed?

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187263200)
Thanks, thread can be resolved

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187263434)
Thanks, thread can be resolved

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187263618)
Thanks, thread can be resolved

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187263868)
Not worth it on a second thought, thread can be resolved

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187263993)
Thanks, thread can be resolved

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187264411)
Thanks, thread can be resolved

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187264889)
or maybe the unrelated comment https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183633068 can be transformed into an issue?

💬 MarcoFalke commented on issue "test: RPC coverage check doesn't work?":
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1538118862)
This may be a good first issue for anyone with python background and passion to dig into the test framework. A starting entry point would be the help text: `./test/functional/test_runner.py --help |grep coverage`.

For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.

📝 MarcoFalke opened a pull request: "refactor: Remove unused GetTimeMillis"
(https://github.com/bitcoin/bitcoin/pull/27594)
The function is unused, not type-safe, and does not denote the underlying clock type. So remove it.

💬 MarcoFalke commented on pull request "fuzz: BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1538252773)
> Finally, I also observed that the fuzzer gets slower with time and after a few hours creates inputs that will take several seconds to run. That seems unavoidable, because the chains that are created are getting longer, and each block must be mined at a small but non-zero difficulty. However, it means that we should probably keep an eye on the qa-corpus seeds once this gets merged.

I guess `LIMITED_WHILE` can be reduced so much that it can catch the CVE only. For BIP 42 and all other purpose
...

💬 MarcoFalke commented on pull request "fuzz: BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1187374067)
Yes, could move outside

💬 kouloumos commented on issue "test: RPC coverage check doesn't work?":
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1538330399)
The RPC coverage works by comparing the invoked RPCs with the full list of available RPC commands per `bitcoin-cli help`. What seems to be problem is that the `rpc_interface.txt` reference-list that we create as part of [`write_all_rpc_commands()`](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/coverage.py#L80) doesn't contain the wallet RPC commands.

Because this is per test run (`rpc_interface.txt` is created once per coverage directory), a possible issue coul
...

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1538364221)
Rebased and tested that it still works.

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1187440836)
Shouldn't use this switch-case, now that the type is an enum class?