💬 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
...
🤔 l0rinc requested changes to a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2965729639)
Concept ACK
Regardless of whether the soft-fork will be accepted, it's important to guard against this early.
However, the current implementation introduces a significant slowdown and I think we could make the testing more solid by explicitly validating that we didn't soft-fork yet.
Therefore it's an Approach NACK from me, please see the detailed explanation below.
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2965729639)
Concept ACK
Regardless of whether the soft-fork will be accepted, it's important to guard against this early.
However, the current implementation introduces a significant slowdown and I think we could make the testing more solid by explicitly validating that we didn't soft-fork yet.
Therefore it's an Approach NACK from me, please see the detailed explanation below.
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171446494)
Seems wasteful to iterate and parse `tx.vin[i].scriptSig` twice, we should already know the last item that the `scriptSig` pushed onto the stack after the first iteration - can we optimize that?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171446494)
Seems wasteful to iterate and parse `tx.vin[i].scriptSig` twice, we should already know the last item that the `scriptSig` pushed onto the stack after the first iteration - can we optimize that?
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171603715)
similarly to other `emplace_back` calls here, we don't need the explicit constructor (it avoids constructing a temporary):
```suggestion
tx_max_sigops.vin.emplace_back(prev_txid, i);
```
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171603715)
similarly to other `emplace_back` calls here, we don't need the explicit constructor (it avoids constructing a temporary):
```suggestion
tx_max_sigops.vin.emplace_back(prev_txid, i);
```
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171450864)
Note: since this magic value is referencing a soft-fork that may or may not be applied, we could mention it here for context: https://github.com/bitcoin/bips/blob/master/bip-0054.md#specification
Q: How often do we expect mined blocks to contradict this new policy rule?
Asking because of https://b10c.me/observations/11-invalid-blocks-783426-and-784121, claiming:
> On April 1st, 2023, F2Pool mined an invalid block at height 783426. Bitcoin Core nodes rejected the block with the reason bad-bl
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171450864)
Note: since this magic value is referencing a soft-fork that may or may not be applied, we could mention it here for context: https://github.com/bitcoin/bips/blob/master/bip-0054.md#specification
Q: How often do we expect mined blocks to contradict this new policy rule?
Asking because of https://b10c.me/observations/11-invalid-blocks-783426-and-784121, claiming:
> On April 1st, 2023, F2Pool mined an invalid block at height 783426. Bitcoin Core nodes rejected the block with the reason bad-bl
...
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171492254)
nit: multiple typos in commit message 27e54beff7d1c9ac68bee379bb6d971a775b9841
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2171492254)
nit: multiple typos in commit message 27e54beff7d1c9ac68bee379bb6d971a775b9841