💬 MarcoFalke commented on pull request "refactor: Split logging utilities from system.h":
(https://github.com/bitcoin/bitcoin/pull/27238#issuecomment-1466487829)
re-ACK aaced5633b81b2f08b993106a527e2a0e1d663c1 🐍

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK aaced5633b81b2f08b99
...

💬 instagibbs commented on pull request "[POLICY] Ephemeral anchors":
(https://github.com/bitcoin/bitcoin/pull/26403#discussion_r1134315587)
suggested in other PR: "switch the ordering? else if (whichType != ANCHOR && IsDust())"

✅ glozow closed an issue: "Error messages for invalid address"
(https://github.com/bitcoin/bitcoin/issues/21741)

🚀 glozow merged a pull request: "Improve address decoding errors"
(https://github.com/bitcoin/bitcoin/pull/26514)

👍 hernanmarino approved a pull request: "Mask values on Transactions View"
(https://github.com/bitcoin-core/gui/pull/708)
ACK 4492de1be11f4131811f504a037342c78f480e20

🚀 glozow merged a pull request: "Avoid integer overflow in CheckDiskSpace"
(https://github.com/bitcoin/bitcoin/pull/27235)

💬 achow101 commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#issuecomment-1466603230)
ACK 4492de1be11f4131811f504a037342c78f480e20

💬 mzumsande commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1134455743)
I don't think so - even with the guarantee that `GetGroup()` is different, there is still a small chance that two addresses happen to hash to the same bucket in `GetNewBucket()`.

👋 pinheadmz's pull request is ready for review: "Add wallet method to detect if a key is "active""
(https://github.com/bitcoin/bitcoin/pull/27216)

💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1134513177)
Oh yes, `walletlock` never gets called here. However, implementing this change results in another `CannotSendRequest`.

💬 hernanmarino commented on issue "Confusing/misleading "Dust:" label in coin selection dialog":
(https://github.com/bitcoin-core/gui/issues/699#issuecomment-1466831109)
+1 on removing it

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1466870416)
> Hmm, this is maybe ok. Another approach would be to try to open to e.g. Tor while still having < 8 outbound connections. To counter the issue of staying with < 8 connections and not being able to open to Tor because e.g. the Tor proxy is broken, then we could give up after a few attempts and then fall back to the old way of filling with random (aka IPv4 ;-)) addresses. If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How m
...

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1466872077)
> > Why it doesn't make sense? I think it is an extra effort to obtain IPv4 addresses if one has IPv6 addresses and the other way around. Why impossible in some network environments? After all you know if you are going to attempt to connect to 5.6.7.8 or 2030:c0fe:....
>
> I also not sure if I agree with counting IPv4 and IPv6 as one network and I think this could need a bit more reasoning. Is it because they are both clearnet? Is it a workaround to don't have 8 protected peers if all network
...

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1134606456)
Thanks, that’s unexpected. I would also expect that `CalculateTotalBumpFees(…)` always is equal or smaller to the sum of individual bump fees. I’ll check it out.

💬 willcl-ark commented on issue "untrust vs unsafe ?":
(https://github.com/bitcoin/bitcoin/issues/18625#issuecomment-1466951596)
I think a breaking change to the RPC argument keys, seeing as we accept named arguments via rpc, would likely not be considered worth it just to harmonise vocabulary.

@achow101 I'd be curious of your thoughts here? If we aren't going to change this then perhaps we can close the issue.

💬 ryanofsky commented on pull request "wallet: fully migrate address book entries for watchonly/solvable wallets":
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114)
In commit "wallet: fully migrate address book entries for watchonly/solvable wallets" (d5f4ae7fac0bceb0c9ad939b9a4fbdb85da0bf95)

It seems like a bug to not write the label when then label is empty, because will make receiving address with empty labels labels appear to be change addresses. If the label isn't written `CAddressBookData::SetLabel` will not be called:

https://github.com/bitcoin/bitcoin/blob/f50fb178c30ea4bec0b369af3d15cab08d33396f/src/wallet/wallet.h#L219-L222

so `CAddressBo
...

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1466964818)
> Could also add a test framework option to whitelist peers. So we don't have to add -whitelist flag everywhere.

Sounds good! Gonna implement it!

💬 furszy commented on pull request "wallet: fully migrate address book entries for watchonly/solvable wallets":
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134628591)
Check #26836, bug should be fixed there. Have re-written the process there without the `persist_address_book` calls.

💬 ryanofsky commented on pull request "wallet: Migrate legacy wallets to descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/19602#discussion_r1134633292)
In commit "Implement MigrateLegacyToDescriptor" (0bf7b38bff422e7413bcd3dc0abe2568dd918ddc)

It seems like this should say `IsMine` not `!IsMine`. Also I'm not sure why `addr_pair.second.purpose == "receive"` check is necessary. It seems like just checking IsMine should be enough, and it would be less robust to rely on purpose field being present.

💬 ryanofsky commented on pull request "wallet: fully migrate address book entries for watchonly/solvable wallets":
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134636537)
Thanks will check it out. Also would be good to have a test for the bug.