💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164245384)
might make sense to extract genesis hash here as well like we did in `HappyPath`
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164245384)
might make sense to extract genesis hash here as well like we did in `HappyPath`
💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164207447)
> makes it more likely that the number changes between releases
That sounds like a counter-argument to me, but don't have strong feelings about it, if you disagree, I don't mind, please resolve it.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164207447)
> makes it more likely that the number changes between releases
That sounds like a counter-argument to me, but don't have strong feelings about it, if you disagree, I don't mind, please resolve it.
💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164253700)
Can we add 2 helpers in that case? I really dislike that the comments don't follow the code's structure :.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164253700)
Can we add 2 helpers in that case? I really dislike that the comments don't follow the code's structure :.
💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164260873)
> chunks of code to minimize diffs
We've rewritten the tests at this stage, diffs and the function prototypes don't help in my opinion.
But if you insist, I don't mind, please resolve the comment
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164260873)
> chunks of code to minimize diffs
We've rewritten the tests at this stage, diffs and the function prototypes don't help in my opinion.
But if you insist, I don't mind, please resolve the comment
💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164267943)
Not sure I understand, especially in the new code which states something like `locator == genesis` - which is basically the same as the comment
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164267943)
Not sure I understand, especially in the new code which states something like `locator == genesis` - which is basically the same as the comment
💬 polespinasa commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-3000887231)
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Nothing to add to the comments already done.
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-3000887231)
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Nothing to add to the comments already done.
🤔 janb84 reviewed a pull request: "build: add root dir to CMAKE_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2954293190)
reACK e27a94596f2a1f5e04722a16165717cc6e891d36
Changes sinds last ACK:
- CMAKE_SYSTEM_PREFIX_PATH -> CMAKE_PREFIX_PATH
Validated that change still solve problem ✅
(https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2954293190)
reACK e27a94596f2a1f5e04722a16165717cc6e891d36
Changes sinds last ACK:
- CMAKE_SYSTEM_PREFIX_PATH -> CMAKE_PREFIX_PATH
Validated that change still solve problem ✅
📝 Prabhat1308 opened a pull request: "test: add functional test for upgradewallet rpc"
(https://github.com/bitcoin/bitcoin/pull/32803)
followups up from https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2875411238 to add coverage for `upgradewallet` rpc. PR adds 2 test cases , trying to upgrade from the latest version and asserting it stays the same and trying to downgrade from the latest version. Test case to actually upgrade is not possible currently since the `createwallet` rpc by default creates wallet with version `FEATURE_LATEST` which is the highest.
(https://github.com/bitcoin/bitcoin/pull/32803)
followups up from https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2875411238 to add coverage for `upgradewallet` rpc. PR adds 2 test cases , trying to upgrade from the latest version and asserting it stays the same and trying to downgrade from the latest version. Test case to actually upgrade is not possible currently since the `createwallet` rpc by default creates wallet with version `FEATURE_LATEST` which is the highest.
💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164322315)
ok taken this, thanks.
new constructor is
```cpp
explicit HTTPRPCTimerInterface(const std::any& context)
: m_context{*Assert(std::any_cast<NodeContext*>(context))}
```
so its like "passed in an `any` which i convert to a `NodeContext` pointer which I assert is not null, then de-reference back to a `NodeContext` and store that as a reference in the class
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164322315)
ok taken this, thanks.
new constructor is
```cpp
explicit HTTPRPCTimerInterface(const std::any& context)
: m_context{*Assert(std::any_cast<NodeContext*>(context))}
```
so its like "passed in an `any` which i convert to a `NodeContext` pointer which I assert is not null, then de-reference back to a `NodeContext` and store that as a reference in the class
💬 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).