💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2012590158)
This pull is merged and closed, so it will have to be a fresh pull with fresh commits on top of current master.

💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2012598409)
noted, will do

🤔 pablomartin4btc reviewed a pull request: "wallet: migration, don't create spendable wallet from a watch-only legacy wallet"
(https://github.com/bitcoin/bitcoin/pull/31423#pullrequestreview-2694630661)
I see the changes done on `test/functional/wallet_migration.py` applied on master makes the test to fail, validating its purpose, however when I test this branch manually (on both `bitcoin-qt` and `bitcoind` + `cli`) I got a **bad_function_call** (returning from [here](https://github.com/bitcoin/bitcoin/blob/33b170721d30e71716ff2a41a7fc56bc0987dca4/src/wallet/wallet.cpp#L4403)) when I create a "blank" legacy wallet, import an address (watch-only)
and run the `migratewallet` to it (both wallets
...

💬 pablomartin4btc commented on pull request "wallet: migration, don't create spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2001090482)
Any reasons why you wouldn't use `std::filesystem::path::string` instead (same for other instances):
```suggestion
LogPrintf("Using wallet %s\n", m_dir_path.string());
```

💬 pablomartin4btc commented on pull request "wallet: migration, don't create spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2001387078)
nit (to add a bit of clarity - I also think this `CWallet::ApplyMigrationData` should be refactored, at least extracting some behaviour into functions to begin with if possible):
```suggestion
bool is_local_wallet_empty = data.desc_spkms.empty() && !data.master_key.key.IsValid();
```

💬 l0rinc commented on issue "fuzz: psbt_base64_decode fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2752074884)
Thanks a lot for checking @hodlinator!

According to https://timsong-cpp.github.io/cppwp/n4861/basic.exec#intro.execution-10:
> evaluations of operands of individual operators [...] are unsequenced. [...] If a side effect on a memory location [...] is unsequenced relative to [...] a value computation using the value of any object in the same memory location [...], the behavior is undefined

It seems that because the operands of `==` are unsequenced, side effects can lead to undefined behavior -
...

💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2752083577)
> I don’t think it makes sense to add just the iteration count, if it should be refactored completely anyway.—Better to do it all in one.

That would be great if you manage to find the time to do it.

> I extended the feerate sensitivity test to show that it prefers two heavy inputs at low feerate over two light inputs and vice versa at high feerate: https://github.com/bitcoin/bitcoin/commit/afd4b807ff1300e4f74ceab6a683f3ff1376369d#diff-36088c93368e137d955348aba223985bd4f198f2aaecd626c830f4612ca
...

💬 l0rinc commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r2012659913)
For the record, this turned out not to be perfection after all, this is UB since the operands are unsequenced therefore it can evaluate the right side first (while the left one has a side-effect that modifies it).
https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2752074884

💬 yancyribbens commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-2752121021)
Concept ACK

🤔 ryanofsky reviewed a pull request: "cmake: Avoid fuzzer "multiple definition of `main'" errors"
(https://github.com/bitcoin/bitcoin/pull/31992#pullrequestreview-2714510681)
Updated 60d2afe65484f755d99191bb650b9f9a784ee2c2 -> 57d8b1f1b3375452213c48ce80e8207e2fe53ebc ([`pr/subtree-fuzz.2`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree-fuzz.2) -> [`pr/subtree-fuzz.3`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree-fuzz.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree-fuzz.2..pr/subtree-fuzz.3)) making suggested code changes and also dropping the check that forbid specifying -DSANITIZERS=fuzzer without -DBUILD_FOR_FUZZING=ON. (Be
...

💬 ryanofsky commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#discussion_r2012558286)
re: https://github.com/bitcoin/bitcoin/pull/31992#discussion_r2010617246

Thanks made both changes and renamed SANITIZE_FLAG and FUZZ_FLAGS variables. I wasn't aware of the caps convention but it makes sense and seems helpful.

💬 ryanofsky commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#discussion_r2012539907)
re: https://github.com/bitcoin/bitcoin/pull/31992#discussion_r2010614869

Thanks, fixed spacing. The other change seems like it would duplicate the regex, so I it's not a change I would like to spend time implementing myself. But if you want to post a more complete change that I can apply, I'd happy to apply it, since I don't have a strong preference.

📝 polespinasa opened a pull request: "wallet, rpc: (v31.0) remove settxfee and paytxfee (work in progress)"
(https://github.com/bitcoin/bitcoin/pull/32138)
**Summary**

This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.

📝 polespinasa opened a pull request: "test: remove strict restrictions"
(https://github.com/bitcoin/bitcoin/pull/32139)
Removed the wallet restrictions for `rpc_deprecated.py` and added specific test case for the current deprecated rpc. Also, added a wallet creation as it's not longer created by default.

For more context check https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2011661090

💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2012742164)
@maflcko please see https://github.com/bitcoin/bitcoin/pull/32139

👋 polespinasa's pull request is ready for review: "wallet, rpc: (v31.0) remove settxfee and paytxfee"
(https://github.com/bitcoin/bitcoin/pull/32138)

💬 polespinasa commented on pull request "wallet, rpc: (v31.0) remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-2752241332)
Note to self: re-check all tests

💬 yancyribbens commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2752254992)
> Of course, compared to the others it has the downside of not actually triggering in production builds, so it should be reserved for cases where (a) the code can safely continue when the assumption is violated (that's what's documented already)

This is a good call out, although I don't see it already documented (here), maybe I'm blind

The other reason I think this is a good call it is this excerpt from http://fiona.dmcs.p.lodz.pl/oopc/reference/en/cpp/language/attributes/assume.html.

>
...

💬 yancyribbens commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2012768210)
```suggestion
an expression inside `Assume` is true, it _will_ have no affect,
```

💬 yancyribbens commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2012768972)
s/affect/effect