💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569335260)
ACK 2808c33bae95a0d2516a6e9a550032af142a04de
I find `translateNamedArguments` somewhat difficult to read, but it seems like there isn't really much that can be done to make it easier to comprehend without completely overhauling it. I've ended up doing that in #27788.
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569335260)
ACK 2808c33bae95a0d2516a6e9a550032af142a04de
I find `translateNamedArguments` somewhat difficult to read, but it seems like there isn't really much that can be done to make it easier to comprehend without completely overhauling it. I've ended up doing that in #27788.
📝 achow101 converted_to_draft a pull request: "rpc: Be able to access RPC parameters by name"
(https://github.com/bitcoin/bitcoin/pull/27788)
Although we accept named RPC parameters, we still access the parameters by position within the RPC implementations. This PR makes it possible to access these parameters directly by name. This is achieved by holding the parameters in a separate `JSONRPCParameters` class which contains mappings from both name to value, and position to value. Two `operator[]` methods are implemented which can accept both strings and ints. This allows for backwards compatibility, while also allowing for future imple
...
(https://github.com/bitcoin/bitcoin/pull/27788)
Although we accept named RPC parameters, we still access the parameters by position within the RPC implementations. This PR makes it possible to access these parameters directly by name. This is achieved by holding the parameters in a separate `JSONRPCParameters` class which contains mappings from both name to value, and position to value. Two `operator[]` methods are implemented which can accept both strings and ints. This allows for backwards compatibility, while also allowing for future imple
...
💬 theStack commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1210987152)
Regarding the goal of simple implementation, is there a strong need to keep the `FE.is_square` method? Right now this is the only call-site, which I think could be replaced by
```suggestion
return (FE(x)**3 + 7).sqrt() is not None
```
I assume trying to actually square is way slower than computing the Jacobi symbol, but since `is_valid_x` doesn't appear to be in a critical code-path, that's probably fine? At least for `feature_taproot.py` I didn't see a decrease in performance.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1210987152)
Regarding the goal of simple implementation, is there a strong need to keep the `FE.is_square` method? Right now this is the only call-site, which I think could be replaced by
```suggestion
return (FE(x)**3 + 7).sqrt() is not None
```
I assume trying to actually square is way slower than computing the Jacobi symbol, but since `is_valid_x` doesn't appear to be in a critical code-path, that's probably fine? At least for `feature_taproot.py` I didn't see a decrease in performance.
💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569405783)
reACK 2808c33bae95a0d2516a6e9a550032af142a04de
(I wouldn't expect bitcoin-cli to convert the types)
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569405783)
reACK 2808c33bae95a0d2516a6e9a550032af142a04de
(I wouldn't expect bitcoin-cli to convert the types)
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031592)
Moved the `Assume` to `LoadRecords`.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031592)
Moved the `Assume` to `LoadRecords`.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031688)
No, added the exception handling back.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031688)
No, added the exception handling back.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031768)
Added it back.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031768)
Added it back.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031914)
Don't remember, undone. Probably a bad rebase?
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031914)
Don't remember, undone. Probably a bad rebase?
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032001)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032001)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032031)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032031)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032083)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032083)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032129)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032129)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032152)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032152)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032186)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032186)
Done as suggested.
📝 achow101 opened a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790)
Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917
This PR adds a database cursor that gives a view over all of the records beginning with the same prefix.
(https://github.com/bitcoin/bitcoin/pull/27790)
Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917
This PR adds a database cursor that gives a view over all of the records beginning with the same prefix.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1569422995)
> 1. The main thing would be making the "walletdb: refactor X loading" commits self contained and not leave behind dead code that needs to be cleaned up later. It's hard to identify what code is being replaced in current commits when not all the code being replaced is actually removed.
I've implemented your suggestions and some additional dead code cleanups.
> 2. I think it might be good to move first 2 commits up to "wallet: Add GetPrefixCursor to DatabaseBatch" ([422a843](https://git
...
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1569422995)
> 1. The main thing would be making the "walletdb: refactor X loading" commits self contained and not leave behind dead code that needs to be cleaned up later. It's hard to identify what code is being replaced in current commits when not all the code being replaced is actually removed.
I've implemented your suggestions and some additional dead code cleanups.
> 2. I think it might be good to move first 2 commits up to "wallet: Add GetPrefixCursor to DatabaseBatch" ([422a843](https://git
...
📝 achow101 converted_to_draft a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914)
Currently when we load a wallet, we just iterate through all of the records in the database and add them completely statelessly. However we have some records which do rely on other records being loaded before they are. To deal with this, we use `CWalletScanState` to hold things temporarily until all of the records have been read and then we load the stateful things.
However this can be slow, and with some future improvements, can cause some pretty drastic slowdowns to retain this pattern. So
...
(https://github.com/bitcoin/bitcoin/pull/24914)
Currently when we load a wallet, we just iterate through all of the records in the database and add them completely statelessly. However we have some records which do rely on other records being loaded before they are. To deal with this, we use `CWalletScanState` to hold things temporarily until all of the records have been read and then we load the stateful things.
However this can be slow, and with some future improvements, can cause some pretty drastic slowdowns to retain this pattern. So
...
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569426939)
> (I wouldn't expect bitcoin-cli to convert the types)
Then this ends up being unusable, e.g.:
```
$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
error code: -3
error message:
JSON value of type null is not of expected type string
```
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569426939)
> (I wouldn't expect bitcoin-cli to convert the types)
Then this ends up being unusable, e.g.:
```
$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
error code: -3
error message:
JSON value of type null is not of expected type string
```
💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569448946)
> > (I wouldn't expect bitcoin-cli to convert the types)
>
> Then this ends up being fairly useless, e.g.:
>
> ```
> $ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
> error code: -3
> error message:
> JSON value of type null is not of expected type string
> ```
Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
...
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569448946)
> > (I wouldn't expect bitcoin-cli to convert the types)
>
> Then this ends up being fairly useless, e.g.:
>
> ```
> $ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
> error code: -3
> error message:
> JSON value of type null is not of expected type string
> ```
Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
...
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569453818)
> Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
Oops, yes, I don't think I was on the HEAD commit.
> Yeah, that does suck, and I don't see an obvious fix -- presumably we'd want named-only parameters to be treated as json rather than a string (unless overridden), but if it's not overridden, then bitcoin-cli has no way of knowing if `change_position` here is a positional param or a named one...
...
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569453818)
> Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
Oops, yes, I don't think I was on the HEAD commit.
> Yeah, that does suck, and I don't see an obvious fix -- presumably we'd want named-only parameters to be treated as json rather than a string (unless overridden), but if it's not overridden, then bitcoin-cli has no way of knowing if `change_position` here is a positional param or a named one...
...