💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549880323)
I think the same applies here as in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549873306, the suggested version focuses on the happy path (which is the purpose of the method - and it's also easer to say out loud), which I found helps with comprehension.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549880323)
I think the same applies here as in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549873306, the suggested version focuses on the happy path (which is the purpose of the method - and it's also easer to say out loud), which I found helps with comprehension.
💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549881417)
Why is that better?
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549881417)
Why is that better?
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563180629)
> > > Not sure I understand the dependence on #33775?
> > > CI is using it's own build, not the guix build right? Also MinGW brings it's own GCC doesn't it?
> >
> >
> > The CI jobs are more useful when using toolchains with versions similar to those in the Guix script.
>
> I think this is just a doc question. I think the `trixie/g++-mingw-w64-ucrt64` task can be left as-is, without adding a dependency here. In the future, #33593 could simply update the comment to clarify which one is ru
...
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563180629)
> > > Not sure I understand the dependence on #33775?
> > > CI is using it's own build, not the guix build right? Also MinGW brings it's own GCC doesn't it?
> >
> >
> > The CI jobs are more useful when using toolchains with versions similar to those in the Guix script.
>
> I think this is just a doc question. I think the `trixie/g++-mingw-w64-ucrt64` task can be left as-is, without adding a dependency here. In the future, #33593 could simply update the comment to clarify which one is ru
...
💬 frankomosh commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549888077)
> Why is that better?
"ignored" clarifies the value is not read at all, while "shouldn't matter" could mean read-but-no-effect. Very minor preference though, I think either can works just fine
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549888077)
> Why is that better?
"ignored" clarifies the value is not read at all, while "shouldn't matter" could mean read-but-no-effect. Very minor preference though, I think either can works just fine
💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549896888)
Need to pick the right name here.
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549896888)
Need to pick the right name here.
👍 maflcko approved a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3492941007)
lgtm
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3492941007)
lgtm
💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549906416)
Thanks, I prefer the previous versions since it's higher level, it's expressing the intent, not the implementation detail
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549906416)
Thanks, I prefer the previous versions since it's higher level, it's expressing the intent, not the implementation detail
🤔 hodlinator reviewed a pull request: "build: Embedded ASMap [3/3]: Build binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3492585142)
### Concept ACK 1dcd2e8a983406d12009e41bac5dbaeb12f330be
I do understand the distaste at adding 1MB+ binary blobs into a reproducible build. From that perspective, it would be cleaner to include the ASMap blob as a separate file included in our Bitcoin Core releases.
But having a separate file opens up the can of worms of where to install it on various operating systems and distributions, how to encourage package maintainers to include and ensure it ends up in the right locations, and making B
...
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3492585142)
### Concept ACK 1dcd2e8a983406d12009e41bac5dbaeb12f330be
I do understand the distaste at adding 1MB+ binary blobs into a reproducible build. From that perspective, it would be cleaner to include the ASMap blob as a separate file included in our Bitcoin Core releases.
But having a separate file opens up the can of worms of where to install it on various operating systems and distributions, how to encourage package maintainers to include and ensure it ends up in the right locations, and making B
...
💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2549691949)
nit: Convention seems to be
```suggestion
if(WITH_EMBEDDED_ASMAP)
```
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2549691949)
nit: Convention seems to be
```suggestion
if(WITH_EMBEDDED_ASMAP)
```
💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2549632572)
PR description nit:
```diff
- -DWITH_EMBEDDED_ASMAP=NO
+ -DWITH_EMBEDDED_ASMAP=OFF
```
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2549632572)
PR description nit:
```diff
- -DWITH_EMBEDDED_ASMAP=NO
+ -DWITH_EMBEDDED_ASMAP=OFF
```
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549938061)
Thanks! Fixed.
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549938061)
Thanks! Fixed.
📝 dergoegge opened a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921)
* Remove or consolidate macOS notes sprinkled throughout the doc into dedicated section
* Note that support for fuzzing on macOS is not maintained
* Provide best effort steps for fuzzing macOS
Closes #33731
(https://github.com/bitcoin/bitcoin/pull/33921)
* Remove or consolidate macOS notes sprinkled throughout the doc into dedicated section
* Note that support for fuzzing on macOS is not maintained
* Provide best effort steps for fuzzing macOS
Closes #33731
💬 dergoegge commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3563260883)
Opened #33921
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3563260883)
Opened #33921
💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549961718)
Why is dev-mode being removed/options being changed here?
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549961718)
Why is dev-mode being removed/options being changed here?
💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549966742)
Yeah, I guess there were some silent conflicts. Also with https://github.com/bitcoin/bitcoin/pull/33919
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549966742)
Yeah, I guess there were some silent conflicts. Also with https://github.com/bitcoin/bitcoin/pull/33919
💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563288119)
Only changes since my last ack: https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3485151628
* Rebase to drop unused `-Warray-bounds` stuff
* Change the `windows-msvcrt-cross` task name after rebase
re-ACK 5b06145e4e99f79b1889e5ac8dca89bf0a718c01 🍍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkU
...
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563288119)
Only changes since my last ack: https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3485151628
* Rebase to drop unused `-Warray-bounds` stuff
* Change the `windows-msvcrt-cross` task name after rebase
re-ACK 5b06145e4e99f79b1889e5ac8dca89bf0a718c01 🍍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkU
...
💬 maflcko commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549976006)
provided on best effort basis -> provided on a best-effort basis [missing article "a" and standard phrasing uses "best-effort" as a compound adjective]
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549976006)
provided on best effort basis -> provided on a best-effort basis [missing article "a" and standard phrasing uses "best-effort" as a compound adjective]
💬 maflcko commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549977895)
I think the note can be kept and moved here in this line?
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549977895)
I think the note can be kept and moved here in this line?
👍 maflcko approved a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493037953)
lgtm. makes sense to just mention the stuff that is known to work and remove the stuff that is known to not work
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493037953)
lgtm. makes sense to just mention the stuff that is known to work and remove the stuff that is known to not work
✅ maflcko closed an issue: "Unable to fuzz in local on MacOS 15.4.1"
(https://github.com/bitcoin/bitcoin/issues/33667)
(https://github.com/bitcoin/bitcoin/issues/33667)