🤔 fanquake reviewed a pull request: "Add release note for #32530"
(https://github.com/bitcoin/bitcoin/pull/32819#pullrequestreview-2966053187)
ACK 558f0880a8f374ab7a06bd829635a5e188bd8419
(https://github.com/bitcoin/bitcoin/pull/32819#pullrequestreview-2966053187)
ACK 558f0880a8f374ab7a06bd829635a5e188bd8419
🚀 fanquake merged a pull request: "Add release note for #32530"
(https://github.com/bitcoin/bitcoin/pull/32819)
(https://github.com/bitcoin/bitcoin/pull/32819)
💬 rkrux commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2171711817)
Good point, I have reworded to remove the word "output". I do want to let an example be present because this behaviour was not apparent to me when I first started using this RPC.
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2171711817)
Good point, I have reworded to remove the word "output". I do want to let an example be present because this behaviour was not apparent to me when I first started using this RPC.
💬 rkrux commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2171715691)
I have not used the term "logical/financial" to describe these transactions yet but I can add it if people believe that would be a valuable addition.
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2171715691)
I have not used the term "logical/financial" to describe these transactions yet but I can add it if people believe that would be a valuable addition.
💬 ismaelsadeeq commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2171735436)
I think just linking the PR without some explanation will not be that enough, so I instead link the issue comment with the comprehensive explanation, it will be easy to dig in up to that PR after clicking the current link in https://github.com/bitcoin/bitcoin/commit/9b75cfda4d62a0a3bde402503244dd57e1621a12.
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2171735436)
I think just linking the PR without some explanation will not be that enough, so I instead link the issue comment with the comprehensive explanation, it will be easy to dig in up to that PR after clicking the current link in https://github.com/bitcoin/bitcoin/commit/9b75cfda4d62a0a3bde402503244dd57e1621a12.
💬 rkrux commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3012715211)
> I don't get it. Watch-only wallets are treated the same as non-watch-only, aren't they?
In a way, they are.
> The available balance is what the wallet considers currently spendable
The current language^ in the RPC help made me curious regarding the balance shown for watch-only wallets because the watch-only wallets can't technically spend them. I checked that the balance shown in the RPC response is correct for such wallets, and this prompted me to add a note specifically for these wa
...
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3012715211)
> I don't get it. Watch-only wallets are treated the same as non-watch-only, aren't they?
In a way, they are.
> The available balance is what the wallet considers currently spendable
The current language^ in the RPC help made me curious regarding the balance shown for watch-only wallets because the watch-only wallets can't technically spend them. I checked that the balance shown in the RPC response is correct for such wallets, and this prompted me to add a note specifically for these wa
...
📝 zaidmstrr opened a pull request: "rpc: Handle -named argument parsing where Base64 encoding is used"
(https://github.com/bitcoin/bitcoin/pull/32821)
Addresses [comment](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628) and [this](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2092039999).
The [PR #31375](https://github.com/bitcoin/bitcoin/pull/31375) got merged and enables `-named` by default in the `bitcoin rpc` interface; `bitcoin rpc` corresponds to `bitcoin-cli -named` as it's just a wrapper. Now, the problem arises when we try to parse the positional paramater which might contain "=" as a Base64 charac
...
(https://github.com/bitcoin/bitcoin/pull/32821)
Addresses [comment](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628) and [this](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2092039999).
The [PR #31375](https://github.com/bitcoin/bitcoin/pull/31375) got merged and enables `-named` by default in the `bitcoin rpc` interface; `bitcoin rpc` corresponds to `bitcoin-cli -named` as it's just a wrapper. Now, the problem arises when we try to parse the positional paramater which might contain "=" as a Base64 charac
...
💬 luke-jr commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2171708556)
Does macOS not allow mounting subdirectories? (Maybe a bind mount) eg, maybe datadir isn't exFAT, but blocks is?
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2171708556)
Does macOS not allow mounting subdirectories? (Maybe a bind mount) eg, maybe datadir isn't exFAT, but blocks is?
💬 luke-jr commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2171706368)
What if it's datadir/blocks-alternative-name or something?
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2171706368)
What if it's datadir/blocks-alternative-name or something?
💬 maflcko commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171811426)
not sure about limiting this to base64. There are many other args that can hold values with an `=` in them. For example, labels.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171811426)
not sure about limiting this to base64. There are many other args that can hold values with an `=` in them. For example, labels.
💬 Sjors commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3012778370)
I ran into this issue in #32784 while editing the multisig tutorial.
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3012778370)
I ran into this issue in #32784 while editing the multisig tutorial.
🤔 hodlinator reviewed a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-2965833344)
Concept ACK be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3
Might be good to add release notes for this PR as it is adding to the user-facing RPC interface, similarly to: https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/release-notes-27826.md
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-2965833344)
Concept ACK be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3
Might be good to add release notes for this PR as it is adding to the user-facing RPC interface, similarly to: https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/release-notes-27826.md
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171711143)
Thanks for incorporating the feedback! These should be snake_case:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L392-L393
as per:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/developer-notes.md?plain=1#L89-L90
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171711143)
Thanks for incorporating the feedback! These should be snake_case:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L392-L393
as per:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/developer-notes.md?plain=1#L89-L90
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171692864)
Still doing it in these places:
https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L298
https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L1110
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171692864)
Still doing it in these places:
https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L298
https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L1110
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171513363)
Still seeing it in 83b6253eb3921a79231f450bb43e1977ea835b5e.
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171513363)
Still seeing it in 83b6253eb3921a79231f450bb43e1977ea835b5e.
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171528297)
Re ceiling - Ah, should have looked the symbols up. I'm fine either way, but would guess this is clearer for most programmers.
---
Sorry, didn't mean it so literally, maybe something like:
```diff
- Note: this is somewhat a retake of #27591
+ Note: this picks up work from the closed #27591.
```
I would put that together with the "Fixes #32775" at the bottom of the description, up to you.
---
You're still using `sigop count` in the formula where I think `sigop cost` would be more
...
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171528297)
Re ceiling - Ah, should have looked the symbols up. I'm fine either way, but would guess this is clearer for most programmers.
---
Sorry, didn't mean it so literally, maybe something like:
```diff
- Note: this is somewhat a retake of #27591
+ Note: this picks up work from the closed #27591.
```
I would put that together with the "Fixes #32775" at the bottom of the description, up to you.
---
You're still using `sigop count` in the formula where I think `sigop cost` would be more
...
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171769104)
Q1. Yeah, I think the interface is less confusing if we remove the condition. #27591 seems to use the term "sigop-adjusted size" rather than "sigop-equivalent size". So could change the description from:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L284
to:
"Sigop-adjusted size in bytes, only present for mempool transactions."
(Unless we implement Q2, which I agree is not critical).
---
Maybe replace "sigop-equivalent"
...
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171769104)
Q1. Yeah, I think the interface is less confusing if we remove the condition. #27591 seems to use the term "sigop-adjusted size" rather than "sigop-equivalent size". So could change the description from:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L284
to:
"Sigop-adjusted size in bytes, only present for mempool transactions."
(Unless we implement Q2, which I agree is not critical).
---
Maybe replace "sigop-equivalent"
...
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171533354)
Nice!
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2171533354)
Nice!
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171892100)
I think you are right; I will generalise it to any string args which might contain `=`
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171892100)
I think you are right; I will generalise it to any string args which might contain `=`
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3012839324)
Concept ACK 14e2125ee0297f0cb046ca91b6ead20052f24678. Thanks for the fix!
If possible it would be good to add python tests to check the new behavior and I also agree with [comment](https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171811426) that this change doesn't need to be limited to base64 parameters and could probably be extended to other string parameters. But maybe the "If parameter is unknown for a given known string method, then treat the whole string as positional" behavi
...
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3012839324)
Concept ACK 14e2125ee0297f0cb046ca91b6ead20052f24678. Thanks for the fix!
If possible it would be good to add python tests to check the new behavior and I also agree with [comment](https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171811426) that this change doesn't need to be limited to base64 parameters and could probably be extended to other string parameters. But maybe the "If parameter is unknown for a given known string method, then treat the whole string as positional" behavi
...