backport: merge bitcoin#18344, #20867, #20286, #21359, #21910, #19651, #21934, #22722, #20295, #23702, partial bitcoin#23706 (rpc backports) #6074
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knst
reviewed
Jun 23, 2024
| @@ -277,8 +277,8 @@ CTxDestination AddAndGetMultisigDestination(const int required, const std::vecto | |||
| if ((int)pubkeys.size() < required) { | |||
| throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("not enough keys supplied (got %u keys, but need at least %d to redeem)", pubkeys.size(), required)); | |||
| } | |||
| if (pubkeys.size() > 16) { | |||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Number of keys involved in the multisignature address creation > 16\nReduce the number"); | |||
| if (pubkeys.size() > MAX_PUBKEYS_PER_MULTISIG) { | |||
There was a problem hiding this comment.
should we increase this amount? bitcoin increased it because they have segwit -> the transaction validation works faster for segwit txes. will it affect performance for us?
There was a problem hiding this comment.
The limit isn't actually going up, under the hood, the limit has always been up to 20 (source, MAX_PUBKEYS_PER_MULTISIG has always been 20), the overhead involved with P2SH is what whittles that number down to 16 (source). Checking against 20 instead of 16 already takes place in dash-tx (source).
In effect, what the backport does is change the error from "too many public keys" (by testing against the without-overhead value as opposed to accounting for overhead) to "excessive redeemScript size" (source) (which is the actual upper bound when accounting for P2SH).
That is what "opens up" up to 20 pubkeys for multisig for SegWit but since we don't have that, it doesn't open up anything, just changes the error.
UdjinM6
requested changes
Jun 25, 2024
UdjinM6
previously approved these changes
Jun 27, 2024
|
This pull request has conflicts, please rebase. |
UdjinM6
approved these changes
Jun 27, 2024
In bitcoin#20286, a new `TxToUniv` will be defined and in order to minimize upstream deviation caused by having to change function calls to account for `const CSpentIndexTxInfo*` being placed _before_ `const CTxUndo*` (requiring us to therefore specify `nullptr` as those calls do not expect spend information) instead of letting the default value remain. To allow for that, their ordering will be swapped. To prevent a confusion with the last two arguments between the `core_io` definition and the `rpc/blockchain` definition, let's do the inversion here too before the backport.
…imateSmartFee, mempoolMinFee and minRelayTxFee.
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 23, 2024
, bitcoin#23174, bitcoin#23785, bitcoin#23581, bitcoin#23974, bitcoin#22932, bitcoin#24050, bitcoin#24515 (blockstorage backports) 1bf0bf4 merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh) 5c1eb67 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh) c440304 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh) e303a4e merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh) 301163c merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh) 732e871 merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh) b402fd5 merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh) a08f2f4 merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh) 472caa0 merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh) d69ca83 merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh) 6df927f chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6078 * Dependent on #6074 * Dependent on #6083 * Dependent on #6119 * Dependency for #6138 * In [bitcoin#24050](bitcoin#24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic. * Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/spork.cpp#L44)) and upstream's usage of it to process translatable strings ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/util/translation.h#L55-L62)). Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore. ## Breaking Changes None expected ## 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 **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1bf0bf4 (with one nit) knst: utACK 1bf0bf4 PastaPastaPasta: utACK 1bf0bf4 Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Dec 3, 2024
…respect `nCoinType` e5114da fix: coin selection with `include_unsafe` option should respect `nCoinType` (UdjinM6) Pull request description: ## Issue being fixed or feature implemented The issue was introduced in #6074 via 69c5aa8 ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK e5114da kwvg: utACK e5114da Tree-SHA512: 5d4e22f9d2cecf2239185e0f4c9d8b29b995b25b4f53a74d6c9b7929aac6ec918ebfb4029a83b72a003fe42fe82619f7ab4892d620bf5846cadf99f1f0cb0969
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Dec 3, 2024
…should respect `nCoinType` e5114da fix: coin selection with `include_unsafe` option should respect `nCoinType` (UdjinM6) Pull request description: ## Issue being fixed or feature implemented The issue was introduced in dashpay#6074 via dashpay@69c5aa8 ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK e5114da kwvg: utACK e5114da Tree-SHA512: 5d4e22f9d2cecf2239185e0f4c9d8b29b995b25b4f53a74d6c9b7929aac6ec918ebfb4029a83b72a003fe42fe82619f7ab4892d620bf5846cadf99f1f0cb0969
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additional Information
Closes deprecate addresses and reqSigs from rpc outputs #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
TxToUnivvariant being introduced in bitcoin#20286The 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), which has always been 20 (source). The limit of up to 16 comes from P2SH overhead (source) and that hasn't changed.In effect, what 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) (which is the true limitation).
Backporting bitcoin#21934 required a minor change in the condition needed to emit
activation_heightashas_signalin the preceding block will evaluate true for bothSTARTEDandLOCKED_INwhile earlier it would only forSTARTED.In bitcoin#22722, a
self.stop_nodehad to be added to account for the absurd fee warning that a new test condition introduces.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.pydepends on the reporting ofaddressesto 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=addressesflag 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/blockdeprecated the following fields (which are no longer returned in the responses by default):addresses,reqSigs.The
-deprecatedrpc=addressesflag must be passed for these fields to be included in the RPC response. Note that these fields are attributes of thescriptPubKeyobject returned in the RPC response. However, in the response ofdecodescriptthese fields are top-level attributes, and included again as attributes of thescriptPubKeyobject.When creating a hex-encoded Dash transaction using the
dash-txutility with the-jsonoption set, the following fields:addresses,reqSigsare 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: