💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2124935777)
> I've noticed two issues in the "tidy" job so far:
>
> 1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
>
> 2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.

The recent push includes:
- commits from https://github.com/hebasto/bitcoin/pull/205, which fixes the point 1
- commits from https://github.com/hebasto/bitcoin/pull/206, which fixes the point 2

🤔 furszy reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2071420549)
Thanks for the review ryanofsky!

> I just noticed [bitcoin/bitcoin#25269](https://github.com/bitcoin/bitcoin/pull/25269), which I hadn't seen when I posted my comment above. It looks like that PR was closed for inactivity but it does look like a good approach that solves the issue I didn't like in my comment. (Though maybe that PR is still a slight regression because it doesn't include the amount of the fee in the error message.) I wonder if you'd consider reopening that PR and maybe includin
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1610124181)
Added in b2322fc476dc983b91492768d2054c2a3966afdd

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1610124223)
Thanks, fixed in 0f4ddfa43749266960a5d05693ed0cdc993328d6.

💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1610144238)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" 2e29e9beeebcadfc7afc56a28791456d40551bed: I think `MAX_SET_SIZE` could be in `txreconciliation.h` then we can use it in the tests.

```diff
-/**
- * Maximum number of wtxids stored in a peer local set, bounded to protect the memory use of
- * reconciliation sets and short ids mappings, and CPU used for sketch computation.
- */
-constexpr size_t MAX_SET_SIZE = 3000;

/**
* Maximum number of transactions for which we
...

🤔 furszy reviewed a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2071488793)
Code review ACK 078c2d74c2779dd459e42daf67ab450fbf223506. Nothing blocking, just tiny nits.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1610134627)
Non-blocking because it is the only way now but it is kinda unnatural to provide `batch` and `w_batch` when `w_batch` contains the `batch` object.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1610135840)
nit: should check `WriteKeyMetadata` return value.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1610125269)
nit: `!updated_metas.empty()`

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610151853)
Presumably this only exists in (the rather recent) FreeBSD >= 13.2? For older versions we need to use the same method as macOS? If so, then we probably need to check this stuff in configure.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1610159034)
I've been talking to @sipa about this recently and I think the limit may not be necessary if the approach for when to compute the reconciliation sets is changed. I'm may get rid of it, but if not, I'll take this

🤔 furszy reviewed a pull request: "Showing Local Addresses in Node Window"
(https://github.com/bitcoin-core/gui/pull/626#pullrequestreview-2071533836)
thanks for the encapsulation jadijadi.

💬 furszy commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610153233)
wrong description

💬 furszy commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610158134)
```suggestion
return m_context->connman ? m_context->connman->getLocalAddresses() : {};
```

💬 furszy commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610174769)
Maybe?
```c++
QString local_addresses;
std::map<CNetAddr, LocalServiceInfo> hosts = clientModel->getLocalAddresses();
for (const auto& [addr, info] : hosts) {
local_addresses += QString::fromStdString(it->first.ToStringAddr());
if (!addr.IsPrivacyNet()) local_addresses += ":" + QString::number(it->second.nPort);
local_addresses += ", ";
}
local_addresses.chop(2); // remove last ", "
```

💬 theStack commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610179097)
Kind of unrelated, and more like a "I wish my c++ skills were better" nit: I wonder if there is a way to do the introduced sanity checks already at compile-time, since these flags are all constant for each of the `ATMPArgs` instances (probably not, or at least not without significant changes in structure...).

👍 theStack approved a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2071579370)
re-ACK a84b57462cd3dfcd059783696aacd796ef1793d4 📦

📝 mzumsande opened a pull request: "validation: Make ReplayBlocks interruptible"
(https://github.com/bitcoin/bitcoin/pull/30155)
This addresses the problem from #11600 that the Rolling Forward process after a crash (`ReplayBlocks()`) is uninterruptible. `ReplayBlocks` can take a long time to finish, and this can be especially annoying to GUI users who are taunted to "press q to shutdown" even though pressing "q" does nothing.

Now, when an interrupt is received during `ReplayBlocks()`, the intermediate progress is saved:
Apart from writing the updated coins to disk, the flush adjusts the lower head block (saved in `DB_
...

💬 mzumsande commented on issue ""Rolling forward" at startup can take a long time, and is not interruptible":
(https://github.com/bitcoin/bitcoin/issues/11600#issuecomment-2125056850)
> The next step here is to make it interruptable.

See #30155 for that.

👍 instagibbs approved a pull request: "policy: restrict all TRUC (v3) transactions to 10kvB"
(https://github.com/bitcoin/bitcoin/pull/29873#pullrequestreview-2071490886)
ACK 154b2b2296edccb5ed24e829798dacb6195edc11

I think a concept re-ack from protocol devs outside this repo is needed for merge