🤔 ryanofsky reviewed a pull request: "wallet, logging: Replace WalletLogPrintf() with LogInfo()"
(https://github.com/bitcoin/bitcoin/pull/30343#pullrequestreview-2144006095)
Thanks for the review! The suggestions are a big improvement and make the logging code more succinct and clean.

Rebased 5b30f17c22ade44d301aed5051a398fa38251ef7 -> 5a629a60c4739b94383596f9d040502b7b2c8b69 ([`pr/gwlog.1`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.1) -> [`pr/gwlog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/gwlog.1-rebase..pr/gwlog.2)) on top of the updated parent PR implementing review sugg
...

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656292662)
re: https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655407301

> `LogWarning`?

Thanks, done in a new commit (see earlier comment)

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656291638)
re: https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655403164
> Why not rename `GetLogSource()` -> `Log()` here and in `WalletStorage` and keep `m_log` private? I tried and it compiles as of [5b30f17](https://github.com/bitcoin/bitcoin/commit/5b30f17c22ade44d301aed5051a398fa38251ef7).

Nice suggestion, implemented. This is much cleaner looking.

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656290463)
re: https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655436010

> `LogInfo(m_log, "Error:` -> `LogError(` here and in other places?

Thanks, done in a new commit (see earlier comment)

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656289947)
re: https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655411776

> nit: Should strive for alphabetical ordering?

Yes, now piped these through `sort`

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656291029)
re: https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655434943

> Should maybe have been a warning all along?

Good catch. It actually looks to me like all of the log prints in this function should be errors. I changed them to use LogError in a new commit. It's a separate commit because the level change is a minor change in behavior, and is less obvious than other level changes.

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#issuecomment-2193679390)
Updated 5a629a60c4739b94383596f9d040502b7b2c8b69 -> db8c6fc2ca5e1ad99aeda741e3b453f1f7bf7955 ([`pr/gwlog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.2) -> [`pr/gwlog.3`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/gwlog.2..pr/gwlog.3)) fixing initialization order bug in last push

✅ fanquake closed an issue: "Issue:XPUN ACCOUNT P Title"
(https://github.com/bitcoin/bitcoin/issues/30346)

:lock: fanquake locked an issue: "Issue:XPUN ACCOUNT P Title"
(https://github.com/bitcoin/bitcoin/issues/30346)

💬 maflcko commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656474489)
Are you sure that `LogError` is the right level? It should only be used for fatal errors where the node needs to shut down, according to the docs. This seems like a warning here, as suggested above.

Some wallet code can probably use `Result` and pass errors up, possibly here as well. And the caller can decide whether to log as a warning or return over RPC (or something else).

🤔 stratospher reviewed a pull request: "cli: restrict multiple exclusive argument usage in bitcoin-cli"
(https://github.com/bitcoin/bitcoin/pull/30148#pullrequestreview-2141106606)
tested ACK d2e8611.

2 more suggestions:
1. include new `OptionsCategory` in the fuzz test too - https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/src/test/fuzz/system.cpp#L56
2. add functional test coverage for this error in `test/functional/interface_bitcoin_cli.py`

💬 stratospher commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1654372307)
b382bf4: nit: s/Cli/CLI

💬 maflcko commented on pull request "Remove redundant `-datacarrier` option":
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2193911303)
Not sure what to do here. So far it doesn't look like there has been a single reviewer in support of this change, so maybe it can be closed for now? If need or support arises in the future, it would be trivial to pick up again, but until then I am not sure this needs to sit around in the `open` state.

💬 maflcko commented on pull request "Remove redundant `-datacarrier` option":
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2193914025)
Again, I'd be happy to review this change, if the changes to `init.cpp` and `./test/` were dropped and this was a refactor.

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1656555460)
> Are you sure that `LogError` is the right level? It should only be used for fatal errors where the node needs to shut down, according to the docs. This seems like a warning here, as suggested above.

Oh, that is interesting. The descriptions of those log levels in https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md are much more severe than I would have expected. Based on those descriptions I would expect LogError to be called LogFatal and
...

✅ maflcko closed an issue: "Brainstorm: Transaction issuer-selected policy limits"
(https://github.com/bitcoin/bitcoin/issues/29454)

💬 maflcko commented on issue "Brainstorm: Transaction issuer-selected policy limits":
(https://github.com/bitcoin/bitcoin/issues/29454#issuecomment-2193933082)
Closing for now, due to inactivity. As this policy discussion isn't directly and exclusively related to the Bitcoin Core code base, it could make more sense to discuss with the greater ecosystem first, for example https://groups.google.com/g/bitcoindev, or https://delvingbitcoin.org/ ?

💬 Sjors commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1656567090)
Taken

💬 Sjors commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1656567248)
Reworded

💬 Sjors commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#issuecomment-2193950235)
Addressed feedback and expanded PR description.