💬 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

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-2231396250)
I think you lost https://github.com/bitcoin/bitcoin/pull/30046/commits/e11d764a6ce72cd571ad9cf818c6918dd16b02f6 in the rebase?

💬 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_r1679768600)
I'm actually having trouble figuring out where this stuff is exposed since we wouldn't be issuing these addresses as a wallet. Mind if I punt on this?

📝 maflcko opened a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457)
`isscript` is unknown for unknown witness versions, so it should be marked optional in the docs

⚠️ maflcko opened an issue: "Fuzz coverage for wallet RPCs is zero"
(https://github.com/bitcoin/bitcoin/issues/30458)
Fuzz coverage for wallet RPCs is zero, however it could help turn up bugs such as https://github.com/bitcoin/bitcoin/issues/30456.

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

_Originally posted by @maflcko in https://github.com/bitcoin/bitcoin/issues/30456#issuecomment-2231372884_

💬 maflcko commented on issue "Fuzz coverage for wallet RPCs is zero":
(https://github.com/bitcoin/bitcoin/issues/30458#issuecomment-2231427446)
I presume this is possible to implement by DRYing some code from the `rpc` fuzz target into a `wallet_rpc` fuzz target.