👍 hodlinator approved a pull request: "init: Handle dropped UPnP support more gracefully"
(https://github.com/bitcoin/bitcoin/pull/31916#pullrequestreview-2645624370)
cr-ACK 44041ae0eca9d2034b7c2bdef24724809cc35e90
Most significant aspect is that it migrates users/nodes who previously enabled UPnP in the GUI to now use the new NATPMP/PCP support. Without this those nodes would probably have little chance of accepting inbound connections (depending on their LAN setup), until the user realizes their node is behaving differently (if ever).
#### Changes since [previous review](https://github.com/bitcoin/bitcoin/pull/31916#pullrequestreview-2641238999)
-
...
(https://github.com/bitcoin/bitcoin/pull/31916#pullrequestreview-2645624370)
cr-ACK 44041ae0eca9d2034b7c2bdef24724809cc35e90
Most significant aspect is that it migrates users/nodes who previously enabled UPnP in the GUI to now use the new NATPMP/PCP support. Without this those nodes would probably have little chance of accepting inbound connections (depending on their LAN setup), until the user realizes their node is behaving differently (if ever).
#### Changes since [previous review](https://github.com/bitcoin/bitcoin/pull/31916#pullrequestreview-2641238999)
-
...
💬 ryanofsky commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2685980592)
> cc [@ryanofsky](https://github.com/ryanofsky) I believe this is an alternative way to ease your concern from [#31161 (review)](https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2644640989), assuming that 31161 triggers a re-configure.
Thanks, this is interesting and I would agree that if we had a mechanism that would make it an error for the configure step to run after any build step had run, that would make the concern I had in #31161 moot. However, I think that change would al
...
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2685980592)
> cc [@ryanofsky](https://github.com/ryanofsky) I believe this is an alternative way to ease your concern from [#31161 (review)](https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2644640989), assuming that 31161 triggers a re-configure.
Thanks, this is interesting and I would agree that if we had a mechanism that would make it an error for the configure step to run after any build step had run, that would make the concern I had in #31161 moot. However, I think that change would al
...
🤔 ryanofsky reviewed a pull request: "doc: Update translation generation instructions"
(https://github.com/bitcoin/bitcoin/pull/31930#pullrequestreview-2645641106)
Code review ACK 75d5d235a6b5eb6b960be0c5e6e181460a1ac5e6. Looks good as a temporary fix and I think after #31741 we should be able to drop the extra argument.
(https://github.com/bitcoin/bitcoin/pull/31930#pullrequestreview-2645641106)
Code review ACK 75d5d235a6b5eb6b960be0c5e6e181460a1ac5e6. Looks good as a temporary fix and I think after #31741 we should be able to drop the extra argument.
🤔 l0rinc requested changes to a pull request: "doc: update fuzz instructions when on macOS"
(https://github.com/bitcoin/bitcoin/pull/31954#pullrequestreview-2645628866)
Concept ACK
Finally fuzzing is working with these changes (tested on M4 Max with Clang 19.1.7), but we're still getting warnings - and a I left a few more suggestions
(https://github.com/bitcoin/bitcoin/pull/31954#pullrequestreview-2645628866)
Concept ACK
Finally fuzzing is working with these changes (tested on M4 Max with Clang 19.1.7), but we're still getting warnings - and a I left a few more suggestions
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972246194)
we can likely do this in a single command:
```suggestion
$ brew install llvm lld
```
We also likely need to remove the command from https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md?plain=1#L150 now
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972246194)
we can likely do this in a single command:
```suggestion
$ brew install llvm lld
```
We also likely need to remove the command from https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md?plain=1#L150 now
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972242558)
Can you add a tracking issue here to know when we can safely remove it
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972242558)
Can you add a tracking issue here to know when we can safely remove it
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972247440)
This seems to need an update - though I'd just remove the command from the comment:
```suggestion
Full configuration steps:
```
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972247440)
This seems to need an update - though I'd just remove the command from the comment:
```suggestion
Full configuration steps:
```
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972250186)
I'm getting:
> clang++: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972250186)
I'm getting:
> clang++: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2686156550)
Note that after https://github.com/bitcoin/bitcoin/pull/30205/files#diff-aaace4d9302b33227cc9106d59234f6ae0a8946cf23fc439b3936444375d986fR282 this now needs a rebase and fix.
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2686156550)
Note that after https://github.com/bitcoin/bitcoin/pull/30205/files#diff-aaace4d9302b33227cc9106d59234f6ae0a8946cf23fc439b3936444375d986fR282 this now needs a rebase and fix.
💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972452806)
Taken
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972452806)
Taken
💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972453011)
Took something similar
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972453011)
Took something similar
💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972453273)
Resolved warnings
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972453273)
Resolved warnings
💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972457053)
Not sure I feel this is big enough to add to the issue list and I would be happy for the docs to be updated when someone notices again that it's not needed any more, especially as this workaround should continue to work in the future even if strictly not needed any more.
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972457053)
Not sure I feel this is big enough to add to the issue list and I would be happy for the docs to be updated when someone notices again that it's not needed any more, especially as this workaround should continue to work in the future even if strictly not needed any more.
💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972458293)
But obviously if you feel differently, go ahead and make the issue.
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972458293)
But obviously if you feel differently, go ahead and make the issue.
💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2686267479)
@Prabhat1308 and @l0rinc, I've updated the instructions to achieve the same goal but without the warnings.
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2686267479)
@Prabhat1308 and @l0rinc, I've updated the instructions to achieve the same goal but without the warnings.
💬 ayanwaar commented on issue "SignatureCreator should supply auxiliary data argument for additional bip340 signature security":
(https://github.com/bitcoin/bitcoin/issues/31883#issuecomment-2686280021)
https://github.com/bitcoin/bitcoin/issues/31883
(https://github.com/bitcoin/bitcoin/issues/31883#issuecomment-2686280021)
https://github.com/bitcoin/bitcoin/issues/31883
💬 ayanwaar commented on something "":
(https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3#commitcomment-153053812)
db36a92c02b83f2e6477a5a55fc061319f7cc6a3
(https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3#commitcomment-153053812)
db36a92c02b83f2e6477a5a55fc061319f7cc6a3
💬 ayanwaar commented on something "":
(https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3#r153053834)
db36a92c02b83f2e6477a5a55fc061319f7cc6a3
(https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3#r153053834)
db36a92c02b83f2e6477a5a55fc061319f7cc6a3
💬 ayanwaar commented on something "":
(https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3#r153053844)
db36a92c02b83f2e6477a5a55fc061319f7cc6a3
(https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3#r153053844)
db36a92c02b83f2e6477a5a55fc061319f7cc6a3
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972485214)
I meant the `ld` issue - not critical
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1972485214)
I meant the `ld` issue - not critical