💬 furszy commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1655511363)
Not sure about using `tx_size`, we would be returning an estimation of the final size of tx. Not the real/current tx weight.

What about getting the current tx weight by serializing the `CMutableTransaction` object that appears inside `FinishTransaction` and explain in the RPC return value description that the returned weight will not be the final one if the transaction is not complete.

💬 furszy commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1655498154)
Same as above, guess that this was used for testing purposes and you forgot to remove it?

💬 furszy commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1655502137)
Isn't the purpose of this test case to include the change output and fail after wise? (not with the "can not accommodate change output" error message)

💬 jonatack commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1655564731)
Yes, I see that your script removed my onion and i2p nodes then and also here.

🤔 hodlinator requested changes to a pull request: "Improve new LogDebug/Trace/Info/Warning/Error Macros"
(https://github.com/bitcoin/bitcoin/pull/29256#pullrequestreview-2142978382)
NACK 5f64eab013a58967af37a59c863c2ab6d45ba6e8

At first glance it seems good to be able to specify an optional category to `LogInfo`, but it risks encouraging inflating `LogInfo` usage over `LogDebug`. This could open up future arguments like "you can just filter out the whole category if you don't like my added `LogInfo`". How about disallowing *category* for `LogInfo` like we currently do, but optionally accept something like *source* for the occasional exception like wallet code in #30343?
...

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(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.

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(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.

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655537932)
Could be made `constexpr` here and in other places:
```suggestion
constexpr auto EXPECTED = std::to_array({
```

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655531815)
```suggestion
const LogFlags category;
```

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1655562082)
Feels like a half-baked workaround for having removed the global `LogPrintf_`.

✅ maflcko closed a pull request: "lint/contrib/doc updates"
(https://github.com/bitcoin/bitcoin/pull/30084)

💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2192698872)
Closing for now, due to inactivity. In any case, the major build system changes should be separate from lint/doc typo fixes, so if you want to pick this up again, I'd recommend to create two separate pull requests.

🤔 hodlinator reviewed a pull request: "wallet, logging: Replace WalletLogPrintf() with LogInfo()"
(https://github.com/bitcoin/bitcoin/pull/30343#pullrequestreview-2142664600)
Went through this and the parent PR together. It was helpful to see how that other PR's changes were used in this one.

💬 hodlinator commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(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 5b30f17c22ade44d301aed5051a398fa38251ef7.

💬 hodlinator commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655406670)
Shouldn't this be a `LogError`/`LogWarning` with removed `Error: `-prefix?

💬 hodlinator commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655436010)
`LogInfo(m_log, "Error:` -> `LogError(` here and in other places?

💬 hodlinator commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655411776)
nit: Should strive for alphabetical ordering?

💬 hodlinator commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655407301)
`LogWarning`?

💬 hodlinator commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655434943)
Should maybe have been a warning all along?

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