💬 katesalazar commented on pull request "gui: add used balance to overview page":
(https://github.com/bitcoin-core/gui/pull/775#discussion_r1383801194)
https://bitcoin.stackexchange.com/a/38998/126793

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1795922777)
> style-nit: all variable and function names should be snake_case https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c.

Thanks for checking that, you are right, I'll fix them all ASAP.

💬 katesalazar commented on pull request "Wallet: Functions to enable adding used balance to GUI overview page":
(https://github.com/bitcoin/bitcoin/pull/28776#issuecomment-1795925965)
Concept ACK

💬 katesalazar commented on pull request "gui: add used balance to overview page":
(https://github.com/bitcoin-core/gui/pull/775#discussion_r1383815524)
> > Because the space is tiny, and the impact should be big, every word in an effective tooltip needs to pack a punch.
>
> Source: Internet

as the tooltip for the "used" line of the "balances" block,
"your current used balance" doesnt feel like a great tooltip

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1383818109)
It makes sense, I'll use `std::set::find` instead of `std::set::count` (L#1141).

Regarding C++20. it's [not enabled](https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L99) yet but perhaps will be soon (#28349). If this PR doesn't get merged first I'll update the code as you suggested so I'm leaving this conversation as unresolved, otherwise I'll do a follow-up.

💬 fanquake commented on issue "Wallet Missing Balances/Unspent":
(https://github.com/bitcoin/bitcoin/issues/28797#issuecomment-1795955354)
cc @achow101

💬 achow101 commented on issue "Wallet Missing Balances/Unspent":
(https://github.com/bitcoin/bitcoin/issues/28797#issuecomment-1796181526)
I think the only conditions for this to happen are either the output has been spent by something in the wallet that has not been broadcast yet, or if the wallet thinks that those outputs are not ismine. The latter shouldn't be possible given that `getaddressinfo` reports both addresses as ismine. Can you check if there is some transaction in your wallet that is spending that output but wasn't broadcast?

It could be that the output is locked with `lockunspent`, but that should only affect `lis
...

💬 achow101 commented on pull request "wallet: cache descriptor ID to avoid repeated descriptor string creation":
(https://github.com/bitcoin/bitcoin/pull/28799#issuecomment-1796284261)
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7

💬 ariard commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1796295871)
Hi @Daniel600

> HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)

For your clarity and wider transparency, to the best of my understanding, we don't have a decision process among the community about mempool policy changes affecting Bitcoin applications in a wide scale fashion, as `mempoolfullrbf` is I believe one of them. I think the best process we m
...

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1383924519)
done

💬 achow101 commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1383925133)
In 50368d92ff0f878af2d6a70773bdf6a6e29b25bb "ArgsManager: support subcommand specific options"

I found this name to be confusing since the word "subcommand" implies that the option itself is a subcommand (i.e. the same category as `COMMANDS`), rather than it being an option to a command. Would suggest renaming this to something like `COMMAND_OPT`.

✅ achow101 closed an issue: "test: wallet_miniscript.py fails with thread sanitizer "
(https://github.com/bitcoin/bitcoin/issues/28800)

🚀 achow101 merged a pull request: "wallet: cache descriptor ID to avoid repeated descriptor string creation"
(https://github.com/bitcoin/bitcoin/pull/28799)

💬 nerd2ninja commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1796429893)
> This major Bitcoin protocol change will most likely result in services wanting to offer 0-conf to

0-conf is bad design philosophy from the start. Simply running a custom client with a different transaction relay policy or paying miners out of band breaks it. However, a 0-conf design with much better security guarantees has been designed. They're called "MAD transactions" you can read about them here: https://coinexplorers.com/general/mad-transactions-mutual-assured-destruction-transactions-
...

💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1796506821)
> Is there a reason not to cover more of DescriptorScriptPubKeyMan in this PR? e.g. DescriptorScriptPubKeyMan::SignTransaction or DescriptorScriptPubKeyMan::FillPSBT.

Not exactly, perhaps one of them may be very slow, let me test more functions and then I can add them all here, np.

💬 brunoerg commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1796514184)
Concept ACK

💬 brunoerg commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1384011959)
In fde0193e687ad50a01a191e14fb6c052b3534bc1: perhaps using f-strings in all similar cases?

💬 brunoerg commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1384016700)
from https://github.com/sipa/asmap/pull/9/

```suggestion
print(
"# %i%s IPv4 addresses changed; %i%s IPv6 addresses changed"
% (
ipv4_changed,
"" if ipv4_changed == 0 else " (2^%.2f)" % math.log2(ipv4_changed),
ipv6_changed,
"" if ipv6_changed == 0 else " (2^%.2f)" % math.log2(ipv6_changed),
)
)
```

🤔 murchandamus reviewed a pull request: "validation: return more helpful results for reconsiderable fee failures and skipped transactions"
(https://github.com/bitcoin/bitcoin/pull/28785#pullrequestreview-1716255880)
crACK 0150e860863f95b18448e7b67f5db27017660670 with grain of salt:

I’m pretty new to reviewing mempool code:

- The introduction of an explicit class for `Wtxid` makes sense to me
- The introduction of `TX_RECONSIDERABLE` makes sense to me in the context of transactions being tested in the context of packages where they might be resubmitted with a higher resulting mining score
- It makes sense to me that some mempool acceptance tests would now fail with `TX_RECONSIDERABLE` and the ones th
...

💬 murchandamus commented on pull request "validation: return more helpful results for reconsiderable fee failures and skipped transactions":
(https://github.com/bitcoin/bitcoin/pull/28785#discussion_r1384000302)
After the name change, perhaps:

```suggestion
const bool is_reconsiderable{res.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE};
```