💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2590522916)
85cff48b782538ed5a1c980015dd01cb32986b66

straggling AncestorCandidateFinder reference

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2590327658)
> or equal

Need to understand why, delving seems to suggest the more "natural" choice https://delvingbitcoin.org/t/spanning-forest-cluster-linearization/1419 "If there is an inactive dependency, where the parent and child are in distinct chunks, and the child chunk has higher feerate than the parent chunk, make the dependency active, merging the two chunks."

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2599895617)
If you don't merge along equal-feerate dependencies, you can end up in a situation with two equal-feerate "chunks" that depend on each other.

E.g. the AC and BD "chunks" in:

```mermaid
graph BT;
A["A: 1/1"];
B["B: 1/1"];
C["C: 1/1"];
D["D: 1/1"];
C --> A;
C -.-> B;
D -.-> A;
D --> B;
```

The solution is an initial phase where you don't care about splitting equal-feerate parts from one another (this PR), and then a secondary phase where you split up chunks in their minimal comp
...

🤔 mzumsande reviewed a pull request: "test: p2p: check that peer's announced starting height is remembered"
(https://github.com/bitcoin/bitcoin/pull/33990#pullrequestreview-3553772985)
I agree that it isn't useful, and depreciation would makes sense in my opinion. If we decide not to do that for some reason, a test makes sense though.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2599927986)
Yeah, exactly.

* Pick a random chunk (from the list you maintain of chunks that need to be considered still)
* See if it can be merged upwards or downwards, and:
* If so, merge with its max-feerate-difference dependency/depender, and add the resulting chunk to list.
* If not, remove the chunk from the list.

💬 ajtowns commented on issue "Net split meta issue":
(https://github.com/bitcoin/bitcoin/issues/33958#issuecomment-3629082728)
> > For the last point, I think there are two main principles we should aim for: (a) minimising the movement and reparsing of data, and (b) minimising the locking and (potentially) maximising the cache performance when we send/receive data. Currently, when we send data and don't need to block, we:

> I'm all for these goals, but I consider almost all of those things to be premature optimizations compared to the real-world bottlenecks that we currently see.

The single biggest bottleneck I see to
...

👍 sedited approved a pull request: "[IBD] coins: reduce lookups in dbcache layer propagation"
(https://github.com/bitcoin/bitcoin/pull/33602#pullrequestreview-3554048589)
ACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41

💬 sedited commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2600124064)
Why not address these TODOs in this PR? Seems like all that needs to be done is pick the test changes from Andrew's commit?

💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2600248073)
We have tried fixing that a few times, a lot of tests need to be updated, see [`320482e` (#30673)](https://github.com/bitcoin/bitcoin/pull/30673/commits/320482e860cb6834984c0c52643021496e57a655#r2206103568) and it scared people and we didn't get reviews for that, so we're doing it in smaller chunks now.

The TODO makes sense here since it documents that the current approach isn't a pessimization, it favors the only case that can happen in reality.

💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#issuecomment-3629137740)
thanks for the reviews!
rfm?

💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2600302551)
I have been thinking about removing this call from `ABCStep` for a while, because it doesn't seem necessary:
During `ConnectTip()`, we call `InvalidBlockFound()`, which calls `InvalidChainFound()` for the block that fails validation. The additional call to `InvalidChainFound()` with the highest connectable block from the chain (which could be a successor of invalid block, or the same block again) can lead to additional unnecessary work and duplicate logging. The only part from `InvalidChainFoun
...

💬 Satoshin-Btc commented on issue "Net split meta issue":
(https://github.com/bitcoin/bitcoin/issues/33958#issuecomment-3629209877)
so i agree with the things you pointed out. the rushed merger was a complete mess the last time in 2022 and didn't bode well either. i get the node and process thread issue for validated peers for txs blocks and so forth. my only thought on solution is something similar to the net split but that causes its own issues. so with dedicated nodes back to where the we're before allowing calls from the different containers helped the process a lot more than the jumbled up system we are dealing with. th
...

💬 sedited commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#issuecomment-3629213020)
Updated beff6ff964c5c1b4a96af89daf6b2ddaede3464a -> 8ca9997e48bb2067ea83fabb1c640af72178f97c ([rmMinPowChecked_0](https://github.com/TheCharlatan/bitcoin/tree/rmMinPowChecked_0) -> [rmMinPowChecked_1](https://github.com/TheCharlatan/bitcoin/tree/rmMinPowChecked_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/rmMinPowChecked_0..rmMinPowChecked_1))

* Addressed [comment](https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2599070189), and [comment](https://github.com/bitcoin/
...

💬 l0rinc commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#issuecomment-3629227687)
reACK 8ca9997e48bb2067ea83fabb1c640af72178f97c

only doc updates since last ACK

👍 stickies-v approved a pull request: "log: don't rate-limit "new peer" with -debug=net"
(https://github.com/bitcoin/bitcoin/pull/34008#pullrequestreview-3554269912)
ACK 8f553ac9df91c03a6e753cce3e7cf2e46b977bdb

Wholesale downgrading inbound connections to `LogDebug` seems sensible to me. Even with ratelimiting, we should still make sure that unconditional logs can't easily be triggered by an attacker.

I think this would benefit from a brief release note. People might be confused why they're no longer seeing inbound connections in their logs?

💬 stickies-v commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2600414530)
Is there a reason why the message is split across the log statement and the `new_peer_info` lambda? That feels a bit clunky to me?

<details>
<summary>git diff on 8f553ac9df</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index a823b2c2e2..54528ff3d0 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3659,23 +3659,23 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
return;
}


...

💬 stickies-v commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2600308329)
clang-format-nit

<details>
<summary>git diff on 8f553ac9df</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index a823b2c2e2..802f68e98f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3661,11 +3661,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,

auto new_peer_info = [&]() {
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr)};
- return strprintf("transpo
...

💬 stickies-v commented on pull request "log: exempt all category-specific logs from ratelimiting when running with debug":
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3629382646)
Closing this in favour of https://github.com/bitcoin/bitcoin/pull/34008, I think that PR's new approach of downgrading the inbound log to `LogDebug` is superior.

I think the approach in this PR makes sense if there are other places in the code with a similar issue (modulo addressing [this comment](https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621967700)), but until that use case pops up, there's no point keeping this PR open.

✅ stickies-v closed a pull request: "log: exempt all category-specific logs from ratelimiting when running with debug"
(https://github.com/bitcoin/bitcoin/pull/34018)

🤔 hebasto reviewed a pull request: "guix: use GCC 14.3.0 over 13.3.0"
(https://github.com/bitcoin/bitcoin/pull/33775#pullrequestreview-3554458851)
My Guix build:
```
aarch64
02de261cf6c90f15a69d42b43e63485a6c0cdb3d745b5916b6834278a1d63dbc guix-build-a72ff31b879f/output/aarch64-linux-gnu/SHA256SUMS.part
cf2d9615338be34f5082940d7d2b1a4c085226a410a75dceec764e42cd5df5ee guix-build-a72ff31b879f/output/aarch64-linux-gnu/bitcoin-a72ff31b879f-aarch64-linux-gnu-debug.tar.gz
e18f34ce40036367f611451ae4dfe39e7dedeb53e549f229cff118bf2c2836a8 guix-build-a72ff31b879f/output/aarch64-linux-gnu/bitcoin-a72ff31b879f-aarch64-linux-gnu.tar.gz
17cd0a5fad2a4e
...

💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2600468539)
True, changed to `ABCStep`. The point is that we want to sometimes stop here (and possibly call `CheckBlockIndex` etc.), because whenever we would release cs_main, the same would be possible from another thread.