deprecate addresses and reqSigs from rpc outputs #6000
Comments
PastaPastaPasta
added a commit
that referenced
this issue
Jun 27, 2024
, bitcoin#21359, bitcoin#21910, bitcoin#19651, bitcoin#21934, bitcoin#22722, bitcoin#20295, bitcoin#23702, partial bitcoin#23706 (rpc backports) b23d94b partial bitcoin#23706: getblockfrompeer followups (Kittywhiskers Van Gogh) 7e5cc5e merge bitcoin#23702: Add missing optional to getblockfrompeer (Kittywhiskers Van Gogh) c294457 merge bitcoin#20295: getblockfrompeer (Kittywhiskers Van Gogh) 63ac87f merge bitcoin#22722: update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (Kittywhiskers Van Gogh) 07e4c2c merge bitcoin#21934: Include versionbits signalling details during LOCKED_IN (Kittywhiskers Van Gogh) 960e768 merge bitcoin#19651: importdescriptors update existing (Kittywhiskers Van Gogh) 1f31823 merge bitcoin#21910: remove redundant fOnlySafe argument (Kittywhiskers Van Gogh) 69c5aa8 merge bitcoin#21359: include_unsafe option for fundrawtransaction (Kittywhiskers Van Gogh) 169dce7 merge bitcoin#20286: deprecate addresses and reqSigs from rpc outputs (Kittywhiskers Van Gogh) 7cddf70 merge bitcoin#20867: Support up to 20 keys for multisig under Segwit context (Kittywhiskers Van Gogh) 7c59923 merge bitcoin#18344: Fix nit in getblockchaininfo (Kittywhiskers Van Gogh) ec0803a refactor: align `TxToUniv` argument list with upstream (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Closes #6000 * `TxToUniv`'s argument list needed to be rearranged to match upstream as closely as possible (i.e. placing Dash-specific arguments at the end of the list to allow for code to be backported unmodified, relying on default arguments instead of having to modify each invocation to insert the default argument in between). This was due to a new `TxToUniv` variant being introduced in [bitcoin#20286](bitcoin#20286) * The maximum number of public keys in a multisig remains the same. The upper limit for bare multisigs is and always has been `MAX_PUBKEYS_PER_MULTISIG` ([source](https://github.com/dashpay/dash/blob/19512988c6e6e8641245bd9c5fab21dd737561f0/src/script/interpreter.cpp#L1143-L1144)), which has always been 20 ([source](https://github.com/dashpay/dash/blob/19512988c6e6e8641245bd9c5fab21dd737561f0/src/script/script.h#L28-L29)). The limit of up to 16 comes from P2SH overhead ([source](https://github.com/dashpay/dash/blob/19512988c6e6e8641245bd9c5fab21dd737561f0/src/script/descriptor.cpp#L877-L880)) and that hasn't changed. In effect, what [bitcoin#20867](bitcoin#20867) does to Dash Core is change the error from "too many public keys" (as we'll be testing against the bare multisig limit) to "excessive redeemScript size" ([source](https://github.com/dashpay/dash/blob/19512988c6e6e8641245bd9c5fab21dd737561f0/src/rpc/util.cpp#L223-L225)) (which is the _true_ limitation). * Backporting [bitcoin#21934](bitcoin#21934) required a minor change in the condition needed to emit `activation_height` as `has_signal` in the preceding block will evaluate true for both `STARTED` and `LOCKED_IN` while earlier it would only for `STARTED`. * In [bitcoin#22722](bitcoin#22722), a `self.stop_node` had to be added to account for the absurd fee warning that a new test condition introduces. * [bitcoin#23706](bitcoin#23706) is partial due to commits in it that rely on RPC type enforcement, which is currently not implemented in Dash Core. * `feature_dip3_deterministicmns.py` depends on the reporting of `addresses` to validate multisigs containing expected payees. There is no replacement RPC call to report the pubkeys that compose a multisig address. In the interm, the `-deprecatedrpc=addresses` flag has been enabled to allow the test to run otherwise unmodified. ## Breaking Changes * The following RPCs: `gettxout`, `getrawtransaction`, `decoderawtransaction`, `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`, `/rest/block` deprecated the following fields (which are no longer returned in the responses by default): `addresses`, `reqSigs`. The `-deprecatedrpc=addresses` flag must be passed for these fields to be included in the RPC response. Note that these fields are attributes of the `scriptPubKey` object returned in the RPC response. However, in the response of `decodescript` these fields are top-level attributes, and included again as attributes of the `scriptPubKey` object. * When creating a hex-encoded Dash transaction using the `dash-tx` utility with the `-json` option set, the following fields: `addresses`, `reqSigs` are no longer returned in the tx output of the response. * The error message for attempts at making multisigs with >16 pubkeys will change to an "excessive redeemScript size" instead of the previous "too many public keys". ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b23d94b PastaPastaPasta: utACK b23d94b Tree-SHA512: 659d9ba3a0a9f8594b307a7056ab172309d5a0d9efec605bc4b345f7e0fb1032ad303af9e8f51dbd6549e82d0b738ad41eae8a5b3aebf59081f39df0d4b5ec7c
|
Thank you for addressing this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Please consider merging bitcoin#20286 and bitcoin#21797, which deprecates/removes
addressesandreqSigsfrom RPC outputs and introducesaddressinstead.This would simplify RPC client development, as the current Bitcoin core client only includes the
addressfield, while Dash still replies withaddresses.The text was updated successfully, but these errors were encountered: