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

💬 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