💬 ryanofsky commented on pull request "Add settings.json prune-prev, proxy-prev, onion-prev settings ":
(https://github.com/bitcoin-core/gui/pull/603#issuecomment-1428630595)
Just to summarize discussion above, I think that as long we believe it is useful to store disabled prune and proxy values, then `settings.json` is a better place to store them than QSettings, so this PR is implementing the right approach, and is ready for review/testing/merge.

If we think it's ok to reset disabled settings to default instead of saving them, then that is the current behavior, and this PR could just be closed. I'm beginning to think more that it is good to store disabled settin
...

💬 achow101 commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1428632058)
> > 3. Check to see if the wallet would decrypt using the substring up to the first null and only then emit errors (or even just warnings) like in 1 or 2.
>
> I think this is the best approach. It's not _that_ far-fetched for a user to have used a null character in their password (e.g. through random password generator) so this will quite likely actually affect some users?

I highly doubt it. I would be surprised if any password manager produced a passphrase that included non-typeable chara
...

📝 Harshil-Jani opened a pull request: "Check for the awk script before executing it"
(https://github.com/bitcoin/bitcoin/pull/27095)
Hi ! I am new to the Bitcoin contributions. I was taking a look at the possible TODOs and I found a TODO in the file `ci/retry/retry` where, awk script was being used and we need to check if it already exist or not. This patch completes the TODO and would possibly be my very first contribution to the Bitcoin Protocol.

Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear
...

💬 achow101 commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1428659487)
NACK

While this isn't incorrect, it's not meaningfully helpful. It doesn't really add anything useful to the codebase. The code as it is now is not broken nor necessarily wrong either.

Yes, it could be a problem if the test is removed, but it's unlikely that the test will be removed, and if it is removed, the line can be just as easily moved at that time.

PRs like this just end up being noise both in terms of open PRs, and in code churn. While this probably won't cause anything to need
...

💬 john-moffett commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1105010018)
While you're touching, maybe a good idea to change to `std::vector<unsigned char, secure_allocator<unsigned char>>`?

💬 sipa commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1105008572)
Given the need for all these dummy `SignatureData` objects that need to be created in calls, would it make sense to add another overload of `SignSignature` that has no `SignaturaData&` argument?

💬 mzumsande commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#issuecomment-1428684154)
[f5724d1 ](https://github.com/bitcoin/bitcoin/commit/f5724d18878c292e4cdc766ac93d28d4252dd157)to [080f83e](https://github.com/bitcoin/bitcoin/commit/080f83e43ddc1fd2ddc57f6590b9166a373d38c2):

I added a commit to change the make the RPC return `false` in the case of unavailable data and improved the error messages. I left `init` behavior unchanged in this case (no aborts).

💬 ryanofsky commented on pull request "refactor, kernel: Remove gArgs accesses from dbwrapper and txdb":
(https://github.com/bitcoin/bitcoin/pull/25862#discussion_r1105021213)
re: https://github.com/bitcoin/bitcoin/pull/25862#discussion_r1087877059

In commit "refactor, validation: Add ChainstateManagerOpts db options" (cde3882cd4d4d1ee5682cab47d05e786cc798eb8)

> Not going against this change, but this line removal doesn't look to be part of a "non-behavior change" commit.

Good catch. It looks like this line got dropped due to a bad rebase. It was not supposed to be deleted.

💬 achow101 commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1428731690)
ACK 0bb90cabc8e2c6bbbc3c52ec1170e725435b625d

💬 sipa commented on pull request "Modernize rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/27081#discussion_r1105069472)
Thanks; I've made some similar changes there.

💬 sipa commented on pull request "Modernize rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/27081#discussion_r1105070609)
Done.

💬 jamesob commented on pull request "Modernize rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/27081#issuecomment-1428763175)
Github ACK https://github.com/bitcoin/bitcoin/pull/27081/commits/e4e17907b686c25dda91e569645a8f501ca48f90

🤔 TheCharlatan requested changes to a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 TheCharlatan commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105059343)
You can also get rid of the `system.h` include in this file.

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