👍 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.

🤔 sipa reviewed a pull request: "net/rpc: Report inv information for debugging"
(https://github.com/bitcoin/bitcoin/pull/33448#pullrequestreview-3257761423)
utACK 2738b63e025d240618b3c72c28243c3e4d7d9c79

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2372167035)
Your analysis seems correct to me. However, I prefer being explicit here. Otherwise, if any user-declared constructor is added in the future, the default constructor would not be generated.

💬 instagibbs commented on pull request "rpc: Always return per-wtxid entries in submitpackage tx-results":
(https://github.com/bitcoin/bitcoin/pull/33427#issuecomment-3323848596)
ACK cad9a7fd7370f6df38370569f9d2de6ac6b7137a