💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1222705608)
Done. Now it is the same as in `process_message.cpp`.

It seems inefficient to me - if `LIMIT_TO_MESSAGE_TYPE` is set to e.g. `filterclear` the fuzzer has to brute force all possible strings with length 11 to find it? That is 2⁸⁸ possibilities.

🤔 glozow reviewed a pull request: "ci: Nuke Android APK task, Use credits for tsan"
(https://github.com/bitcoin/bitcoin/pull/27834#pullrequestreview-1469402645)
ACK fa22538e481fa2c4f0b5d6f91166335e60b67fe9
Agree with removing this task (which doesn't run any tests) to make room for a more useful task (which does run tests).

💬 MarcoFalke commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1582213817)
I think it should be fine to re-add this the next time it finds an issue, or when the (unit) tests are run, or some other reason appears to re-add it.

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1222713396)
A modern fuzz engine will read the `LIMIT_TO_MESSAGE_TYPE` and inject it into the fuzz input, so it shouldn't take more than a few seconds to guess. In any case it shouldn't take 2^88 tries

💬 MarcoFalke commented on pull request "ci: Invalidate Cirrus CI docker cache":
(https://github.com/bitcoin/bitcoin/pull/27838#issuecomment-1582219284)
I think it is fine to merge this? Seems like it fixed the issue and there is no risk or downside, right?

💬 achow101 commented on pull request "ci: Invalidate Cirrus CI docker cache":
(https://github.com/bitcoin/bitcoin/pull/27838#issuecomment-1582231514)
ACK fac7f4ab5e93e2cd1469dfcda215c7e20d9fe1fb

CI seems to be working here

🚀 achow101 merged a pull request: "ci: Invalidate Cirrus CI docker cache"
(https://github.com/bitcoin/bitcoin/pull/27838)

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1222736555)
We won't be able to get the private key since we don't know the descriptor id in order to do the lookup.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1222737605)
I don't think that's necessary as wallet such as ones without private keys or those with only imports won't have a global hd key.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1222738220)
This follows the same pattern for the other upgrading methods.

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

this should be const

💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1222736738)
in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06
Please also include here (1) the default value (2) how old "stale" is specifically, using the constants.

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

maybe replace this `hour` and the one in the other tests with a constant at the top of the file, `SECONDS_PER_HOUR`

💬 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