💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3323153850)
`504803f909...48c5939cf4`: fix tidy, note that also in `master` there is inconsistency: the prototype of `OpenNetworkConnection()` in `net.h` uses `strDest` whereas the implementation in `net.cpp` uses `pszDest`. Resolved by this PR.

> If you're updating the name anyways, maybe just get rid of the hungarian notation and call it `destination`?

Yes, I think this would be very nice to do. I avoided it because it would bloat the diff as a lot of lines inside the body of the functions will be
...

💬 ryanofsky commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3323154136)
Code review ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3

> As a best practice, Bitcoin Core has avoided an option when the developers cannot articulate when they should be used.

I like this PR, and think ideally a followup could improve the documentation more to address this problem. I think the documentation could mention the previous default, and that it was changed to improve network efficiency and censorship resistance, but that there are practical and philosophical disagreements about
...

💬 willcl-ark commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3323201709)
> Will the be useful overall? I think it is clear that no developer is using this platform right now (maybe not even a user), so the benefit of the CI config is mostly a sanity check.

My understanding of this job is that it represents all Big Endian systems, and therefore, as you say just exists as a sanity check (that we don't have any endianness bugs). If nobody uses any BE systems (IBM Z, older SPARC, some powerpc) perhaps it's not even useful in that role though...

I'd agree with havin
...

👍 luke-jr approved a pull request: "rpc: fix getblock(header) returns target for tip"
(https://github.com/bitcoin/bitcoin/pull/33446#pullrequestreview-3257204496)
crACK bf7996cbc3becf329d8b1cd2f1007fec9b3a3188

👍 vasild approved a pull request: "docs: Undeprecate datacarrier and datacarriersize configuration options"
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3257209276)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3

Thanks!

💬 waketraindev commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3323324465)
Concept NACK

The peeps wanting to reject transactions with this option don't want it.

And in general the extra pool is too small to "cache" a large number of transactions causing rejecting nodes to redownload already seen transactions.

✅ RandyMcMillan closed an issue: "cli:passing non-integers to datacarriersize"
(https://github.com/bitcoin/bitcoin/issues/33460)

💬 RandyMcMillan commented on issue "cli:passing non-integers to datacarriersize":
(https://github.com/bitcoin/bitcoin/issues/33460#issuecomment-3323416365)
@maflcko thanks

👍 vasild approved a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3257285157)
ACK ecacfac7ead178ace3b0ec9393a7c63b5d8d3576

💬 vasild commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2371863274)
My understanding is that such default constructing is not needed because `m_search_string` will be default constructed anyway. That is, drop the line `m_search_string()`.

💬 vasild commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2371858856)
Correct me if I am wrong, in the absence of other constructors (like in `RecentRequestEntry`), this line is not needed because it will generate the same constructor that will be generated if we don't specify any constructor. In other words, I suggest to drop the line `RecentRequestEntry() = default;`.

💬 vasild commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2371865866)
Same here, remove `CZMQAbstractNotifier() = default;`?

💬 vasild commented on pull request "log: reduce excessive "rolling forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2371943984)
d541409a64c60d127ff912dad9dea949d45dbd8c introduced rate limiting. If we are going to introduce a function (macro) to ignore the rate limiting, then it is just a matter of time before some code that uses the new unlimited function triggers a disk-fill. It looks to me that introducing such a function defeats the purpose of rate-limiting.

Now, if we really want to have rate-unlimited logging function, then we can consider that d541409a64c60d127ff912dad9dea949d45dbd8c went a bit too far:
>
...

💬 vasild commented on pull request "log: reduce excessive "rolling forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2371965633)
Not modifying the original `LogPrintLevel_(rate-unlimited = true)` would remove this discussion from this PR. I agree with the other changes in this PR, but am somewhat anxious about the new `LogEssential()` macro.

💬 RandyMcMillan commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2371985398)
By NOT taking the Hash160 of the signet_challenge - the substring will be familiar to the user. This IMO is preferred.

example:
The user may have multiple signet challenges - it is better to preserve the “finger print” of the signet challenge so that the user can easily identify the related datadir when browsing the file structure manually.

additionally:
I have a proposal which displays the signet_challenge(finger print) in the gui.

💬 RandyMcMillan commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2372006430)
the collision concern isnt a huge issue IMO. Keeping the appended “finger print” familiar to the user is preferred.

💬 moonsettler commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3323721236)
> Those who fully want to embrace filtering should be looking for alternative software, rather than demand that Bitcoin Core implements it for them.

Keeping the option makes it a lot easier to maintain such software over time. There are more people who want to set it to non-default value today than ever before.

💬 sipa commented on pull request "rpc: fix getblock(header) returns target for tip":
(https://github.com/bitcoin/bitcoin/pull/33446#issuecomment-3323745993)
utACK bf7996cbc3becf329d8b1cd2f1007fec9b3a3188

💬 sipa commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2372128145)
Pass by reference? `Proxy` is pretty large (80 bytes).

👍 vasild approved a pull request: "rpc: addpeeraddress: throw on invalid IP"
(https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3257726472)
ACK 316a0c513278d53cb25f42ea502d20691962aad6

As an alternative, we could use the `error` field in the response instead of throwing an exception. The `error` field is currently not set in `master` if an invalid IP address is provided.