💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447648863)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Think this should also mention that responses contain "jsonrpc": "2.0" field. Or this could be an additional row in the table.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447648863)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Think this should also mention that responses contain "jsonrpc": "2.0" field. Or this could be an additional row in the table.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447616571)
In commit "test: cover more HTTP error codes in interface_rpc.py" (29ebb0d7774ee2d10aa274086e8ff4b2395f7047)
Setting params to None seems like a bug because it means a `"params": null` JSON field will be sent, when previously authproxy would send an empty params object. Would suggest changing this to `params={}` to avoid a change in behavior.
Also, I'm not sure if it is a bug for our server to accept null params. The spec says "If present, parameters for the rpc call MUST be provided as a
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447616571)
In commit "test: cover more HTTP error codes in interface_rpc.py" (29ebb0d7774ee2d10aa274086e8ff4b2395f7047)
Setting params to None seems like a bug because it means a `"params": null` JSON field will be sent, when previously authproxy would send an empty params object. Would suggest changing this to `params={}` to avoid a change in behavior.
Also, I'm not sure if it is a bug for our server to accept null params. The spec says "If present, parameters for the rpc call MUST be provided as a
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447662808)
In commit "rpc: JSON-RPC 2.0 should not respond to "notifications"" (bc2d55898b315d1ee80a70377d65f79c829246ed)
This does not seem correct. According to https://www.jsonrpc.org/specification#notification a notification is a 'Request object without an "id" member', not a request object with a `null` id member. It also explicitly says requests that have "Null as a value for the id member" are treated as "Responses with an unknown id" not notifications.
Probably a reasonable way to fix this wo
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447662808)
In commit "rpc: JSON-RPC 2.0 should not respond to "notifications"" (bc2d55898b315d1ee80a70377d65f79c829246ed)
This does not seem correct. According to https://www.jsonrpc.org/specification#notification a notification is a 'Request object without an "id" member', not a request object with a `null` id member. It also explicitly says requests that have "Null as a value for the id member" are treated as "Responses with an unknown id" not notifications.
Probably a reasonable way to fix this wo
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447645890)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Would drop "Protocols can be mixed in a batch request." It seems ok, but not a good thing to encourage, and unless it's required by the specs, I don't think we should commit to this behavior by adding it to our documentation.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447645890)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Would drop "Protocols can be mixed in a batch request." It seems ok, but not a good thing to encourage, and unless it's required by the specs, I don't think we should commit to this behavior by adding it to our documentation.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447684268)
In commit "test: handle 204 NO_CONTENT correctly in authproxy" (174a995edf594dccf745d2593c281d3e4a1ff7f7)
It would be good to have a comment that explains why this block of code is needed, because it seems like an impossible case to hit through normal usage of authproxy. Maybe: "# Check for no-content HTTP status code, which can be returned when an RPC client requests a JSON-RPC 2.0 "notification" with no response. Currently this is only possible if clients call the _request() method directly
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447684268)
In commit "test: handle 204 NO_CONTENT correctly in authproxy" (174a995edf594dccf745d2593c281d3e4a1ff7f7)
It would be good to have a comment that explains why this block of code is needed, because it seems like an impossible case to hit through normal usage of authproxy. Maybe: "# Check for no-content HTTP status code, which can be returned when an RPC client requests a JSON-RPC 2.0 "notification" with no response. Currently this is only possible if clients call the _request() method directly
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447679340)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Maybe replace "errors like if..." with "errors in cases like the endpoint not being found (404) or the RPC request not being formatted correctly (500)." Grammar sounds a little off the way it is written now.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447679340)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Maybe replace "errors like if..." with "errors in cases like the endpoint not being found (404) or the RPC request not being formatted correctly (500)." Grammar sounds a little off the way it is written now.
👍 ryanofsky approved a pull request: "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1813747756)
Code review ACK b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2. Looks great! Just suggested changes since last review
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1813747756)
Code review ACK b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2. Looks great! Just suggested changes since last review
👍 brunoerg approved a pull request: "fuzz: Improve fuzzing stability for ellswift_roundtrip harness"
(https://github.com/bitcoin/bitcoin/pull/29219#pullrequestreview-1813766726)
crACK 154fcce55c84c251fad8d280eafb3c0a5284fcd4
(https://github.com/bitcoin/bitcoin/pull/29219#pullrequestreview-1813766726)
crACK 154fcce55c84c251fad8d280eafb3c0a5284fcd4
💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1885310514)
Pushed three additional commits to my branch that may make a dent in the CI issues:
- make `fs::path` hashable: https://github.com/jamesob/bitcoin/commit/8f5fdf8ad3d5102e4afaae415e004d4cf6c667cc
- avoid use of `std::filesystem::path` where possible: https://github.com/jamesob/bitcoin/commit/4ef785749ff1c481b5681739653c5b153637e44e
- update linter for mockable filesystem ops: https://github.com/jamesob/bitcoin/commit/face876d55711299c2679aad59b24ee04aa892f0
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1885310514)
Pushed three additional commits to my branch that may make a dent in the CI issues:
- make `fs::path` hashable: https://github.com/jamesob/bitcoin/commit/8f5fdf8ad3d5102e4afaae415e004d4cf6c667cc
- avoid use of `std::filesystem::path` where possible: https://github.com/jamesob/bitcoin/commit/4ef785749ff1c481b5681739653c5b153637e44e
- update linter for mockable filesystem ops: https://github.com/jamesob/bitcoin/commit/face876d55711299c2679aad59b24ee04aa892f0
👍 theStack approved a pull request: "test: add assumeutxo wallet test"
(https://github.com/bitcoin/bitcoin/pull/28838#pullrequestreview-1813788224)
Code-review ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
Also ran the test locally.
(https://github.com/bitcoin/bitcoin/pull/28838#pullrequestreview-1813788224)
Code-review ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
Also ran the test locally.
💬 theStack commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1447730295)
nit: looks like an unintended newline here?
```suggestion
assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)
```
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1447730295)
nit: looks like an unintended newline here?
```suggestion
assert_equal(n.getblockchaininfo()["headers"], SNAPSHOT_BASE_HEIGHT)
```
⚠️ glozow opened an issue: "`-maxtxfee` is used as a fee and a feerate"
(https://github.com/bitcoin/bitcoin/issues/29220)
### Please describe the feature you'd like to see added.
The `-maxtxfee` (which is put into `m_default_max_tx_fee` is documented as "the maximum total fees to use in a single wallet transaction"
https://github.com/bitcoin/bitcoin/blob/632a2bb731804dffe52bd4cbd90bfee352d25ede/src/wallet/init.cpp#L63
This seems true in many instances when we look at the code:
https://github.com/bitcoin/bitcoin/blob/632a2bb731804dffe52bd4cbd90bfee352d25ede/src/wallet/spend.cpp#L1293-L1295
https://github.com/
...
(https://github.com/bitcoin/bitcoin/issues/29220)
### Please describe the feature you'd like to see added.
The `-maxtxfee` (which is put into `m_default_max_tx_fee` is documented as "the maximum total fees to use in a single wallet transaction"
https://github.com/bitcoin/bitcoin/blob/632a2bb731804dffe52bd4cbd90bfee352d25ede/src/wallet/init.cpp#L63
This seems true in many instances when we look at the code:
https://github.com/bitcoin/bitcoin/blob/632a2bb731804dffe52bd4cbd90bfee352d25ede/src/wallet/spend.cpp#L1293-L1295
https://github.com/
...
💬 theuni commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885372563)
> Would still like to see them PR'd separately, but I cherry-picked the two relevant commits, dropped `native_cmake`, and have dropped anything libtool related here.
It's been quite a while since I worked on those, I don't remember if they were completely merge-ready (I suspect windows cross might be broken for ex.). Playing with them locally now.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885372563)
> Would still like to see them PR'd separately, but I cherry-picked the two relevant commits, dropped `native_cmake`, and have dropped anything libtool related here.
It's been quite a while since I worked on those, I don't remember if they were completely merge-ready (I suspect windows cross might be broken for ex.). Playing with them locally now.
💬 achow101 commented on pull request "Add Gutter Guard Selector":
(https://github.com/bitcoin/bitcoin/pull/28977#issuecomment-1885378443)
Is there a benefit to using SRD here over something oldest first? We are already doing SRD so it might be better to use a different underlying strategy. Having a selection based on oldest first would also be useful for consuming negative EV UTXOs at very low feerates, e.g. less than 3 s/vb.
It also seems like having the limit be inversely scaled by feerate would be better than the fixed limit. That way, as the feerates increase, we get less consolidatory instead of current behavior where it s
...
(https://github.com/bitcoin/bitcoin/pull/28977#issuecomment-1885378443)
Is there a benefit to using SRD here over something oldest first? We are already doing SRD so it might be better to use a different underlying strategy. Having a selection based on oldest first would also be useful for consuming negative EV UTXOs at very low feerates, e.g. less than 3 s/vb.
It also seems like having the limit be inversely scaled by feerate would be better than the fixed limit. That way, as the feerates increase, we get less consolidatory instead of current behavior where it s
...
📝 xonx4l opened a pull request: "FIX:When opening or autoloading wallets there should be clear messages about rescanning in progress and wallets' names."
(https://github.com/bitcoin-core/gui/pull/786)
issue-: #259
The "Opening Wallet" popup would now show "Opening Wallet: " instead of just "Opening Wallet". This identifies the specific wallet.
The "Rescanning" popup would show "Rescanning: " or "Rescanning - 0%" if a progress bar is added. Again, this identifies the wallet.
The main window title would include the wallet name after being opened/loaded thanks to the change in WalletView::showNormalIfMinimized().
Other dialogs like Send/Receive coins would also show the wallet name i
...
(https://github.com/bitcoin-core/gui/pull/786)
issue-: #259
The "Opening Wallet" popup would now show "Opening Wallet: " instead of just "Opening Wallet". This identifies the specific wallet.
The "Rescanning" popup would show "Rescanning: " or "Rescanning - 0%" if a progress bar is added. Again, this identifies the wallet.
The main window title would include the wallet name after being opened/loaded thanks to the change in WalletView::showNormalIfMinimized().
Other dialogs like Send/Receive coins would also show the wallet name i
...
💬 m3dwards commented on pull request "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256":
(https://github.com/bitcoin/bitcoin/pull/29180#issuecomment-1885402883)
I've reviewed the changes and they appear correct to me. libbitcoinconsensus will continue to not get SSE4 ASM (nor any other optimisations) and libbitcoinkernel (external?) will now get all of them (if available and enabled by ./configure).
I think `DISABLE_OPTIMIZED_SHA256` is much clearer macro name. I find the `USE_ASM` symbol / macro and `asm` feature in autoconf confusing.
If I've read it correctly, the `ASM` feature in autoconf enables not only the assembly optimisations but also st
...
(https://github.com/bitcoin/bitcoin/pull/29180#issuecomment-1885402883)
I've reviewed the changes and they appear correct to me. libbitcoinconsensus will continue to not get SSE4 ASM (nor any other optimisations) and libbitcoinkernel (external?) will now get all of them (if available and enabled by ./configure).
I think `DISABLE_OPTIMIZED_SHA256` is much clearer macro name. I find the `USE_ASM` symbol / macro and `asm` feature in autoconf confusing.
If I've read it correctly, the `ASM` feature in autoconf enables not only the assembly optimisations but also st
...
🤔 pablomartin4btc reviewed a pull request: "test: detect OS in functional tests consistently using `platform.system()`"
(https://github.com/bitcoin/bitcoin/pull/29034#pullrequestreview-1813903714)
tACK 878d914777a03a04ecb84217152e8b7fd73a5062
Tested on both Linux (Ubuntu 22.04 & WSL) and Windows.
As @kevkevinpal [pointed it out](https://github.com/bitcoin/bitcoin/pull/29034#issuecomment-1847703106), there's still usage of `os.name` in `test_framework.py`, not sure if you want to cover that as well.
(https://github.com/bitcoin/bitcoin/pull/29034#pullrequestreview-1813903714)
tACK 878d914777a03a04ecb84217152e8b7fd73a5062
Tested on both Linux (Ubuntu 22.04 & WSL) and Windows.
As @kevkevinpal [pointed it out](https://github.com/bitcoin/bitcoin/pull/29034#issuecomment-1847703106), there's still usage of `os.name` in `test_framework.py`, not sure if you want to cover that as well.
💬 achow101 commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1885446023)
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
I also prefer the use of macros/functions for the different categories as it is much less verbose than `LogPrintLevel(CAT, LEVEL, ...)`. This pattern is also similar to what many other logging frameworks do, e.g. python's `logging` module.
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1885446023)
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
I also prefer the use of macros/functions for the different categories as it is much less verbose than `LogPrintLevel(CAT, LEVEL, ...)`. This pattern is also similar to what many other logging frameworks do, e.g. python's `logging` module.
💬 achow101 commented on pull request "test: wallet migration, add coverage for tx extra data":
(https://github.com/bitcoin/bitcoin/pull/29204#issuecomment-1885450376)
ACK 016cc807f77a9128d430a0df1edd133521628a33
(https://github.com/bitcoin/bitcoin/pull/29204#issuecomment-1885450376)
ACK 016cc807f77a9128d430a0df1edd133521628a33
💬 theuni commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885470222)
> > Would still like to see them PR'd separately, but I cherry-picked the two relevant commits, dropped `native_cmake`, and have dropped anything libtool related here.
>
> It's been quite a while since I worked on those, I don't remember if they were completely merge-ready (I suspect windows cross might be broken for ex.). Playing with them locally now.
Here's a cleaned up version: https://github.com/theuni/bitcoin/commits/depends-no-libtool/
They needed an update after 63c0c4ff10b2f6ed85
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885470222)
> > Would still like to see them PR'd separately, but I cherry-picked the two relevant commits, dropped `native_cmake`, and have dropped anything libtool related here.
>
> It's been quite a while since I worked on those, I don't remember if they were completely merge-ready (I suspect windows cross might be broken for ex.). Playing with them locally now.
Here's a cleaned up version: https://github.com/theuni/bitcoin/commits/depends-no-libtool/
They needed an update after 63c0c4ff10b2f6ed85
...