💬 laanwj commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862408273)
Concept ACK but NACK on the implementation, as i understand this makes the build output depend on the system clock.
What about making the COPYRIGHT_YEAR depend on the last git commit?
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862408273)
Concept ACK but NACK on the implementation, as i understand this makes the build output depend on the system clock.
What about making the COPYRIGHT_YEAR depend on the last git commit?
🤔 i-am-yuvi reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2824481927)
re-ACK 69139049194e55a7fd99b7125b9afccb8585922d
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2824481927)
re-ACK 69139049194e55a7fd99b7125b9afccb8585922d
💬 fanquake commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862417346)
> What about making the COPYRIGHT_YEAR depend on the last git commit date in UTC?
I'm open to implementing this in any way, but if the logic for doing so, is going to end up being more than a few lines / add other requirements (i.e what happens if you're building from a tarball/no git etc), then maybe we shouldn't bother. Just wanted to -1 the number of things that need maintenance.
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862417346)
> What about making the COPYRIGHT_YEAR depend on the last git commit date in UTC?
I'm open to implementing this in any way, but if the logic for doing so, is going to end up being more than a few lines / add other requirements (i.e what happens if you're building from a tarball/no git etc), then maybe we shouldn't bother. Just wanted to -1 the number of things that need maintenance.
📝 fanquake converted_to_draft a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445)
Then we no-longer have to "bump" it.
(https://github.com/bitcoin/bitcoin/pull/32445)
Then we no-longer have to "bump" it.
💬 hebasto commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#discussion_r2079330128)
nit: Add `UTC` option to avoid using local time?
(https://github.com/bitcoin/bitcoin/pull/32445#discussion_r2079330128)
nit: Add `UTC` option to avoid using local time?
👍 hebasto approved a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445#pullrequestreview-2824496328)
ACK f930f5e11024fd44a9f1c666d6eee3c9c19edfa0.
It would be useful to mention in the commit message / PR description a note from the CMake docs:
> If the `SOURCE_DATE_EPOCH` environment variable is set, its value will be used instead of the current time.
(https://github.com/bitcoin/bitcoin/pull/32445#pullrequestreview-2824496328)
ACK f930f5e11024fd44a9f1c666d6eee3c9c19edfa0.
It would be useful to mention in the commit message / PR description a note from the CMake docs:
> If the `SOURCE_DATE_EPOCH` environment variable is set, its value will be used instead of the current time.
🚀 fanquake merged a pull request: "lint: Remove string exclusion from locale check"
(https://github.com/bitcoin/bitcoin/pull/32434)
(https://github.com/bitcoin/bitcoin/pull/32434)
👍 laanwj approved a pull request: "doc: swap "Docker image" for "container image""
(https://github.com/bitcoin/bitcoin/pull/32444#pullrequestreview-2824508134)
ACK 1372eb09c5d031315bc6cde34f7a15297e96f4cc
(https://github.com/bitcoin/bitcoin/pull/32444#pullrequestreview-2824508134)
ACK 1372eb09c5d031315bc6cde34f7a15297e96f4cc
💬 laanwj commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862438404)
> If the SOURCE_DATE_EPOCH environment variable is set, its value will be used instead of the current time.
Well that's some important context... Agree in that case.
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862438404)
> If the SOURCE_DATE_EPOCH environment variable is set, its value will be used instead of the current time.
Well that's some important context... Agree in that case.
👍 hebasto approved a pull request: "doc: swap "Docker image" for "container image""
(https://github.com/bitcoin/bitcoin/pull/32444#pullrequestreview-2824537049)
ACK 1372eb09c5d031315bc6cde34f7a15297e96f4cc.
(https://github.com/bitcoin/bitcoin/pull/32444#pullrequestreview-2824537049)
ACK 1372eb09c5d031315bc6cde34f7a15297e96f4cc.
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2862468060)
> Maybe we should give a warning at least (and perhaps suggesting: `-proxy=0=cjdns`) because that (`-proxy=ip[:<port>] -onlynet=onion -cjdnsreachable`) would make CJDNS not to work (?).
That would make automatic outbound connections to Tor peers only. Still manual outbound and inbound connections would work and be possible to/from CJDNS peers. Maybe a warning in the lines of "Automatic outbound connections will only be made to Tor peers, if you want to include CJDNS, then use -onlynet=cjdns i
...
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2862468060)
> Maybe we should give a warning at least (and perhaps suggesting: `-proxy=0=cjdns`) because that (`-proxy=ip[:<port>] -onlynet=onion -cjdnsreachable`) would make CJDNS not to work (?).
That would make automatic outbound connections to Tor peers only. Still manual outbound and inbound connections would work and be possible to/from CJDNS peers. Maybe a warning in the lines of "Automatic outbound connections will only be made to Tor peers, if you want to include CJDNS, then use -onlynet=cjdns i
...
📝 fanquake opened a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446)
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
`iffaddrs.h` that implements one, but not the other.
(https://github.com/bitcoin/bitcoin/pull/32446)
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
`iffaddrs.h` that implements one, but not the other.
👍 rkrux approved a pull request: "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)"
(https://github.com/bitcoin/bitcoin/pull/32305#pullrequestreview-2824580145)
tACK 4b24186
```
git range-diff 23ee07d...4b24186
```
(https://github.com/bitcoin/bitcoin/pull/32305#pullrequestreview-2824580145)
tACK 4b24186
```
git range-diff 23ee07d...4b24186
```
💬 fanquake commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2862498362)
@ryanofsky / @theuni want to circle back?
I think we'll also backport this for `29.x`.
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2862498362)
@ryanofsky / @theuni want to circle back?
I think we'll also backport this for `29.x`.
💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079413858)
```suggestion
`--uninstall` [flag](https://guix.gnu.org/manual/devel/en/guix.html#index-uninstalling-Guix).
```
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079413858)
```suggestion
`--uninstall` [flag](https://guix.gnu.org/manual/devel/en/guix.html#index-uninstalling-Guix).
```
💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079415082)
This is an interesting bug
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079415082)
This is an interesting bug
💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#issuecomment-2862518703)
> This probably applies to testnet and regtest too.
Yes, for quick testing, not having to use an external script to create credentials is more convenient too. Usually cookie-based auth will suffice, but not always when dealing with external applications that need a username/password.
(https://github.com/bitcoin/bitcoin/pull/32423#issuecomment-2862518703)
> This probably applies to testnet and regtest too.
Yes, for quick testing, not having to use an external script to create credentials is more convenient too. Usually cookie-based auth will suffice, but not always when dealing with external applications that need a username/password.
💬 fanquake commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2862534081)
No movement in #31507, but could rebase & update this with the one new commit?
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2862534081)
No movement in #31507, but could rebase & update this with the one new commit?
👍 rkrux approved a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2824665753)
re-ACK f09e1f9
```
git range-diff b6101b1...f09e1f9
```
Range diff contains changes from [this review](https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2818667962) & rebase now that #28710 is merged that has led to the removal of `LegacyScriptPubKeyMan` and naturally all the changes in it from this PR - the overall diff has become marginally smaller.
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2824665753)
re-ACK f09e1f9
```
git range-diff b6101b1...f09e1f9
```
Range diff contains changes from [this review](https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2818667962) & rebase now that #28710 is merged that has led to the removal of `LegacyScriptPubKeyMan` and naturally all the changes in it from this PR - the overall diff has become marginally smaller.
💬 fanquake commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#issuecomment-2862600446)
Guix Build:
```bash
876f676d8a0f348d8a8146b05d9206e8dffbd050269e27fcc41cf09273dfbc51 guix-build-c983612fb43b/output/aarch64-linux-gnu/SHA256SUMS.part
f47bfe426d480a0b7229be7b99f588bad1a5073cc9415cc99f62dd1aac560acf guix-build-c983612fb43b/output/aarch64-linux-gnu/bitcoin-c983612fb43b-aarch64-linux-gnu-debug.tar.gz
cda94cfa1ed1db52b077744fbaed9f4cd4f9c732035c5058634bdf36a4f5fa3d guix-build-c983612fb43b/output/aarch64-linux-gnu/bitcoin-c983612fb43b-aarch64-linux-gnu.tar.gz
f80800376c204970
...
(https://github.com/bitcoin/bitcoin/pull/32446#issuecomment-2862600446)
Guix Build:
```bash
876f676d8a0f348d8a8146b05d9206e8dffbd050269e27fcc41cf09273dfbc51 guix-build-c983612fb43b/output/aarch64-linux-gnu/SHA256SUMS.part
f47bfe426d480a0b7229be7b99f588bad1a5073cc9415cc99f62dd1aac560acf guix-build-c983612fb43b/output/aarch64-linux-gnu/bitcoin-c983612fb43b-aarch64-linux-gnu-debug.tar.gz
cda94cfa1ed1db52b077744fbaed9f4cd4f9c732035c5058634bdf36a4f5fa3d guix-build-c983612fb43b/output/aarch64-linux-gnu/bitcoin-c983612fb43b-aarch64-linux-gnu.tar.gz
f80800376c204970
...