💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1655435296)
In f732495191d346b8:

The reset call needs to be moved above the `reload_wallet` function declaration. Otherwise, the wallet will be reloaded in read-only mode for any failure occurring in-between the `reload_wallet` declaration and this line, such as failures during backup creation or the decryption procedure.

I have written a test to trigger this misbehavior: furszy@c24b673 (feel free to pull it here). The test passes when the fix is cherry-picked on top: furszy@2d5d9a4.

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2192524319)
Rebased to remove a workaround that was added in the meantime.

🤔 furszy reviewed a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050#pullrequestreview-2142868185)
ACK functionality wise. Not blocking if others are happy with the current code.

Have to admit that I'm not fan of the multiple `CTxOut` creations and `GetScriptForDestination` calls when we could cache the `CTxOut` early in the process, leaving the silent payment destinations empty, just so they can be modified later on. The first commit `GetSerializeSizeForRecipient` and `IsDust` decouplings seem more like an unneeded indirection rather than something worth doing to me atm. But as said above
...

💬 TheCharlatan commented on pull request "kernel, logging: Pass Logger instances to kernel objects":
(https://github.com/bitcoin/bitcoin/pull/30342#issuecomment-2192559502)
Concept ACK.

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1655499562)
Fixed and pulled the test. I decided to just move this into `reload_wallet` directly, it doesn't need to live outside of it.

🤔 furszy reviewed a pull request: "Wallet: Add `max_tx_weight` to transaction funding options (take 2)"
(https://github.com/bitcoin/bitcoin/pull/29523#pullrequestreview-2142912594)
Code reviewed b3ac1179ff90f, getting closer. Left few topics.

💬 furszy commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1655491087)
tiny nit:
```suggestion
max_selection_weight -= change_outputs_weight;
```

💬 furszy commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1655496818)
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_r1655503306)
`PEP 8: E305 expected 2 blank lines after class or function definition, found 1`. Let's not remove this line.

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