💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468790150)
This is indeed clearer even if it introduces another copy.

💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468803070)
> don't we have unsynchronized reads/writes?

Yes, we do, but with the current code I am not sure if that is easily solvable. My preferred route here is we get these changes in, and then adapt our internal logging code to better cater to the kernel usages. There are other problems with it too: We can't distinguish between different logging sources and the global is potentially leaky on exit.

✅ antonilol closed an issue: "add ability to remove imported public keys/addresses (or even privkeys)"
(https://github.com/bitcoin/bitcoin/issues/23765)

🚀 fanquake merged a pull request: "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2"
(https://github.com/bitcoin/bitcoin/pull/33185)

🤔 cedwies reviewed a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/33680#pullrequestreview-3387641073)
Approach ACK

Do `ForceFlushToDisk(...)`/`FlushToDisk(...)` require `cs_main` to be held?
Most call sites hold it during shutdown flushes, but the function itself doesn’t assert or document that (no `AssertLockHeld(cs_main)`). If this is correct, I think a short comment can help that a Lock is not strictly needed.

💬 cedwies commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2468910777)
Isn't `LogPrintf` deprecated? Could we replace this with `LogError` or `LogWarning`? Maybe also including the flush mode can be helpful.

💬 cedwies commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2468702094)
Why are we wiping the cache here? We only compute stats from `CoinsDB()`, not the in-memory cache, so a non-wiping flush (`wipe_cache=false`) should be enough for consistency (?)

💬 naiyoma commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2469112566)
I have tried to make this work, but first I'd have to change `valid_witness_malleate_tx` to also return the parent
`return parent, child_one, child_two
`

and then 👇🏽

```

siblings_parent, sibling1, sibling2 = valid_witness_malleate_tx(wallet, tx_originator)
self.log.info(f" - sibling1: txid={sibling1.txid_hex}, wtxid={sibling1.wtxid_hex}")
self.log.info(f" - sibling2: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}")
import pdb; pdb.set_tr
...

👍 willcl-ark approved a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3388161946)
ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d

I like the approach here of enforcing as-we-go through the codebase. Something similar was done recently in `nixpkgs` [with their `treefmt` formatter](https://youtu.be/3m_cvSuOB-Y?si=GuuU3aC7IR_FKB6q).

Tested that this worked locally by modifying the test script and running.

<details>
<summary>Details</summary>

```bash
#!/usr/bin/env bash
set -x

BASE_ROOT_DIR="$HOME/src/core/bitcoin"
BASE_BUILD_DIR="$BASE_ROOT_DIR/build"
FILES_WITH
...

💬 naiyoma commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2469254718)
this isn't necessary anymore, i'm using `wallet.send_to()` to create parent transaction

🚀 hebasto merged a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308)

📝 naiyoma converted_to_draft a pull request: "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value"
(https://github.com/bitcoin/bitcoin/pull/33498)
TLDR: When a node is connected to multiple networks (e.g., clearnet and Tor), it keeps separate ADDR response caches for each network. These are refreshed about once per day (randomized between 21 and 27 hours). See →[ https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L3519](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L3519)
This is an example of a dual-homed node’s addrman response, with separate caches:

_IPv4 response:_
```
{102.130.242.11:8333 : 1741100851}
{119
...

💬 hebasto commented on pull request "ci: Treat IWYU violations as errors":
(https://github.com/bitcoin/bitcoin/pull/26763#issuecomment-3456104646)
See https://github.com/bitcoin/bitcoin/pull/31308.

👍 ryanofsky approved a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3388413169)
Code review ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3, just tweaking semantics slightly since last review so if an `interruptWait` call is made shortly after a `waitNext` call it will reliably cause the `waitNext` call to return right away without blocking, even if the `waitNext` call had not begun to execute or wait yet.

💬 naiyoma commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3456123571)
Moved back to draft, working on addressing -> https://github.com/bitcoin/bitcoin/pull/33498#pullrequestreview-3319680730

🤔 rkrux reviewed a pull request: "wallet: Always rewrite tx records during migration"
(https://github.com/bitcoin/bitcoin/pull/32985#pullrequestreview-3388443761)
lgtm ACK af041c405756d3b8bb04cb2ebd8c32cf237ac2a9

This appears to be a preparatory commit for the latter PR #33034 where transaction is written in the SQLite transactions table as well during migration. This appears fine also because during wallet loading, the transaction is rewritten if it needs to be reordered based on an updated `nOrderPos`.

I don't suppose there is an easy way to test this in the `wallet_migration` functional test because the wallet is loaded right after migration is done
...

👋 fanquake's pull request is ready for review: "depends: sqlite 3.50.4; switch to autosetup"
(https://github.com/bitcoin/bitcoin/pull/32655)

💬 fanquake commented on pull request "depends: sqlite 3.50.4; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#issuecomment-3456168612)
Rebased for #33185 and dropped the [nomerge] `--with-pic` commit.

💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2469409178)
Good question. Probably better to split the requests into two that will each give <=4MB responses, which probably means sending two compact block descriptions for the overall template, rather than one.

👋 Raimo33's pull request is ready for review: "refactor: optimize: avoid allocations in script & policy verification"
(https://github.com/bitcoin/bitcoin/pull/33645)