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

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1187451150)
I think unreachable code can be `Assert(false)`?

💬 Sjors commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1538386300)
Ran the same commit again with `-debug=ipc` to get more useful log info. ([log](https://gist.github.com/Sjors/c2b227d5c7d210524b9ca7d78b93c2ed))

I also tried disabling the indexes to simplify the setup, but that didn't prevent the issue.

💬 mononaut commented on pull request "rpc: distinguish between vsize and sigop-adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/27591#issuecomment-1538391444)
Concept ACK

💬 fanquake commented on pull request "refactor: Remove unused GetTimeMillis":
(https://github.com/bitcoin/bitcoin/pull/27594#issuecomment-1538393639)
Nice - Concept ACK