👋 mzumsande's pull request is ready for review: "test: add functional test for outbound connection management"
(https://github.com/bitcoin/bitcoin/pull/33954)

💬 janb84 commented on pull request "guix: reduce allowed exported symbols":
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3583157897)
my Guix Build Output (equal to fanquake)

**Host architecture:** `aarch64`
**Commit:** `2984690b20f7`

```shell
5e1eb207d68450278951fe45b8c06465beeb49af6a84ef12d037430dcd6519ad guix-build-2984690b20f7/output/aarch64-linux-gnu/SHA256SUMS.part
dd93dd1b964f0629eae5db5106ad1afc307f4317fdecd5f44223aa1942f56348 guix-build-2984690b20f7/output/aarch64-linux-gnu/bitcoin-2984690b20f7-aarch64-linux-gnu-debug.tar.gz
af0c96a7f3f1eb528e21af366d4fd6f585dd5b6620d147822f028bb4c14023f0 guix-build-29
...

📝 Crypt-iQ opened a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956)
`CConnman::Stop()` resets `semOutbound`, yet `m_reconnections` is not cleared in `Stop`. Each `ReconnectionInfo` contains a grant member that points to the memory that `semOutbound` pointed to and `~CConnman` will attempt to access the grant field (memory that was already freed) when destroying `m_reconnections`. Fix this by calling `m_reconnections.clear()` in `CConnman::Stop()` and add appropriate annotations.

I was able to reproduce the original issue https://github.com/bitcoin/bitcoin/iss
...

💬 sedited commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2566504962)
I'm confused by this change. Why are you introducing this variable? The comment is also wrong, since this processes the block on the background chain. I would still prefer if the behaviour of ProcessNewBlock would not be changed as part of this patch set.

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3583308231)
Thanks for all the feedback so far and sorry for the slow response!

> Rather than being direct RPC functionality, maybe it would be better to have an RPC function to export a copy of the utxo set at the current height, and have a separate bitcoin-kernel binary that performs the rollback and utxoset stats calculation itself?

Hm, feels like a bit overengineered for this functionality, considering the overhead for test coverage and build changes for this. Maybe I am overcomplicating it in my
...

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567639)
Changed to an `Assume` instead.

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567768)
I don't think so, my understanding is that the cursor/LevelDB iterator works on a snapshot of the DB itself which doesn't get mutated. You can see the same pattern in `WriteUTXOSnapshot` where we are working with a cursor without holding `cs_main` as well.

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567985)
Hm, I don't think we log anything on the creation of the DB so I think I would keep it the same on the destruction. It should only be a debug level log if we would add anything like that since it seems a bit low level for the general user.

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569186)
Hm, this one and the other similar comments about test coverage are all cases that are pretty hard to hit because they should only be possible in a case of db corruption or a similarly unlikely event. Not saying that this wouldn't be valuable coverage, but afaik we hardly ever go through the hassle to cover such cases and this RPC is far from being a critical path in the comparison to the rest of the code base. So, unless you have a specific suggestion for how the can be hit in practical way I w
...

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569370)
Taken

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569595)
I don't think this is necessary, I don't think a user would naturally assume that there is a reason to suspend network activity for this. Previously we had to do this as basically a hack. When we don't do this anymore it would seem odd to me to mention it.

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3583311390)
[7f28483](https://github.com/bitcoin/bitcoin/commit/7f284835be87c4d1a8a56804992043e12dae1ea1) to [53ef86f](https://github.com/bitcoin/bitcoin/commit/53ef86f0ecd4ca2b66c4e3dce591ac4718e38f0e):
Rebased due to silent conflict with #33892 and changed "at least" to "more than" in the error message (same reason as https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2543670655)

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2566624413)
> i think it would be better if the caller could decide if there really was a storage corruption and an error should be logged, or if the user just had a typo in the rest url.

Sounds good - WDYT about https://github.com/romanz/bitcoin/commit/9716ff4dd92b31fa4d97b909892f7fc8e3c82b9c?

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2566629544)
Thanks! Fixed in [ff5c3fecea..f50c0d9a3e](https://github.com/bitcoin/bitcoin/compare/ff5c3feceaba496ff25efd8420cfcc32e0864bcc..f50c0d9a3ec3b830de79d64082e9f0ab4a9fc4d2).

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2566644764)
You're right - I just wanted to mention that trying to fetch a pruned block still returns `404 Not Found` error.

For testing the `catch (const std::runtime_error& e)` clause, I have used an invalid URL encoding by passing the `%XY` parameter:
```
$ curl -v "http://localhost:8332/rest/blockpart/00.hex?%XY"
```

👍 l0rinc approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3512429582)
I managed to go through the rest of the commits, please see my remaining comments, hope they're useful.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566225675)
We could separate the concerns here slight by giving this list of values a name, e.g.
```C++
bool IsPrivateBroadcastAllowed(std::string_view type) noexcept
{
return type == NetMsgType::VERSION
|| type == NetMsgType::VERACK
|| type == NetMsgType::INV
|| type == NetMsgType::TX
|| type == NetMsgType::PING;
}

void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
{
AssertLockNotHeld(m_total_bytes_sent_mutex);

if (pnode->IsPrivat
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566302310)
My impression was that we don't usually log this much.
And given how unreasonably large this method is (>1600 lines) and how intertwined the logic is here, I wonder if we really need all this logging - with very heavy indentation and lots of lines for something that's not very important.

Given that most of the messages seem to end with
```C++
, peer=%d%s",
msg,
node.GetId(),
node.LogIP(fLogIPs);
```
maybe we could add some helpers like
```C++
template <typename... Args>
void LogPri
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566516453)
There are several changes happening here at once, and it is not obvious how the non-private-broadcast behavior changes as a result.

Would it be possible to first refactor `PushNodeVersion` in a preparatory commit with no behavioral changes, and then add the private-broadcast specialization in a follow-up commit?

For example, the preparatory commit could introduce a small helper lambda and keep the existing behavior (simplified debug logging as described in other comment):
```C++
void Pee
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566435407)
can we use `Ticks` here?
https://github.com/bitcoin/bitcoin/blob/fa6b45fa8ec8248544d22ba8429be8f6df19024a/src/util/time.h#L72-L76
```suggestion
Ticks<std::chrono::seconds>(PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME));
```
or maybe this also works:
```suggestion
count_seconds(PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME))
````