💬 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

💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#issuecomment-3323854411)
rfm?

💬 Crypt-iQ commented on pull request "log: reduce excessive "rolling forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2372212754)
I think `Error` and `Warning` should be rate limited as they currently are. I am ok with a critical level and macro, but I think it might need more conceptual review and feedback vs. what this PR aims to do?

💬 sipa commented on pull request "rpc: addpeeraddress: throw on invalid IP":
(https://github.com/bitcoin/bitcoin/pull/33430#issuecomment-3323871396)
utACK 316a0c513278d53cb25f42ea502d20691962aad6

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2372218235)
Thanks! Taken.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2372218950)
Thanks! Fixed.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3323881144)
The feedback from @vasild has been addressed.

💬 sipa commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2372244246)
In commit "net: change FindNode() to not return a node and rename it"

I think this name change is confusing, because `CNode::m_addr_name` is not necessarily an address/port string; in the case of manually-connected nodes (`-addnode` and friends), it can also be a hostname.

Checking the codebase to verify this made me realize that the RPC help for `getpeerinfo` is also wrong in this regard.

💬 fanquake commented on pull request "tor: enable PoW defenses for automatically created hidden services":
(https://github.com/bitcoin/bitcoin/pull/33414#issuecomment-3323930161)
@laanwj you might have some thoughts here?

💬 hebasto commented on pull request "CPack":
(https://github.com/bitcoin/bitcoin/pull/33455#issuecomment-3323935593)
> Run `cpack` from the build directory after a successful build to select multiple package generators:
>
> ```shell
> cmake --build .
> cpack -G '7Z;NSIS64'
> ```
>
> Or build the `package` and/or `package_source` targets:
>
> ```shell
> cmake --build . --target package
> cmake --build . --target package_source
> ```

Is the reproducibility of these commands guaranteed, or can it be enforced somehow?

> TODO:
>
> * [ ] Windows start menu entries
>
> * [ ] Option
...

🚀 fanquake merged a pull request: "ci: disable cirrus cache in 32bit arm job"
(https://github.com/bitcoin/bitcoin/pull/33302)