💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2672606808)
If people think that deprecation of `settxfee` is appropriate, then I fail to see why `-paytxfee` isn't deprecated at the same time. If anything, any reason for deprecation should be even stronger for that setting, given that it isn't dynamic and thus can't be used with dynamic fee estimation at all, and even less without the rpc.

🤔 glozow reviewed a pull request: "wallet, rpc: deprecate settxfee"
(https://github.com/bitcoin/bitcoin/pull/31278#pullrequestreview-2631078036)
concept ACK, I wish I saw this earlier

💬 glozow commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1964293216)
RPC examples for "specify a feerate per transaction" would be helpful

💬 achow101 commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2672613100)
Concept ACK

The startup option should also be deprecated.

💬 fanquake commented on pull request "fuzz: Limit wallet_notifications iterations (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31467#issuecomment-2672635509)
@dergoegge you likely also have some thoughts here?

✅ fanquake closed a pull request: "test: locking -testdatadir when not specified and then deleting lock and dir at end of test"
(https://github.com/bitcoin/bitcoin/pull/31328)

💬 fanquake commented on pull request "test: locking -testdatadir when not specified and then deleting lock and dir at end of test":
(https://github.com/bitcoin/bitcoin/pull/31328#issuecomment-2672636274)
I think its fine to leave this as-is for now. So going to close.

💬 maflcko commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2672638759)
> > You can also set -O1 instead of -O0, but I guess you don't want that?
>
> Just re-read your message, isn't `-O2` our default? i.e. I don't think I was using `-O0` when I first saw this.

You explicitly set it to the empty string in `-DCMAKE_CXX_FLAGS_RELWITHDEBINFO=""` and IIRC the default for compilers is `-O0`, so I presumed that this was intentional.

💬 fanquake commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2672644248)
Moved to draft for now. You'll need to squash your commits, and address the various review feedback.

📝 fanquake converted_to_draft a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668)
Fix related to issue: https://github.com/bitcoin/bitcoin/issues/31263

💬 zaidmstrr commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2672645087)
Concept ACK

💬 zaidmstrr commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1964317304)
Done

📝 mahdirahimi1999 opened a pull request: "Refactor file handling with context managers in multiple files"
(https://github.com/bitcoin/bitcoin/pull/31919)
**Description:**
This pull request refactors multiple instances of file handling across different files by replacing direct `open()` calls with context managers. The change does not affect the core functionality of the application but improves resource management by ensuring that files are properly closed after use. This results in cleaner code and better resource handling without altering any existing behavior.

✅ maflcko closed a pull request: "fuzz: Limit wallet_notifications iterations (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31467)

💬 maflcko commented on pull request "fuzz: Limit wallet_notifications iterations (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31467#issuecomment-2672649759)
Follow-ups could be:

* Remove knapsack, or disable it for this target
* Temporarily remove the target until knapsack is removed
* Other possible avenues for speed-up of this target, or a cleanup, or a fix for stability issues

Though, I don't have time to work on this right now, so closing for now.

💬 fanquake commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2672651916)
Concept ACK - has enforcement, few conflicts, added to 30.x.

✅ fanquake closed a pull request: "Refactor file handling with context managers in multiple files"
(https://github.com/bitcoin/bitcoin/pull/31919)

👍 fanquake approved a pull request: "test: Remove non-portable IPv6 test"
(https://github.com/bitcoin/bitcoin/pull/31580#pullrequestreview-2631146511)
ACK d871d778251c35fd55d420ecf5c278f3edfea227

🚀 fanquake merged a pull request: "test: Remove non-portable IPv6 test"
(https://github.com/bitcoin/bitcoin/pull/31580)

💬 glozow commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1964333623)
This `CFeeRate` construction seem wrong to me - you've swapped the size and fee?