💬 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))
````

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566578365)
to minimize the diff, this comment for `PickTxForSend` should be adjusted in a previous commit where it was introduced (not in 129d67ebafdd63c4a48fc6c8f45c21f729cd8cf3)

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566558339)
could we move this in a separate commit as well, it's not immediate obvious to me why this is safe to do

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566595639)
we likely need a release notes for this, many people would want to know about it

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566503863)
I prefer making the code easy to focus on the important stuff, so that the glue code fades into the background. Here the debug logging is dominating the method.
I'm more interested here in seeing e.g. why `pnode.GetLocalNonce()` is inlined during this change.

Instead of multilining it, can we use `pnode.LogIP(fLogIPs)` here as well as we do in other places:
```suggestion
LogDebug(BCLog::NET, "send version message: version %d, blocks=%d, txrelay=%d, peer=%d%s", PROTOCOL_VERSION, my_heig
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2566603668)
```suggestion
"via the Tor network. This conceals the transaction's origin. The transaction\n"
```

💬 fjahr commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#issuecomment-3583484232)
> Not sure whether the concept of script validation has to creep into the benchmarking parameters, maybe we could generalize it to `-thread-count` or `-par` or something.

Thanks, happy to change this and address the other comments. But aside from that do you think this is generally interesting enough to be considered merging? I was unsure and since I haven't worked much on benchmarking I would like to get some concept-ack-ish feedback from the benchmarking wg :)

⚠️ multicryptobank opened an issue: "[BIP-352] Silent Payments Implementation"
(https://github.com/bitcoin/bitcoin/issues/33957)
Hi.

We are keen on 'playing with'/testing Silent Payments in Bitcoin Core and thus contributing to it.
There seems to be a myriad of relevant PRs (#28201, #28122, #27827 and so on) as well as (open) issues which confuse us.
Which one is the latest one? How many (and which ones) must be implemented? In which order?

Are there any Windows desktop wallets we could use for testing purposes?

Sorry about so many questions.

Thanks in advance.

@josibake @Eunovo

💬 pinheadmz commented on issue "[BIP-352] Silent Payments Implementation":
(https://github.com/bitcoin/bitcoin/issues/33957#issuecomment-3583520845)
Follow https://github.com/bitcoin/bitcoin/issues/28536

✅ pinheadmz closed an issue: "[BIP-352] Silent Payments Implementation"
(https://github.com/bitcoin/bitcoin/issues/33957)

⚠️ theuni opened an issue: "Net split meta issue"
(https://github.com/bitcoin/bitcoin/issues/33958)
Note: This is just a thought dump for now.. an attempt at a high-level roadmap based on an initial POC and the feedback I've gathered thus far. I will edit this meta issue frequently as a real roadmap takes shape.

The goal of this project to cleanly separate the net/net_processing layers, so that neither depends on the implementation details of the other.

In order to achieve that, we'll need to de-tangle the two subsystems (namely `CConnman` and `PeerManager`) and connect them instead via abst
...

💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3583625311)
> > It might be possible to do it faster and with less disk usage for relatively short rollbacks via a two step process:
>
> I actually had a similar idea early on but then stayed with the simpler approach. I will try this out with a POC and check the performance impact.

I tried to a few different takes on the delta-based idea including some vibe coding tests. [Here](https://github.com/fjahr/bitcoin/commit/a86bbfbd6f0d6e440ff8c5c303e392e51e3eb60b) is the latest one, something is definitely
...