💬 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

💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1610126595)
wondering if we should have a constant ala `TX_TRUC_VERSION` that is easily greppable? Exceptions being sprinkled across the codebase like this will be harder to track

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610209650)
That said, it looks like FreeBSD 12 is pretty much unusable: https://forums.freebsd.org/threads/freebsd-12-2-stable-pkg-update-failed.92034/#post-640223

💬 t-bast commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2125127269)
Concept ACK on restricting to 10kvB.

Lightning will need an upgrade path for existing channels to start using TRUC txs, and we're working on several options to upgrade existing channels. We will make sure the upgrade mechanism lowers the maximum number of HTLCs to fit inside the 10kvB limit.

💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2125128514)
I think it's not really worth worrying about benchmarks here, I think it's pretty clear that moving can only be better.

I'd like to get this in if there are no objections, as there's some more low-hanging fruit wrt UniValue moves.

💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610270917)
Thanks for all the support and patient. Applying them. The `{}` was also my first try but I got:
`error: initializer list cannot be used on the right hand side of operator ':'`
Now I changed it to
```
if (m_context->connman)
return m_context->connman->getLocalAddresses();
else
return {};
```
to prevent that ugly `empty` variable of mine.

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2125173212)
> Another mismatch, this time on riscv64 -_-

I've just rebuilt on 2 different machines (one aarch64 one x86_64), and they match your output here (removing my comment above):
```bash
00becde2dd12878e3b9f50f27899a6a8b752343dade7c71781632715c3001473 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/SHA256SUMS.part
a685b9cee54014e74639be1e8db2d55b7c008fdb3b31c1c708c364a49b56759a guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/bitcoin-e8c25e8a35e3-aarch64-linux-gnu-debug.tar.gz
d612281584098
...

💬 achow101 commented on pull request "depends: Fetch miniupnpc sources from an alternative website":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2125175024)
ACK 21b8a14d37c19ce292d5529597e0d52338db48a9

The main page of tuxfamily.org has 2 posts stating that they've had weeks of downtime in the past 6 months, so I think it makes sense to switch to a different mirror as they seem to be having issues. It's unfortunate that miniupnp.free.fr is http only, but we check the hash so I think it's fine.

💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610274712)
This is more beautiful and understandable. Replaced the `it->first` & `it->second` with `addr` & `info`:

```
for (const auto& [addr, info] : hosts) {
local_addresses += QString::fromStdString(addr.ToStringAddr());
if (!addr.IsPrivacyNet()) local_addresses += ":" + QString::number(info.nPort);
local_addresses += ", ";
}
local_addresses.chop(2); // remove last ", "
```

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610286827)
If so i would prefer not supporting it at all for older FreeBSD. It's so rare compared to the other operating systems already, and most of the userbase will be setting their own firewall. But we indeed need to check that it doesn't cause a compilation error.