💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679690571)
> But from what I can tell, we're not actually adding them to `sha256.cpp`? So... is this currently using an un-optimized sha2?

Please note the `PUBLIC` keyword, which propagates definition as a usage requirement:
https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L125

and

https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L128

where that usage requirement is actually appl
...

🤔 sipa reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2180269609)
ACK 21090d032f5c4d90a41c3ca2030690c75899ec1a

I have a few non-blocking suggested improvements in the new commit. I have also pushed a sanity check commit in https://github.com/sipa/bitcoin/commits/pr_28280, feel free to cherry-pick or squash in.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679414088)
In commit "coins: pass linked list of flagged entries to BatchWrite"

Can you add a one-paragraph doxygen description of what this type is for?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679416368)
In commit "coins: pass linked list of flagged entries to BatchWrite"

Can you add `LIFETIMEBOUND` to the `usage`, `sentinel`, and `map` arguments? That may make the compiler complain if the `CoinsViewCacheCursor` object is used after the arguments go out of scope.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679484508)
In commit "coins: pass linked list of flagged entries to BatchWrite"

The behavior change is a bit confusing here, as `cursor.WillErase(*it)` will be true for spent items, which was not true for the old code's `erase`. Would it make sense to split this commit up in two; one part which introduces the cursor and only iterate over flagged entry, and then a second part which moves the deleting of spent entries from the separate loop in `CCoinsViewCache::Sync` call into the `CCoinsViewCache::BatchW
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679435737)
In commit "coins: pass linked list of flagged entries to BatchWrite"

I'd prefer a comment that explains what the flag does rather than just when to set it. For example "If will_erase is not set, iterating through the cursor will erase spent coins from the map, and other coins will be unflagged (removing them from the linked list). If will_erase is set, the underlying map will not be modified, as the caller is expected to wipe the entire map anyway."

📝 alfonsoromanz opened a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455)
Adding tests in `./test/functional/wallet_assumeutxo.py` to cover the following scenarios:
- test import descriptors while background sync is in progress
- test loading a wallet (backup) on a pruned node

💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679703168)
> But from what I can tell, we're not actually adding them to sha256.cpp? So... is this currently using an un-optimized sha2?

Please note the PUBLIC keyword, which propagates the definition as a usage requirement:
https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L125

and

https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L128

where that usage requirement is actually appl
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679709347)
Ok, I am fine with just adding a comment and leave the code as is. This is probably too edge case to bother complicating the code with it, but just mentioning why I think it is better to keep using the initially assigned port even if it is different than the one we requested:

* We request port `8333`, but for whatever reason the router assigns `15000` with an external address let's say `1.2.3.4`.
* We start using and advertise `1.2.3.4:15000` to other peers, it propagates into nodes' addrman
...

💬 maflcko commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679712159)
Why not bookworm? https://packages.debian.org/bookworm/python3 (3.11)?

💬 fanquake commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679712998)
Looks like these don't allow overriding/take into account user flags? i.e If I configure with `-DCMAKE_CXX_FLAGS_RELEASE="-march=armv8-a+nocrypto"`, I'd expect the ARM SHA-NI check to fail, but it doesn't. Also doesn't seem to work with `APPEND_CXXFLAGS`, which should always be (globally) taken into account?

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679713040)
Yes, missed updates should result in us advertising the wrong (old) address only until the next renewal.

⚠️ instagibbs opened an issue: "getaddressinfo: complains missing `isscript` when called on unknown witness version"
(https://github.com/bitcoin/bitcoin/issues/30456)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

*** test_framework.authproxy.JSONRPCException: Internal bug detected: RPC call "getaddressinfo" returned incorrect type:
{
"isscript": "key missing, despite not being optional in doc"
}

f.e., when called with `bcrt1pfeesnyr2tx` on regtest

### Expected behaviour

I expect it to not return an internal bug

### Steps to reproduce

getaddressinfo "bcrt1pfeesnyr2tx"

with a wallet
...

✅ fanquake closed an issue: "intermittent issue in feature_reindex.py: Reindex fails to continue after restart; `initload` thread hangs forever"
(https://github.com/bitcoin/bitcoin/issues/30424)

✅ fanquake closed an issue: "Shutdown during reindex-chainstate can block forever"
(https://github.com/bitcoin/bitcoin/issues/23234)

🚀 fanquake merged a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435)

💬 maflcko commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1679727768)
```suggestion
def test_descriptor_import(self, node, wallet_name, key, timestamp, expected_error_message=None):
```

`should_succeed == not expected_error_message`

💬 maflcko commented on issue "getaddressinfo: complains missing `isscript` when called on unknown witness version":
(https://github.com/bitcoin/bitcoin/issues/30456#issuecomment-2231372884)
Unrelated note: Fuzz coverage is zero, so it seems like adding coverage here could turn up other bugs.

Ref: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/rpc/addresses.cpp.gcov.html

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2231387636)
> Getting a mismatch again. This time for block 716120 (00000000000000000009bc9db59a266e367f54fdd4cda7b5ddb31f6ae9723083).

We had an issue with that very block before as well. https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2076895408
Is it possible that you reverted the fix for that issue somehow? The tweaks seem to be the same as back then.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2231392644)
This is quite weird. @josibake added a test case for this: https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079095313