💬 ryanofsky commented on issue "IPC via TCP Sockets":
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3000963484)
The reason the `ipcbind` option doesn't support TCP currently is that there's no authentication, so listening on a TCP socket would let any process on the system running as any user connect to the port, and potentially access wallets or take advantage of privileges the bitcoin process has.
So currently:
- ssh provides a reasonable way to forward sockets with permissions and authentication (`ssh -L /tmp/local.sock:/path/to/remote.sock user@remotehost` or `ssh -R /tmp/remote.sock:/path/to/local.
...
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3000963484)
The reason the `ipcbind` option doesn't support TCP currently is that there's no authentication, so listening on a TCP socket would let any process on the system running as any user connect to the port, and potentially access wallets or take advantage of privileges the bitcoin process has.
So currently:
- ssh provides a reasonable way to forward sockets with permissions and authentication (`ssh -L /tmp/local.sock:/path/to/remote.sock user@remotehost` or `ssh -R /tmp/remote.sock:/path/to/local.
...
💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164325336)
Good idea, but would that reduce confidence in the test coverage for `RPCRunLater()` etc?
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164325336)
Good idea, but would that reduce confidence in the test coverage for `RPCRunLater()` etc?
💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2164329006)
See https://github.com/asmap/asmap-data/issues/25
The repository description there says:
> This repository is strictly used for demonstration purposes to help with conceptual discussions about ASMap in the Bitcoin Core release process. Any data uploaded here should be treated with extreme caution and is not inteded for production use.
which doesn't seem to match the expectations around the repository as used here.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2164329006)
See https://github.com/asmap/asmap-data/issues/25
The repository description there says:
> This repository is strictly used for demonstration purposes to help with conceptual discussions about ASMap in the Bitcoin Core release process. Any data uploaded here should be treated with extreme caution and is not inteded for production use.
which doesn't seem to match the expectations around the repository as used here.
💬 b-l-u-e commented on issue "getdescriptorinfo returns unusable descriptor":
(https://github.com/bitcoin/bitcoin/issues/29320#issuecomment-3000977049)
I can confirm this issue has been resolved in the current Bitcoin Core v28.0.0.
**Test Results with current Bitcoin Core:**
- Private descriptor: `wpkh(tprv8ZgxMBicQKsPex2iyDeZBBVUEXgmM2AaQMCXS3Ea6n4Jq7GBH7WXKD6svcVkheMiUHRN55CDmHD5VBamQdeHYfdyy7H4Bpox98dB4y5EdiZ/84h/1h/0h/1/*)#y3f8rvu5`
- getdescriptorinfo now returns: `wpkh([6ec4a9d6/84h/1h/0h]tpubDC2eVxnwvoKRpatNU2E3kB8W1v12AcVvtE4M525uiPF1dWuQhLnjNSVJEX5CeAk8gfm6fV9Rmu6LvVzGP2fPr1X9JfDPGz4hToKFbcGBD32/1/*)#0k803cul`
- deriveaddresses work
...
(https://github.com/bitcoin/bitcoin/issues/29320#issuecomment-3000977049)
I can confirm this issue has been resolved in the current Bitcoin Core v28.0.0.
**Test Results with current Bitcoin Core:**
- Private descriptor: `wpkh(tprv8ZgxMBicQKsPex2iyDeZBBVUEXgmM2AaQMCXS3Ea6n4Jq7GBH7WXKD6svcVkheMiUHRN55CDmHD5VBamQdeHYfdyy7H4Bpox98dB4y5EdiZ/84h/1h/0h/1/*)#y3f8rvu5`
- getdescriptorinfo now returns: `wpkh([6ec4a9d6/84h/1h/0h]tpubDC2eVxnwvoKRpatNU2E3kB8W1v12AcVvtE4M525uiPF1dWuQhLnjNSVJEX5CeAk8gfm6fV9Rmu6LvVzGP2fPr1X9JfDPGz4hToKFbcGBD32/1/*)#0k803cul`
- deriveaddresses work
...
💬 maflcko commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3000987189)
are there any plans that the rpc will be used anytime soon in the next couple of years? If not, it could make sense to just remove it, and add it back, in the unlikely case it will be used?
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3000987189)
are there any plans that the rpc will be used anytime soon in the next couple of years? If not, it could make sense to just remove it, and add it back, in the unlikely case it will be used?
💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2164346212)
I agree and think this can trivially be made generic in the future if needed, but I don't see the need now?
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2164346212)
I agree and think this can trivially be made generic in the future if needed, but I don't see the need now?
💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2164345058)
Not going to take this for now as I think I need to create and use a stream to read into? two backslashes feels easier IMO. :)
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2164345058)
Not going to take this for now as I think I need to create and use a stream to read into? two backslashes feels easier IMO. :)
💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2164342630)
done in 2ad18fc9784e6a37e453bf844ba10c4e46f54754
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2164342630)
done in 2ad18fc9784e6a37e453bf844ba10c4e46f54754
💬 maflcko commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3001049953)
thx for the test
review ACK 003a3cdb29dcc1e3e1a53f8227de73389071fbd1 📝
<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 RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: rev
...
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3001049953)
thx for the test
review ACK 003a3cdb29dcc1e3e1a53f8227de73389071fbd1 📝
<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 RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: rev
...
💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3001064611)
re-ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675 💱
<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 RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK e70c2087b00f6cfe1a95
...
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3001064611)
re-ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675 💱
<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 RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK e70c2087b00f6cfe1a95
...
🤔 pablomartin4btc reviewed a pull request: "doc: Add fetching single PRs from upstream to productivity.md"
(https://github.com/bitcoin/bitcoin/pull/32783#pullrequestreview-2954506359)
ACK 83ccae194d0de3e64af54174898f030594be3a4f
In previous section ("Reference PRs easily with `refspec`s"), also touched recently in #32774, it could be mentioned that the user could use `git remove add`... instead of adding the remote manually into the git config file.
(https://github.com/bitcoin/bitcoin/pull/32783#pullrequestreview-2954506359)
ACK 83ccae194d0de3e64af54174898f030594be3a4f
In previous section ("Reference PRs easily with `refspec`s"), also touched recently in #32774, it could be mentioned that the user could use `git remove add`... instead of adding the remote manually into the git config file.
💬 pablomartin4btc commented on pull request "doc: Add fetching single PRs from upstream to productivity.md":
(https://github.com/bitcoin/bitcoin/pull/32783#discussion_r2164412160)
nit: looking at the comments around remotes, perhaps you can mention somewhere/ at the end of this note that users could check their setup with `git remote -v` (and maybe as you said, `git clone` adds the origin automatically - keeping in mind what you also said that this is not a git guide).
(https://github.com/bitcoin/bitcoin/pull/32783#discussion_r2164412160)
nit: looking at the comments around remotes, perhaps you can mention somewhere/ at the end of this note that users could check their setup with `git remote -v` (and maybe as you said, `git clone` adds the origin automatically - keeping in mind what you also said that this is not a git guide).
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3001109067)
> I haven't reviewed in depth, but I don't think the children are supposed to be mined and then reorged, only the parent is.
Agree! I shall change the test to match this!
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3001109067)
> I haven't reviewed in depth, but I don't think the children are supposed to be mined and then reorged, only the parent is.
Agree! I shall change the test to match this!
💬 Prabhat1308 commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3001128290)
> are there any plans that the rpc will be used anytime soon in the next couple of years? If not, it could make sense to just remove it, and add it back, in the unlikely case it will be used?
Looking through the code this is pretty much a dead rpc where no one can use it to actually upgrade wallet and there is only 1 check that `multiwallet` can't call it before this PR . I am not very sure about what the future plans are for wallet but the only case where I see this being used is if there is
...
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3001128290)
> are there any plans that the rpc will be used anytime soon in the next couple of years? If not, it could make sense to just remove it, and add it back, in the unlikely case it will be used?
Looking through the code this is pretty much a dead rpc where no one can use it to actually upgrade wallet and there is only 1 check that `multiwallet` can't call it before this PR . I am not very sure about what the future plans are for wallet but the only case where I see this being used is if there is
...
💬 fanquake commented on pull request "build: add root dir to CMAKE_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3001155413)
If we ship this, it should also be backported to `29.x`.
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3001155413)
If we ship this, it should also be backported to `29.x`.
📝 hebasto opened a pull request: "Fix build on macOS when `qt@6` is installed"
(https://github.com/bitcoin/bitcoin/pull/32804)
Fix https://github.com/bitcoin/bitcoin/issues/31009.
(https://github.com/bitcoin/bitcoin/pull/32804)
Fix https://github.com/bitcoin/bitcoin/issues/31009.
💬 hebasto commented on issue "29.x Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-3001185852)
Mind testing https://github.com/bitcoin/bitcoin/pull/32804?
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-3001185852)
Mind testing https://github.com/bitcoin/bitcoin/pull/32804?
🤔 furszy reviewed a pull request: "wallet: remove dead code in legacy wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32758#pullrequestreview-2954618622)
Concept ACK
I think the second commit would look nicer and simpler after something like e86d71b749c08bde6002b9aa2baee824975a518a (which is part of #31423). We’d be able to inline the function at that point.
(https://github.com/bitcoin/bitcoin/pull/32758#pullrequestreview-2954618622)
Concept ACK
I think the second commit would look nicer and simpler after something like e86d71b749c08bde6002b9aa2baee824975a518a (which is part of #31423). We’d be able to inline the function at that point.
👍 instagibbs approved a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32589#pullrequestreview-2954625132)
ACK 0922f6bbc33ac2abe3f3d9dc98dade896718864f
(https://github.com/bitcoin/bitcoin/pull/32589#pullrequestreview-2954625132)
ACK 0922f6bbc33ac2abe3f3d9dc98dade896718864f
💬 josibake commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3001202596)
At the risk of further derailing this thread from FreeBSD and OpenBSD (sorry!), I also ran into a similar problem with not being able to override the compiler in depends. TLDR; built a nix dev environment that includes all llvm tooling (no gcc, g++) and was trying to do a depends build. This fails like so:
```
make -C depends NO_QT=1 MULTIPROCESS=1
make: Entering directory '/home/josie/bitcoin/depends'
Extracting native_capnp...
/home/josie/bitcoin/depends/sources/capnproto-cxx-1.2.0.tar.
...
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3001202596)
At the risk of further derailing this thread from FreeBSD and OpenBSD (sorry!), I also ran into a similar problem with not being able to override the compiler in depends. TLDR; built a nix dev environment that includes all llvm tooling (no gcc, g++) and was trying to do a depends build. This fails like so:
```
make -C depends NO_QT=1 MULTIPROCESS=1
make: Entering directory '/home/josie/bitcoin/depends'
Extracting native_capnp...
/home/josie/bitcoin/depends/sources/capnproto-cxx-1.2.0.tar.
...