💬 ryanofsky commented on pull request "wallet, rpc: add BIP44 `account` in `createwallet`":
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1883982591)
re: https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1869859432
> I think this might be a better candidate for a `createwalletdescriptor` (#29130) equivalent for external signers rather than continuing to jam more arguments into `createwallet`.
This makes sense, and I added this idea to my list of potential createwalletdescriptor extensions: https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435607572
But in general, as long as the createwallet `blank` option defaults
...
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1883982591)
re: https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1869859432
> I think this might be a better candidate for a `createwalletdescriptor` (#29130) equivalent for external signers rather than continuing to jam more arguments into `createwallet`.
This makes sense, and I added this idea to my list of potential createwalletdescriptor extensions: https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435607572
But in general, as long as the createwallet `blank` option defaults
...
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446759709)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446759709)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446759761)
Fixed
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446759761)
Fixed
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446759842)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446759842)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760427)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760427)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760540)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760540)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760584)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760584)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760618)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760618)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760656)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760656)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760708)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760708)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760770)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760770)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760887)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760887)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761034)
Updated the test to check this.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761034)
Updated the test to check this.
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761065)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761065)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761386)
I think that would require a much more invasive refactor as it requires accessing much lower level data. Perhaps for a followup.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761386)
I think that would require a much more invasive refactor as it requires accessing much lower level data. Perhaps for a followup.
📝 theStack opened a pull request: "test: assumeutxo: spend coin from snapshot chainstate after loading"
(https://github.com/bitcoin/bitcoin/pull/29215)
This PR extends the AssumeUTXO functional test by submitting a spending transaction for an UTXO that is only available in a the snapshot chainstate (after loading via `loadtxoutset`), i.e. it hasn't been seen in a block before. With that we can verify that snapshot coins are visible to the mempool.
Note that we unfortunately can't use MiniWallet here, as the only available UTXO to spend from the snapshot chainstate is at height 200, where a P2PKH created from the test framework's deterministi
...
(https://github.com/bitcoin/bitcoin/pull/29215)
This PR extends the AssumeUTXO functional test by submitting a spending transaction for an UTXO that is only available in a the snapshot chainstate (after loading via `loadtxoutset`), i.e. it hasn't been seen in a block before. With that we can verify that snapshot coins are visible to the mempool.
Note that we unfortunately can't use MiniWallet here, as the only available UTXO to spend from the snapshot chainstate is at height 200, where a P2PKH created from the test framework's deterministi
...
💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1446828496)
not sure why I didn't think of just using the value returned from `create_self_transfer` fixed in [24b490f](https://github.com/bitcoin/bitcoin/pull/28885/commits/24b490f292149e6151dcddaf11ac0d5bf16dde44)
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1446828496)
not sure why I didn't think of just using the value returned from `create_self_transfer` fixed in [24b490f](https://github.com/bitcoin/bitcoin/pull/28885/commits/24b490f292149e6151dcddaf11ac0d5bf16dde44)
🤔 ryanofsky reviewed a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1812307340)
Reviewed 756da6d1e67fba65a992dc0090ee8c0cfa26abe3 and this looks very good. So far I reviewed all the c++ code, but not the python or documentation yet.
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1812307340)
Reviewed 756da6d1e67fba65a992dc0090ee8c0cfa26abe3 and this looks very good. So far I reviewed all the c++ code, but not the python or documentation yet.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446841611)
In commit "rpc: identify JSON-RPC 2.0 requests" (9e1c6fa84eef606d5504ebcbbd2a2ca7a4f748dc)
The spec says this field 'MUST be exactly "2.0"' (https://www.jsonrpc.org/specification#request_object), so maybe it should be an error if the field is set and contains any other value. This way if there is ever a JSON-RPC 2.1 or 3.0 protocol, clients can easily check for support without getting back a strange JSON 1.1 response. FWIW, chatgpt suggestd the following response:
```json
{
"jsonrpc"
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446841611)
In commit "rpc: identify JSON-RPC 2.0 requests" (9e1c6fa84eef606d5504ebcbbd2a2ca7a4f748dc)
The spec says this field 'MUST be exactly "2.0"' (https://www.jsonrpc.org/specification#request_object), so maybe it should be an error if the field is set and contains any other value. This way if there is ever a JSON-RPC 2.1 or 3.0 protocol, clients can easily check for support without getting back a strange JSON 1.1 response. FWIW, chatgpt suggestd the following response:
```json
{
"jsonrpc"
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446812606)
In commit "rpc: refactor single/batch requests" (27afe01560b1d930c3b072a180cab95c857eff24)
This ret variable seems unnecessary. Can this just say `reply = UniValue{UniValue::VARR}` and use the `reply` variable instead of `ret` below?
The name is also confusing because `ret` normally refers to a return value, which this is not what this is.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446812606)
In commit "rpc: refactor single/batch requests" (27afe01560b1d930c3b072a180cab95c857eff24)
This ret variable seems unnecessary. Can this just say `reply = UniValue{UniValue::VARR}` and use the `reply` variable instead of `ret` below?
The name is also confusing because `ret` normally refers to a return value, which this is not what this is.