💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679665692)
It's the instruction to push next two bytes on the stack

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679666497)
should have coverage, please check it fails :)

💬 sipa commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-2231284275)
utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b

💬 theuni commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679674217)
In commit ff368465a66298487a88d8da575f5251d094d687

Could you remind me what's going on here, please? I assume these aren't needed because we're meant to be adding compile definitions to specific libs. But from what I can tell, we're not actually adding them to `sha256.cpp`? So... is this currently using an un-optimized sha2?

Generally I'd feel more comfortable turning these into opt-outs, I think, to avoid accidentally building without optims.

For the sake of review/sanity, I think for
...

💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2231294972)
> maflcko's suggestion wouldn't remove the dependency. Unless you place the new enum somewhere else?. Which I don't think it worth it.

Yes, I was thinking of putting it in a file for types which we have been doing in the code base in several places. And @ryanofsky suggests introducing a `kernel/types.h` [here](https://github.com/bitcoin/bitcoin/pull/30214/commits/97cb62492903b188074de5c27b3dcba2c9b768ff) already. Moving code isn't hard to review and `chain.h` is big enough to be broken up a b
...

👍 BrandonOdiwuor approved a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2180709184)
Code Review ACK 5fd48360198d2ac49e43b24cc1469557b03567b8

💬 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)