💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1222817552)
Done

💬 dergoegge commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1582320665)
@theuni maybe the `\n` check for logging? Also suggested in https://github.com/bitcoin/bitcoin/issues/27825

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1582321920)
> My proposed fix is to disallow upgrade procedure for wallets created with master by setting `WALLET_FLAG_GLOBAL_HD_KEY` for all newborn wallets. Later we want to add a separate method to change HD key anyway to cover that use case.

I thought this was doing this already, but it turns out that we don't set the descriptor flag until after the upgrade stuff is run for creation, so it wasn't being set. I've added a commit which will set the flag for all new wallets.

💬 MarcoFalke commented on pull request "Remove confusing "Dust" label from coincontrol dialog":
(https://github.com/bitcoin-core/gui/pull/719#issuecomment-1582357495)
Concept ACK

💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1222883957)
I've added a commit with this comment.

💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1222884969)
Done as suggested

💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1222885500)
It's tested in the new test

💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1222914050)
I've split it up so that tests are introduced in the commits that fix the issues.

💬 achow101 commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1222919617)
For wallets that are not in a wallet dir, the db's filename will be the name of the wallet itself.

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1222985986)
yes, that would be unnecessary

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1222989539)
Thank you, I updated it.

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1222991211)
I modify as you suggested 👍🏾

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1222991573)
Thank you

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1222991937)
sure fixed

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1582537371)
On treating IPv4 and IPv6 as one network - I am ok either way. I see the merit in amiti's considerations above, but also on @ajtowns's. One more thing to consider - for an attacker it is not too difficult to get IPv6 connectivity but that is also the case for Tor. I mean it is not too difficult to get Tor connectivity either, but that is still _some_ extra effort.

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1223007090)
Ok, I am just checking because elsewhere the intention is to treat clearnet as one network and this code does not do that. I do not have a strong opinion either way.

💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#issuecomment-1582565180)
tsan failure looks unrelated: https://cirrus-ci.com/task/5970681108627456

💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1223049862)
I think this should fail rather than being ignored?

💬 Sjors commented on pull request "doc: clarify full Xcode download is not needed":
(https://github.com/bitcoin/bitcoin/pull/27835#issuecomment-1582643067)
Some background, it should indeed not be necessary to install Xcode: https://www.freecodecamp.org/news/install-xcode-command-line-tools/

I improved the prose a bit.

💬 Sjors commented on pull request "doc: clarify full Xcode download is not needed":
(https://github.com/bitcoin/bitcoin/pull/27835#discussion_r1223097442)
macOS does not ship with all of Xcode, but they do ship with the `xcode-select` command.