Bitcoin Core Github
44 subscribers
119K links
Download Telegram
📝 l0rinc opened a pull request: "clang-format: modernize and realign clang-format configuration"
(https://github.com/bitcoin/bitcoin/pull/32813)
Updates `.clang-format` file to reflect modern Clang-Format standards while preserving most of the existing formatting behavior, except for the very last commit which [aligns `AfterStruct` brace placement](https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072678126) with `AfterClass` for code formatting.

* the first few commits modernize the deprecated config keys to help with filtering later;
* followed by the removed redundant/default values via scripted diffs verified via `clang-
...
💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r2167065003)
Because `FinishTransaction` now takes a `CMutableTransaction&` instead of a `const CMutableTransaction&`, we can't give it an rvalue.
🤔 maflcko reviewed a pull request: "clang-format: modernize and realign clang-format configuration"
(https://github.com/bitcoin/bitcoin/pull/32813#pullrequestreview-2958744097)
last commit seems fine, but the others i am not sure. at a minimum you'll have to drop the scripted diffs from executing, because they can't be reproduced anyway with different clang-format versions
💬 maflcko commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167057157)
a666d8ab6f3c019a9f7e184beb9b90affbb8075c: not sure about removing the default values. I don't think there is any harm in having them. In fact, we don't want this to silently change from down under with the next clang update.
💬 maflcko commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167063237)
7dda3d2c0e65ac1fa08ac3b24a13f2005c6bef1a: not sure about changing the values. seems fine to just keep the old value, which will likely work forever in a backward compatible way
👍 vasild approved a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-2958772275)
ACK 3e7abceecfd790bc0887f647d3f731328e19810f
💬 l0rinc commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167075660)
What about the other default values, what if those change?
💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r2167076582)
Done
💬 l0rinc commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167081920)
Many of these keys aren't even listed in the documentation and the IDE shows that many are invalid since it doesn't match the allowed schema.
💬 maflcko commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167114881)
I'd say the others are probably edge cases to not matter much, but no strong opinion. Seems fine to add them as well to avoid silent changes. However, I don't think the config is forward compatible, so it can probably only contain keys of the minimum required clang version.
💬 vasild commented on pull request "doc: add release notes for #32425":
(https://github.com/bitcoin/bitcoin/pull/32727#issuecomment-3005398388)
`a4cfaa5eab...b9a2e8ee96`: take suggestions
💬 vasild commented on pull request "doc: add release notes for #32425":
(https://github.com/bitcoin/bitcoin/pull/32727#discussion_r2167135148)
Done, plus some more rewording.
💬 hebasto commented on pull request "cmake: Use `HINTS` instead of `PATHS` in `find_*` commands":
(https://github.com/bitcoin/bitcoin/pull/32805#issuecomment-3005421352)
@purpleKarrot

Thank you for the review!

> Nit: Is there a problem on master?

In terms of build failure -- no.
In terms of needlessly including overly broad paths like `/usr/local/include` or `/opt/homebrew/include` on macOS -- yes.

> It looks like the result from `libqrencode.pc` is ignored...

It depends on the platform used for building. On macOS, that's the case on the master branch, but not with this PR.

> ... if that is not causing actual problems, maybe `pkg_check_module
...
🤔 mzumsande reviewed a pull request: "test: fix catchup loop in outbound eviction functional test"
(https://github.com/bitcoin/bitcoin/pull/32742#pullrequestreview-2958894994)
Code Review ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6

I also think that this part of the test would benefit from additional cleanups / more comments, it's pretty hard to understand what's going on.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2167155199)
The reviewers of the PR are only ACKing that the actual data from the collaborative run was used and that the data was encoded correctly by reproducing the file hash. This is the part I am referring to a little further below:

```
Files will not be merged to the repository
without at least two additional reviewers confirming that the process described
above was followed as expected and that the encoding step yielded the same
file hash.
```

The 5 participants is the number I am looking
...
👍 pablomartin4btc approved a pull request: "doc: add release notes for #32425"
(https://github.com/bitcoin/bitcoin/pull/32727#pullrequestreview-2958946450)
> a4cfaa5eab...b9a2e8ee96: take suggestions

re-ACK b9a2e8ee96
👍 hebasto approved a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2958947519)
re-ACK 6c2538d5bfeafe6f234e5382c151d173826d78f0, only rebased since my previous [review](https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2891203225).
💬 vasild commented on pull request "doc: add release notes for #32425":
(https://github.com/bitcoin/bitcoin/pull/32727#discussion_r2167192118)
Done.

"for example `-proxy=127.0.0.1:5555=ipv6` would configure a proxy only for IPv6"

> it looks like it's talking about the past behavior

I guess I used "5: The second conditional" from https://www.perfect-english-grammar.com/would.html:
> If I had enough money, I would travel around the world.
> If we lived in Madrid, we would study Spanish.
🤔 janb84 reviewed a pull request: "doc: add release notes for #32425"
(https://github.com/bitcoin/bitcoin/pull/32727#pullrequestreview-2959000718)
reACK b9a2e8ee965daffe2bc618f58b21f0ddebadeb23

Changes sinds last ACK;
- div textual changes

(still find it weird that a non-localhost ipv4 address for ipv6 proxy can be used, but agree to disagree on that )
💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2167253212)
I think the +1 offset would still be needed in a corner case when the input of the ceil function is already an integer. In this case, if +1 is removed, the timeout will not be triggered because the check `current_time > peer->m_headers_sync_timeout` ([here](https://github.com/bitcoin/bitcoin/blob/7d5a6d17398aae3cea6d63564d33e7e49da3c71c/src/net_processing.cpp#L5888)) would evaluate to `timeout > timeout` (false) instead of `timeout + 1 > timeout` (true).