💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1655579858)
I'm not sure this is asserting what you expect. In the loading process, we load the encryption keys after the key records. So the encryption keys map should always be empty and `HasEncryptionKeys` should always return false but not for the reasons stated here.

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1655577477)
nano nit:
```suggestion
bool LegacyDataSPKM::AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey)
```

💬 ryanofsky commented on pull request "kernel, logging: Pass Logger instances to kernel objects":
(https://github.com/bitcoin/bitcoin/pull/30342#issuecomment-2192731526)
Updated 45423355735770f01d2bac738cc8b0b41415e921 -> db1b9f7696af80036de739408cd9eae5d06481ec ([`pr/gklog.1`](https://github.com/ryanofsky/bitcoin/commits/pr/gklog.1) -> [`pr/gklog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/gklog.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/gklog.1..pr/gklog.2)) moving code to an earlier commit to fix a "test-each-commit" test failure https://github.com/bitcoin/bitcoin/actions/runs/9684398780/job/26722073921?pr=30342, and cleaning up cod
...

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1655636322)
Yes, that's what the comment is saying. It is because this function is only called during loading, and because legacy wallet records are loaded first, that this assertion makes sense. After all, to be migrated legacy wallets can still be encrypted.

📝 ceffikhan opened a pull request: "test: fix debug log assertion in p2p_i2p_ports test"
(https://github.com/bitcoin/bitcoin/pull/30345)
This PR addresses an p2p_i2p_ports.py test in the Bitcoin Core test framework where the expected debug log message was incorrect, causing the test to fail intermittently.

**Changes:**

- Corrected the expected debug log message in test/functional/p2p_i2p_ports.py to properly match the error message generated.

**Steps Taken to Fix:**

-Updated the expected debug log message in the method.

Closes #30030

💬 tdb3 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655953816)
Good catch. Might also be good to print a log message before returning for awareness.

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1656109703)
re: https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655528095

> `source` feels a bit abstract and special case, it would be good to keep `LogDebug(BCLog::CATEGORY, ...` examples as the most prominent as they are the common case.

Great point, reverted this. There's no need to change this document anymore since the current version of this PR is backwards compatible (unlike the original version) so the original recommendations are still good. There's more complete reference documen
...

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1656110508)
re: https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655531815

Thanks again, but for now I did not take this suggestion. I think making individual struct members const can unnecessarily restrict the ways the struct can be initialized. I think the safety benefits of making the `category` member const are better enforced and more explicit when the entire struct is const, which should be best practice and already the case everywhere.

🤔 ryanofsky reviewed a pull request: "Improve new LogDebug/Trace/Info/Warning/Error Macros"
(https://github.com/bitcoin/bitcoin/pull/29256#pullrequestreview-2143744323)
Updated 5f64eab013a58967af37a59c863c2ab6d45ba6e8 -> 38d3298ea7e547797871140a02df3e7d60d34d36 ([`pr/bclog.17`](https://github.com/ryanofsky/bitcoin/commits/pr/bclog.17) -> [`pr/bclog.18`](https://github.com/ryanofsky/bitcoin/commits/pr/bclog.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bclog.17..pr/bclog.18)) with suggested changes.

---

re: https://github.com/bitcoin/bitcoin/pull/29256#pullrequestreview-2142978382

Thank you for the close review!

On disallowing catego
...

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1656112131)
re: https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655537932

> Could be made `constexpr` here and in other places:

Thanks! I don't think there's an obvious way to make these constexpr, but they should be const, so changed the declarations here, below, and in the next PR.

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1656111030)
re: https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655531667

> Could do with a comment such as `// Implicitly constructible from a LogFlags value in order to be created by LogDebug(BCLog::NET, ...) etc.` if the current approach is kept.

Thank you! Added.

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1656113790)
re: https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655562082

> Feels like a half-baked workaround for having removed the global `LogPrintf_`.

Yes, good catch. I wanted to leave the test case below unchanged in the main commit to check (and let it be obvious) that the new logging features do not change any current behavior. But it's not good to leave behind a reference to an old function with a confusing name, so I added a new commit afterwards to modernize the test.

⚠️ 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`