⚠️ Vero7979 opened an issue: "Issue:XPUN ACCOUNT P Title"
(https://github.com/bitcoin/bitcoin/issues/30346)
<!-- Edit the body of your new issue then click the ✓ "Create Issue" button in the top right of the editor. The first line will be the issue title. Assignees and Labels follow after a blank line. Leave an empty line before beginning the body of the issue. -->

💬 Fonta1n3 commented on issue "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs.":
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2193612603)
Hmm not sure I understand why combinepsbt would be involved in stripping the data or if I'm misreading.

In Payjoin transactions for example you need to send a psbt to your counterparty, stripping that psbt of any bip32derivs which the counter party needn't sign for or outputs they don't need to verify would be beneficial for privacy.

In short, you'd need to strip the bip32derivs before using combinepsbt or joinpsbts.

💬 Fonta1n3 commented on issue "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs.":
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2193617560)
Thank you for opening a PR I will keep an eye on it and add any input I can there. Thanks!

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

> Shouldn't this be a `LogError`/`LogWarning` with removed `Error: `-prefix?

Agree that makes sense. I implemented it in a followup commit so the main commit is still a refactoring that does not change behavior.

For now I did not remove "Error:" and "Warning:" prefixes because it would be a more significant, possibly unwelcome change, since the prefixes could help messages stand out, and might be relied upon by tes
...

🤔 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)