💬 TheCharlatan commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105070733)
I think you can remove the `ArgsManager` from the arguments here.

👍 Lokoluis approved a pull request: "test: Fix intermittent sync issue in wallet_pruning"
(https://github.com/bitcoin/bitcoin/pull/27093)

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105084574)
done

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105084632)
done

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#issuecomment-1428788415)
Updated per feedback. Thanks. Tiny [diff](https://github.com/bitcoin/bitcoin/compare/ebb14064a0589bc56b71e267204696accc577036..fafaa7a7e15a49da0214ce9546a42117c63e611f)

💬 hernanmarino commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1428803693)

> Noticed that we are calling cli-only commands with the "-" prefix. Would be good to follow the same convention here too. The renaming will let us detect them in a simpler manner by checking the first character without requiring to split the string etc.

This seems like something "nice to have" but there are other issues apart from what Pablo mentioned that makes me think that we shouldn´t do that:

- There are other console-only commands that already existed, like `help-console`
- Thi
...

💬 hernanmarino commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1428811330)
> I agree with furszy's idea of introducing the functions `IsCliOnlyCommand ` and `ExecuteCliCommand `, to implement the `generate` cli-only command, for a cleaner implementation and with the advantage of leaving the structure ready for potential expansion of additional cli-only commands.

The provided method `executeConsoleOnlyCommand` is almost identical to the suggested `ExecuteCliCommand` and is also ready for potential future new console only commands, the only difference is that it detec
...

💬 sipa commented on pull request "net: avoid overriding non-virtual ToString() in CService and use better naming":
(https://github.com/bitcoin/bitcoin/pull/25619#issuecomment-1428811771)
utACK c9d548c91fb12fba516dee896f1f97692cfa2104

💬 rserranon commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1428851086)
testing ACK

👍 ponury1990 approved a pull request: "wallet: ensure the wallet is unlocked when needed for rescanning"
(https://github.com/bitcoin/bitcoin/pull/26347)

💬 w0xlt commented on pull request "[Draft / POC] Silent Payments":
(https://github.com/bitcoin/bitcoin/pull/24897#issuecomment-1429142222)
Update: The silentpayment index has been removed from the codebase.

`scantxoutset` has been changed to use `txindex` and `CBlockUndo`. So the scan works without silentpayment index.

`getsilentpaymentblockdata` also works without any indexes, using only `CBlockUndo`. The code is relatively simple. Therefore, even serving light clients, there is no need for indexing.

Seems to work fine. Not sure about the performance.

A new PR, with the index, will be added soon.

💬 darosior commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1105320169)
Will do if i need to retouch.

💬 martinus commented on pull request "Add simulation-based `CCoinsViewCache` fuzzer":
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1429201435)
@sipa While rebasing #25325 I saw that the fuzzer in `coins_view.cpp` still uses `CCoinsMap coins_map;` and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?

I can add it while rebasing, I'm touching that line anyways. Unless you want to do this separately

💬 hebasto commented on pull request "Check for the awk script before executing it":
(https://github.com/bitcoin/bitcoin/pull/27095#issuecomment-1429249881)
@Harshil-Jani

Thank you for your contribution! I think it is more appropriate to submit your patch to the [upstream project](https://github.com/kadwanev/retry).

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1429293948)
Rebased due to #27011. Note that this adds a `/*deterministic=*/true` [here in test/fuzz/coins_view.cpp](https://github.com/bitcoin/bitcoin/pull/25325/commits/78f597be2879c39d9d2b98e21ed0120d2308de20#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R119), see `git range-diff 0007d69...78f597b`

💬 TheCharlatan commented on pull request "refactor, kernel: Remove gArgs accesses from dbwrapper and txdb":
(https://github.com/bitcoin/bitcoin/pull/25862#issuecomment-1429298769)
Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f

👍 TheCharlatan approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 MarcoFalke commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1429330974)
Closing for now due to controversy.

✅ MarcoFalke closed a pull request: "refactor: Move coin_control variable to test setup section"
(https://github.com/bitcoin/bitcoin/pull/26154)

💬 MarcoFalke commented on pull request "Use designated initializers":
(https://github.com/bitcoin/bitcoin/pull/24531#discussion_r1105484093)
Just for clarity and documentation. This code is wrong as-is and has long been removed. Omitting the field name here can lead to bugs, see https://godbolt.org/z/9TnoMKMT3 for a playground.