💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1222723313)
in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06:

prefix with `DEFAULT_`, make doxygen compatible, use braced initialization. Documentation about where the option can be used belongs where that code lives, not here.

```suggestion
/** Whether we allow importing a fee_estimates file older than MAX_FILE_AGE. */
static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false};
```

💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1222742962)
in dea3accb1f2eeef74f293262c168d68c0ec444cb

Given that you comment "there are no blocks" you should probably also assert that is true, e.g. `assert_equal` the result from `getbestblockhash` before and after

🤔 glozow reviewed a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1469429394)
Looks pretty good, thanks for adding the option.
Please clean up the commit messages. dea3accb1f2eeef74f293262c168d68c0ec444cb claims to add the regtest option, but that is done in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06.

💬 MarcoFalke commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1582276308)
Another failure on a doc change: https://cirrus-ci.com/task/5213373952950272?logs=ci#L1404


```
CXX qt/libbitcoinqt_a-qrc_bitcoin.o
CXX qt/libbitcoinqt_a-qrc_bitcoin_locale.o
AR libbitcoin_node.a
AR libbitcoin_util.a
AR libbitcoin_wallet.a
AR libbitcoin_zmq.a
AR libbitcoin_cli.a
AR libbitcoin_common.a
AR libbitcoin_consensus.a
CXXLD crypto/libbitcoin_crypto_base.la
CXXLD crypto/libbitcoin_crypto_arm_sh
...

💬 dergoegge commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1582306388)
> can you show an example to back up this claim: "I have only ever seen this task pass (even if all others fail)..."

@jarolrod I can't find the PR this happened on and I've only seen that one time but I swear I'm not making this up. The macOS task has other build flags, so it must have been some weird combination of that and tests/lint failing but some builds succeeding.

👍 dergoegge approved a pull request: "net, refactor: net_processing, add `ProcessCompactBlockTxns`"
(https://github.com/bitcoin/bitcoin/pull/26969#pullrequestreview-1469548500)
Code review ACK 686629d2be5545ef59cf0e97f4f3a74c6cde2efa

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

💬 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