💬 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

💬 fanquake commented on pull request "refactor: Remove unused GetTimeMillis":
(https://github.com/bitcoin/bitcoin/pull/27594#issuecomment-1538394966)
cc @martinus

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1187478957)
I'll clarify what I meant here: you can copy-paste the result of `listdescriptors`, because this now uses `h` instead of `'`

👍 ryanofsky approved a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1416823725)
Code review ACK 3b34ac7465919b968795063995f6610a73aa2d29. Left some comments but this looks good as is and would not change unless necessary

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187459164)
In commit "refactor: Move functions to BlockManager methods" (1def580e80a1892e473324016db9a0f2ffec0c27)

re: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1140490220

> Thank you for the suggestion, will split this up.

Seems like this could be marked resolved

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187472027)
In commit "refactor: Use BlockManager options for params" (0400fb7b55415867d9ff0aa88898920c60b4b2df)

Seem like you could squash this commit into the last commit and decrease review burden because the changes are mechanical and basically every line that changed here changed last commit. Not important though.

💬 Sjors commented on pull request "Switch hardened derivation marker to h":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1187491311)
Descriptors have always required parsers to handle both `h` and `'`, and I'm not aware of any that don't. For old BIP32 derivation parsing software I'm less sure about that.

💬 Sjors commented on pull request "Switch hardened derivation marker to h":
(https://github.com/bitcoin/bitcoin/pull/26076#issuecomment-1538425147)
Thanks for the review!

The reason for not breaking legacy wallet RPC calls is that I'm assuming any code that uses legacy wallets is really old and nobody wants to touch it. On our side eventually we want to get rid of the legacy wallet - or at least freeze it - so there's no need to modernise it.

I updated the title, description and release note. Pushed the latter as a new commit to not lose ACKs.