💬 fanquake commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1161597309)
Yea. Fixed

💬 achow101 commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1501769224)
Concept ACK

Please squash your commits

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1161726047)
yes, that was something I touched while I was verifying the `req` class member.

💬 achow101 commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#issuecomment-1501822228)
ACK 68eed5df8656bed1be6526b014e58d3123102b03

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1161727508)
I'll change it. Thanks.

💬 achow101 commented on pull request "wallet: Avoid underpaying transaction fees when signing taproot spends":
(https://github.com/bitcoin/bitcoin/pull/23502#issuecomment-1501832645)
> How would this behave in the context of changeless transactions? Would we be overestimating the target and dropping an excess to the fees?

All overages due to size estimation differences are dropped to fees. The output values are set based upon the estimated size and they cannot be changed after signing. Once signing is completed and the final transaction created, if it ends up being smaller than estimated, whatever could have been saved will not be saved.

> Does that get recognized when
...

💬 achow101 commented on pull request "wallet: Avoid underpaying transaction fees when signing taproot spends":
(https://github.com/bitcoin/bitcoin/pull/23502#discussion_r1161737139)
I don't quite follow your question.

The estimated stack size is used to choose which result to return as multiple results could be valid. In normal signing, we use the smallest valid stack. In estimating, we use the largest.

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161754205)
I followed the indentation style used for the same `warnings` field in RPCs createmultisig, addmultisigaddress, importmulti, importdescriptors, etc., as yes, `clang-format` would change the whole function.

💬 achow101 commented on pull request "bumpfee: avoid making bumped transactions with too low fee when replacing outputs":
(https://github.com/bitcoin/bitcoin/pull/27308#discussion_r1161754652)
Done

💬 achow101 commented on pull request "bumpfee: avoid making bumped transactions with too low fee when replacing outputs":
(https://github.com/bitcoin/bitcoin/pull/27308#discussion_r1161754817)
Changed to `temp_mtx`

💬 achow101 commented on pull request "bumpfee: avoid making bumped transactions with too low fee when replacing outputs":
(https://github.com/bitcoin/bitcoin/pull/27308#discussion_r1161757888)
> If I understand this test right, it checks whether the absolute fee of a smaller replacement transaction sufficiently surpasses that of a larger original transaction.

No, it doesn't check any feerates since we can't see the candidate bump transaction. The calculations here are just for figuring out a feerate that would fail the modified checks and checking that `bumpfee` fails as expected.

> I looked around in `wallet_bumpfee.py`, and had the impression that we generally seem to assume t
...

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161770541)
Thanks for looking! I'm not sure whether compilers optimize for it but happy to drop that unrelated change, will do.

💬 ryanofsky commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1161771202)
> [8cd7730](https://github.com/bitcoin/bitcoin/commit/8cd77302305a2387368aabe8744b1859caf9956a): paging @ryanofsky: do you think we should move all this complexity here to the interface?

Right, probably it would be better to move the new code from the `wallet:WalletImpl::getAvailableBalance()` method in `src/wallet/interfaces.cpp` into the `wallet::GetAvailableBalance()` function in `src/wallet/spend.cpp` or into another function there like `wallet::GetSelectedCoinsBalance`. Reasoning is

-
...

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161777870)
Changed here and elsewhere

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161777986)
Done as suggested

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161778269)
Done as suggested

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161778333)
Done as suggested

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161787781)
What do you think about: `Multiple messages will each be delimited by a newline (LF) character.`

(`\n` is the escape sequence (for use in a string literal) and `0A` is the OS-specific hex encoded representation)

- https://en.cppreference.com/w/cpp/language/escape
- https://en.wikipedia.org/wiki/Newline
- https://www.ibm.com/docs/en/zos/2.4.0?topic=output-escape-sequences

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161800526)
`WALLET_FLAG_AVOID_REUSE` is defined in `walletutil.h`, not in `wallet.h`, and per `git grep "#include <wallet/wallet.h>" | wc -l` this commit would avoid needlessly adding `WALLET_FLAG_CAVEATS` to nearly fifty compilation units.

I'll add `#include <wallet/walletutil.h>` to `wallet/rpc/wallet.cpp` in this change.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161800943)
If they're no longer active, we don't know internalness.