💬 ishaanam commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1195808323)
nit:
```suggestion
/** Updates wallet birth time if 'new_birth_time' is lower than m_birth_time*/
```

💬 ishaanam commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1195809999)
Why is this check also done here if it is already done in `FirstKeyTimeChanged`?

💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195879972)
I mostly renamed it to make sure there'd be compiler errors anywhere that it didn't get updated, just in case some type conversion happened automatically.

💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195881028)
Yeah, this is broken as-is. My original idea was to add a timestamp to the `mapRelay` entries too, then it was to drop `mapRelay` and add `vExtraTxnForCompact`-relay, then I compromised by just doing it wrong...

💬 MarcoFalke commented on pull request "Descriptor unit tests and simplifications":
(https://github.com/bitcoin/bitcoin/pull/24361#issuecomment-1550798295)
> Drafted for now given it's (maybe) waiting on https://github.com/bitcoin/bitcoin/pull/26076.

That one was merged last week.

💬 MarcoFalke commented on issue "Mac osx 12.6.5 ":
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1550799971)
Please check your debug.log for possible causes; Alternatively you can upload it here.

You can find the debug.log in your [data dir](https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#data-directory-location).

Please be aware that the debug log might contain personally identifying information.

💬 MarcoFalke commented on issue "Can't compile v24.0.1":
(https://github.com/bitcoin/bitcoin/issues/27680#issuecomment-1550801604)
Closing for now. Let us know if you have any other questions.

✅ MarcoFalke closed an issue: "Can't compile v24.0.1"
(https://github.com/bitcoin/bitcoin/issues/27680)

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1196058973)
nit: for the next ~~node~~test

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1550888614)
Code review ACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5

I plan to test this manually a bit later

💬 hebasto commented on issue "Mac osx 12.6.5 ":
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1550921209)
FWIW, Bitcoin Core v22.0 has reached its EOL (see https://bitcoincore.org/en/lifecycle/).

Suggesting to [download](https://bitcoincore.org/en/download/) a new supported version.

📝 MarcoFalke opened a pull request: " build: Bump minimum Clang to clang-10 "
(https://github.com/bitcoin/bitcoin/pull/27682)
It doesn't make sense to support a minimum clang version that is difficult to install on all supported operating systems, which generally ship a later version:

* Ubuntu Focal 20.04: https://packages.ubuntu.com/focal/clang-10 and https://packages.ubuntu.com/focal/clang-12
* Debian Bullseye: https://packages.debian.org/bullseye/clang-11
* CentOS 8 Stream: All Clang versions from 11.0 to 15.0

Also, it allows to drop build code, which means it won't waste review when rolling over into cmake
...

💬 hebasto commented on pull request "[WIP] rebase: Call ProcessNewBlock() asynchronously":
(https://github.com/bitcoin/bitcoin/pull/18963#issuecomment-1550945493)
From my research it follows that the "[Move BlockChecked to a background thread](https://github.com/bitcoin/bitcoin/pull/18963/commits/2ed0b647435b80946f7f000d573ff4b4e274ca70)" commit fixes a lock-order-inversion in the MessageHandler thread.

See https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514928912.

💬 hebasto commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1550947349)
> Steps to reproduce on a fresh install of Fedora 38:

[Can](https://github.com/bitcoin/bitcoin/pull/18963#issuecomment-1550945493) be fixed by https://github.com/bitcoin/bitcoin/pull/18963.

💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1550948362)
I guess a simpler way to phrase it is: what can go wrong if c14ae132c5a5204a9a755c84c6de05fb30459221 picks the incorrect chainstate for a block? What are all the different ways in which we can receive a block? What do we expect to happen in each of these cases? Tests would be a really nice way to illustrate that, but are apparently a pain to write for net_processing.

💬 hebasto commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#issuecomment-1550949496)
> Also, it allows to drop build code, which means it won't waste review when rolling over into cmake (`cmake/module/CheckStdFilesystem.cmake`).

Concept ACK.

💬 hebasto commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196123867)
The only left usage of `SAVED_LIBS` can de dropped as well:
```diff
--- a/build-aux/m4/l_filesystem.m4
+++ b/build-aux/m4/l_filesystem.m4
@@ -25,8 +25,7 @@ AC_DEFUN([CHECK_FILESYSTEM], [
AC_MSG_RESULT([yes])
],[
AC_MSG_RESULT([no])
- SAVED_LIBS="$LIBS"
- LIBS="$SAVED_LIBS -lstdc++fs"
+ LIBS="$LIBS -lstdc++fs"
AC_MSG_CHECKING([whether std::filesystem needs -lstdc++fs])
AC_LINK_IFELSE([AC_LANG_SOURCE([_CHECK_FILESYSTEM_testbody])],[

...

📝 MarcoFalke converted_to_draft a pull request: "build: Bump minimum supported GCC to g++-9"
(https://github.com/bitcoin/bitcoin/pull/27662)
It is a bit frustrating to write valid C++ code only to realize that g++-8 fails to parse it later on.

The only non-EOL operating system still shipping with g++-8 is CentOS Stream 8. I think it is reasonable for users of affected Linux distributions to:

* Upgrade their operating system, or compiler to a supported version.
* Alternatively, stay with a previous release of Bitcoin Core as long as it is supported.

Fixes https://github.com/bitcoin/bitcoin/issues/27537

💬 Sjors commented on issue "Use muhash for assumeUTXO snapshot ":
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1550964299)
For the purpose of verifying a consensus change the ordering doesn't matter. The end user of the snapshot would benefit from a sha256 hash in order to not waste time verifying incorrect snapshots (assuming that sha256 is quicker than muhash here).

A file checksum doesn't need to be in the consensus code, though if we offer an automatic download it may need to be somewhere in the code. The right file checksum may depend on the specific distribution mechanism, e.g. if you use a torrent you'd us
...

💬 MarcoFalke commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196131293)
Ok, will do on the next push. Otherwise it will go away when the file is translated to cmake or when GCC is bumped to g++-9, whichever happens earlier.